halo2 icon indicating copy to clipboard operation
halo2 copied to clipboard

Move `Params` from field to function

Open kilic opened this issue 2 years ago • 1 comments

This PR basically moves the Params field from MSM and adds it as an argument to eval function of MSM. With this change reference to ecc point parameters are passed only when they are actually used. So that we can get rid of 'params bound for good.

Not implemented in this PR but I observe that params argument can be also replaced with parameter k at plonk::verify_proof, poly::multiopen::verify_proof and poly::commitment::verify_prooffunctions if it is found as a simplification.

kilic avatar Jun 26 '22 16:06 kilic

@ebfull and I discussed this in a pairing. Initially we were reluctant to accept this PR, because the intent of storing the parameters was to ensure they stayed self-consistent across the API usage, for improved safety. However, there are two things in the current API inconsistent with this desire:

  • The proving and verifing APIs take the params and the proving/verifying key separately, intentionally so that use cases with many different circuits can share a single params in memory. We don't enforce consistency between these.
  • We don't enforce in the batch logic that the MSMs are using the same parameters.

The other influencing factor on us changing our mind was:

Not implemented in this PR but I observe that params argument can be also replaced with parameter k at plonk::verify_proof, poly::multiopen::verify_proof and poly::commitment::verify_prooffunctions if it is found as a simplification.

It's not quite this simple: we can't just take k as a parameter, because in plonk::verify_proof we also use params.commit_lagrange() and we don't want to compute g_lagrange on the fly. However, it is indeed the case that in a verifying-only context, we do not need the full Params structure inside plonk::verify_proof, and could omit g. We do need g in a single-verifier context though, in which case we don't save much if anything, but even then we only need it within SingleVerifier rather than plonk::verify_proof so there are maybe some chances for savings in certain usages.

So on balance we think this PR is fine to consider. Still needs a review from one of us (likely @ebfull as this will interact with the refactoring work he is doing).

str4d avatar Jun 29 '22 19:06 str4d