Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Groth16 Solidity verifier improvements #1094

Open
1 of 7 tasks
ivokub opened this issue Apr 5, 2024 · 0 comments
Open
1 of 7 tasks

refactor: Groth16 Solidity verifier improvements #1094

ivokub opened this issue Apr 5, 2024 · 0 comments
Assignees
Labels
consolidate strengthen an existing feature

Comments

@ivokub
Copy link
Collaborator

ivokub commented Apr 5, 2024

With #1063 being merged, there are still a few open issues to solve. This issue lists all the related sub-issues

  • the method NbPublicWitness returns the number of K elements 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 affect gnark-solidity-checker as it doesn't actually use --commitments parameter.
  • add method NbCommitments to 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 the ExportSolidity method on groth16.Proof to use smaller types for representing lengths (I think uint8 could actually be sufficient)
  • with the previous modification, it is possible to drop the --commitments argument from gnark-solidity-checker tool 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 in witness.Witness for exporting compatible []*big.Int output which can be easily passed to Solidity verifier bindings. Recommendation is to add ExportSolidity etc. to witness.Witness interface.
  • 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 WithSolidityProverOption etc. 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 stack too deep error. Currently we have omitted CI tests for such circuits (mostly in std/math/polynomial pacakge 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. (fixed in feat: groth16 solidity use calldatacopy for commitments #1097)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consolidate strengthen an existing feature
Projects
None yet
Development

No branches or pull requests

1 participant