-
Notifications
You must be signed in to change notification settings - Fork 516
refactor: Groth16 Solidity verifier improvements #1094
Copy link
Copy link
Open
Labels
type: consolidatestrengthen an existing featurestrengthen an existing feature
Description
With #1063 being merged, there are still a few open issues to solve. This issue lists all the related sub-issues
- the method
NbPublicWitnessreturns the number ofKelements in the key, but this may mismatch the actual number of assignable number of public witness elements in case commitments are used. But for PLONK backend, the method returns correct output. This leads to a mismatch in Solidity test where for PLONK we compute wrong number of commitments used, but fortunately it doesn't affectgnark-solidity-checkeras it doesn't actually use--commitmentsparameter. - add method
NbCommitmentsto the proving/verification key for Groth16 and PLONK. - currently there is API mismatch between Groth16 and PLONK solidity verifier. PLONK solidity verifier takes as inputs the serialized proof as byte array, but Groth16 verifier expects an array of
uint256. This means that we have to manually convert between serialized gnark proof and what Solidity verifier expects (and this may lead to invalid inputs). I think it would be better to also takes as input the serialized proof and additionally remove the conditional method signature difference depending if the circuit uses commitments or not (as it can be deduced from the proof itself). Now, the problem may be that the serialized proof is not the most gas-optimal as it includes as array header the number of items in the array, which may have a lot of zeroes. But we can change theExportSoliditymethod ongroth16.Proofto use smaller types for representing lengths (I think uint8 could actually be sufficient) - with the previous modification, it is possible to drop the
--commitmentsargument fromgnark-solidity-checkertool as the Solidity contract should parse sufficient number of commitment elements from the proof directly. For me it seems a little error-prone that we can provide mismatching arguments (e.g. proof has N/X commitment elements and public witness elements, but arguments define M/Y). - Both Groth16 and PLONK Solidity verifiers takes as public inputs
uint256[], but the input is actually somewhat difficult to construct. We do not have corresponding export method inwitness.Witnessfor exporting compatible[]*big.Intoutput which can be easily passed to Solidity verifier bindings. Recommendation is to addExportSolidityetc. towitness.Witnessinterface. - Currently PLONK and Groth16 Solidity verifiers use different hash-to-field functions. We have fixed the default HTF to be the one compatible with PLONK Solidity verifer, but this requires passing in prover/verifier options for Groth16 prover and native verifier. Maybe, instead of having to fix the hash function directly we can have an option (similarly to what we have for the recursive in-circuit verifier) a la
WithSolidityProverOptionetc. which fixes all the parameters just to be right. This also allows to upgrade the HTF used in Solidity verifier in the future with less breakage. I currently do not know where to put this option though. See also feat: allow configurable hash-to-field function for Groth16 Solidity verifier #1102 which adds support to different hash functions in Solidity. -
Related to previous, currently Groth16 verifier uses SHA2 for hashing commitments to field. I think in general SHA2 is good, but the issue is when we have many inputs to the commitment, then we get(fixed in feat: groth16 solidity use calldatacopy for commitments #1097)stack too deeperror. Currently we have omitted CI tests for such circuits (mostly instd/math/polynomialpacakge which uses non-native arithmetic), but it would be better to fix. Either if it possible to somehow chain SHA2 calls or use keccak-f instead.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
type: consolidatestrengthen an existing featurestrengthen an existing feature