Open
Conversation
altergui
changed the title
altergui
force-pushed
the
even-more-fixes
branch
from
541ca93 to
35321e1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple issues across the circuits package, including public key validation, VoteID range validation, and results subtraction validation. It updates dependencies to lean-imt-go v0.0.4-rc1 and gnark-crypto-primitives v0.0.4, with corresponding code updates to support new API changes and fixes.
Changes:
- Add public key validation in voteverifier to ensure keys are on-curve and not at infinity
- Add VoteID range validation in state transition circuits
- Add subtraction underflow validation in results verification
- Update dependency versions and corresponding code to use new APIs
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod, go.sum | Update lean-imt-go to v0.0.4-rc1 and gnark-crypto-primitives to v0.0.4 |
| crypto/signatures/ethereum/signer.go | Add PublicKeyCoordinates() method to extract secp256k1 public key coordinates |
| crypto/signatures/ethereum/signer_test.go | Add import and tests for new PublicKeyCoordinates() method |
| circuits/voteverifier/vote_verifier.go | Add assertValidSecp256k1PublicKey() and call it in verifySigForAddress() |
| circuits/voteverifier/dummy.go | Update dummy values to use actual signer and public key coordinates |
| circuits/test/voteverifier/public_key_validation_test.go | Add test for off-curve public key rejection |
| circuits/statetransition/statetransition.go | Add VoteID range validation |
| circuits/statetransition/dummy.go | Update MerkleProof dummy with new fields matching lean-imt-go v0.0.4-rc1 |
| circuits/results/results.go | Add assertion that SubAccumulators ≤ AddAccumulators |
| circuits/results/results_test.go | Add test for subtraction underflow detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
altergui
force-pushed
the
even-more-fixes
branch
2 times, most recently
from
af85cc5 to
f34cbda
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
this brings * fix(smt): bind path bits to key * fix(smt): constrain IsOld0 to boolean
changes: * fix(circuit): range-constrain address and weight on PackLeaf * fix(circuit): bind lean IMT proof shape in-circuit
Add an explicit secp256k1 on-curve and non-infinity check before ECDSA verification and address derivation in the vote verifier circuit.
…and statetransition Enable groth16.WithSubgroupCheck() in the recursive verifiers so malformed inner proof points cannot satisfy the weaker no-subgroup-check relation. The previous behavior relied on off-circuit validation in the sequencer pipeline, but these circuits should remain sound even under a malicious-prover model.
Advance the reencryption scalar for each field when constructing encrypted-zero ballots, and mirror the same schedule in-circuit. This removes the preserved pairwise-difference fingerprint left by reusing the same offset across all ciphertext fields in a ballot.
Remove the host-side early return for all-identity ballots so reencryption always adds encrypted zero, matching the circuit implementation. Add a regression test to ensure zero ballots are rerandomized instead of being returned unchanged.
Range-constrain the results accumulators to the BabyJub subgroup order before they are used as scalar inputs. This prevents wrapped field values from being interpreted as valid tally deltas under a weaker mod-field relation than the host-side arithmetic expects. Tweaked testutil NewVoteForTest to cover the edge case of a zero Result
Make BigInt.IsInField enforce the canonical range 0 <= x < p instead of only checking the upper bound. Negative integers are valid modulo-field representatives, but they are not the canonical encoding used for API validation and ballot input hashing. Values like -1 must be normalized to p-1 via ToFF before they should pass an IsInField check.
Add a 160-bit range check before comparing the packed BN254 address witness against the Ethereum address derived from the secp256k1 public key. This closes the aliasing gap where multiple larger BN254 elements could pack to the same compiler-field value even though only the canonical 20-byte address should be accepted. A regression test now covers the address + r_BLS12_377 case.
Rename the aggregator public input that tracks the real vote prefix from ValidProofs to VotersCount. This matches the surrounding circuit terminology, where real slots are described as voters or real votes and the remaining slots are valid dummy padding rather than invalid proofs. Also introduce AggregatorCircuit.VoteMask() so the latch-based real-slot mask matches the shape already used in state transition. Update the aggregator circuit, sequencer assignment construction, and dummy/test harnesses accordingly.
altergui
force-pushed
the
even-more-fixes
branch
3 times, most recently
from
9c7beaf to
49a4a70
Compare
Make padded slots an outer aggregator concern instead of an inner voteverifier mode. Use placeholder proofs for padded slots, remove the voteverifier IsValid public input, and update the recursive witness shape to publish only ballot-hash limbs. This also drops the old voteverifier dummy fixture, keeps regression coverage for padded-slot batch-hash binding and witness shape, and simplifies sequencer/test assignment setup.
altergui
force-pushed
the
even-more-fixes
branch
from
49a4a70 to
8adf9b4
Compare
altergui
force-pushed
the
even-more-fixes
branch
from
e479ecb to
c021643
Compare
Mask padded slots to the placeholder ballot-hash constant before using BallotHashes in the public BatchHash commitment. Reuse the same masked hashes for recursive proof witnesses so the public BatchHash and recursive verification are bound to the same values. Add a regression test covering a forged padded-slot hash.
altergui
force-pushed
the
even-more-fixes
branch
from
c021643 to
64ec606
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.