Circuit-preserving code improvements#92
Merged
Merged
Conversation
The top-level README described the crate as "built with halo2 and a local fork of Orchard" and pointed at a non-existent `orchard/` directory "at the workspace root". Reality (per Cargo.toml + Cargo.lock): the crate depends on upstream `orchard = "=0.13.1"` from crates.io with the `unstable-voting-circuits` feature, and the valar-orchard fork has been retired with no [patch.crates-io] override remaining in tree. Replace the "Companion crate" section with a "Dependency on `orchard`" section reflecting actual ground truth, soften the line-5 wording from "local fork" to "on top of upstream", and drop the "(local)" annotation from the key-deps table. The sub-level voting-circuits/README.md still claims a [patch.crates-io] pin to valargroup/orchard, which is also stale; that will be folded into the upcoming README unification commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The repo holds a single Rust crate that was nested at
voting-circuits/voting-circuits/. There is no Cargo workspace and no
sibling crate, so the extra path layer was redundant: every CI step had
to set `working-directory: voting-circuits`, every README command needed
`--manifest-path voting-circuits/Cargo.toml`, and the two READMEs had
drifted apart with conflicting claims about the project.
Move Cargo.toml, Cargo.lock, CHANGELOG.md, src/, tests/, and benches/
up one level via `git mv` so file history follows. Drop the
`working-directory` defaults and `Swatinem/rust-cache@v2` `workspaces:`
hints from .github/workflows/test.yml (all three jobs), and strip
`--manifest-path voting-circuits/Cargo.toml` from the three row-budget
doc-comment invocations in src/vote_proof/circuit.rs.
Unify the two READMEs into a single top-level file that keeps the
proof-flow diagram, package layout, shared-gadgets table, circuit-
details table, and key-dependencies table from the old top-level
README, and folds in the wrapper-crate pointer, MSRV note, domain-tags
pointer, orchard-dependency note, and license section from the old
sub-level README. Build/test/bench commands are now plain `cargo X`
with no `--manifest-path`, and the license links resolve to the repo
root (no `../`).
Contradictions resolved while merging:
- Protocol name: top-level said "Zally voting protocol"; sub-level
and Cargo.toml description said "Zcash shielded-voting protocol".
Resolved to "Zcash shielded-voting protocol".
- Orchard dependency: top-level "local fork" claim was fixed in the
previous commit; sub-level [patch.crates-io] pin claim was also
stale (Cargo.toml has no [patch] section, Cargo.lock pulls
orchard 0.13.1 from crates.io). Unified text reflects the actual
state: upstream =0.13.1 with the unstable-voting-circuits feature,
valar-orchard fork retired.
- Build/test commands: collapsed to plain `cargo build` / `cargo
test` / `cargo bench` (no `--manifest-path`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nothing in the repo references `local-crates/` — no Cargo.toml `[patch]`
block, no CI step, no script, no docs. It looks like a developer
workflow hint for swapping in local crate checkouts via
`[patch.crates-io] = { path = "local-crates/..." }`, but that's a
personal pattern, not a project convention.
Per-developer ignores belong in .git/info/exclude (or a global
core.excludesFile), not in the tracked .gitignore. If you want this
back, add it to one of those instead and everyone else won't have to
wonder what it's for.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Allow compatible patch releases of orchard rather than pinning exactly, so downstream consumers can pick up bug fixes without a version bump here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audit every `pub(crate)` and downgrade items that didn't need crate-wide visibility. The bulk of items only used in their own file become private; items used only within a single subtree (delegation/, vote_proof/, share_reveal/) become `pub(super)`. `pub(crate) mod foo;` declarations become plain `mod foo;` where the parent already controls all external re-exports — making the curated `mod.rs` re-export boundary enforced rather than informal. Also downgrade inactive `pub` markers — `pub` items in submodules that weren't re-exported from the parent `mod.rs`, so the `pub` was unreachable from outside the crate. `SENTINEL_EXPONENT`/`SENTINEL_COUNT` become private; `domain_van_nullifier` and the `Circuit::with_*` builder chain (plus `NoteSlotWitness`) become `pub(super)`; the delegation `Config` is added to the `mod.rs` re-exports for consistency with the other two circuits. No behavior change; all 137 lib tests + 2 integration tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The protocol's ballot divisor (12,500,000 zatoshi per ballot) was defined twice — once in delegation::circuit and once in vote_proof::builder, each with a comment instructing the reader that they must match. Collapse to one definition in delegation (where ballot scaling is first applied), re-export it through `delegation::BALLOT_DIVISOR` for crate-internal use, and import it from vote_proof::builder. No more "must match" comments; the import is self-documenting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`delegation::InstanceError` was reachable only via wildcard match on `DelegationBuildError::Instance(_)`; external callers couldn't name the inner variant. Re-export it from `delegation/mod.rs` so they can match `DelegationBuildError::Instance(InstanceError::RkIsIdentity)` by name. Also drop the stale "Lives inside the orchard crate to access `pub(crate)` key internals" line from the vote_proof builder module docstring — left over from when this code lived as a sub-crate. The remaining docstring describes what the module actually does. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`BALLOT_DIVISOR` and `VOTE_COMM_TREE_DEPTH` are protocol-wide parameters shared across the three ZKP circuits, but were anchored inside `delegation` and `vote_proof` respectively, forcing siblings to reach across modules to use them. Likewise `spend_auth_g_affine`, `shares_hash`, `share_commitment`, and `poseidon_hash_2` were public helpers only reachable through `vote_proof`'s laundered re-exports. Introduce a top-level `params` module for the shared constants, and re-export the four helpers directly from the crate root. The proof modules now import each item from its canonical home. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename src/circuit/ to src/gadgets/ and keep only the modules used by two or more circuits there. Move circuit-specific gadgets into a new `gadgets` submodule inside their owning circuit: delegation/gadgets/ gadget, mul_chip, imt_circuit vote_proof/gadgets/ authority_decrement share_reveal has no per-circuit gadget files, so it gets no submodule. Non-gadget circuit-specific items (offline data structures, builders, prove/verify wrappers, top-level Circuit structs) are unaffected. No public API change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promote the remaining `pub(crate)` items in `domain_tags` to `pub` so every item in the module uses the same qualifier. The module itself is declared `mod domain_tags;` at the crate root, which gates external reach to crate-internal regardless of per-item markers — letting the module declaration be the single point that controls visibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Configure crate-level allows for lints that don't fit halo2 circuit code (too_many_arguments, type_complexity, needless_range_loop) via Cargo.toml [lints.clippy], remove the now-redundant per-function #[allow] attrs, and fix the remaining lints inline (manual_memcpy, manual_range_contains, cloned_ref_to_slice_refs, single_match, unnecessary_sort_by, unnecessary_map_or, new_without_default, empty_line_after_doc_comments). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Making `params` private hid `VOTE_COMM_TREE_DEPTH`, which downstream consumers reach for to size vote commitment Merkle paths. Re-export just the depth constant at the crate root and keep the rest of `params` internal. Update the unreleased changelog entry to point at the new canonical path and drop the now-incorrect mention of a public `params` module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These were exposed during the protocol-item lift but have no external consumers — only the in-tree circuits use them. Demote both to `pub(crate)` so the public API only carries the items downstream code actually depends on, and remove the matching Unreleased changelog bullets that advertised the renames. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The delegation circuit's condition 8 range-checks `remainder < 2^24`, which is looser than `BALLOT_DIVISOR = 12_500_000`. That slack admits a second satisfying witness `(num_ballots - 1, remainder + BALLOT_DIVISOR)` in ~34% of `v_total` values, so the proven relation is "floor-division or one less" rather than exact floor-division. The deviation is one-sided (over-claim is impossible) and self-harming, so the circuit ships as-is; this commit only updates docs and adds a CHANGELOG entry. Adds an authoritative "Soundness scope" + "Tightening options" subsection to `src/delegation/README.md` §8 and points every other site that asserted exact floor-division at it via brief cross-references. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audit of `cargo public-api diff v0.6.0..HEAD` turned up several public API deltas not reflected in the Unreleased section: two `vote_proof` free functions dropped entirely, four `delegation::Circuit` builder methods narrowed to `pub(super)`, and three additive items (`InstanceError`/`Config` re-exports, `Default` for `SpacedLeafImtProvider`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surveys zcash_voting and vote-sdk's actual imports and drops the items neither references: vote_proof's VoteProofBuildError / create_vote_proof / Config / *_PUBLIC_OFFSET constants, share_reveal's verify_share_reveal_proof / Config / *_PUBLIC_OFFSET constants, and delegation's SyntheticPaddingNoteParts / *_PUBLIC_OFFSET / GOV_NULL_PUBLIC_OFFSETS. delegation::build_nullifier_list moves behind a new unstable-internal-api Cargo feature so the in-tree IMT integration test can still reach it. share_commitment relocates to the crate root to match the layout used for the other shared protocol helpers. The CHANGELOG's Unreleased section documents the specifics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Group std, external crates, then super/crate; merge nested imports where applicable. Pure formatting cleanup, no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The vote-proof and delegation layers maintained Affine + Projective pairs of the same point in several places (ea_pk, note commitments, ak_P), and recomputed projective forms from affine inputs solely to perform scalar multiplication. Pasta supports Affine * Scalar and mixed-addition natively, so the projective copies were pure overhead. Also folds r_vpk = G*vsk + G*alpha_v into the equivalent G*(vsk + alpha_v), removes the no-op Affine -> Projective -> Affine round-trip in spend_auth_g_affine, and decodes rk directly into affine in Instance::from_parts instead of via projective. Witness values, public inputs, circuit constraints, and the verification key are unchanged.
Hashes the `PinnedVerificationKey` debug repr (which is halo2's stable
identity view of a VK — constraint system, fixed commitments, permutation
argument, evaluation domain) with blake2b-256 and asserts against a pinned
constant. Catches accidental VK drift from refactors that *shouldn't* move
the circuit shape; an intentional shape change must update the constant
alongside the regenerated/redistributed VK.
Marked TODO(sean) and `#[ignore]`-gated, matching the existing convention
for keygen-heavy tests. Run with:
cargo test --release -- --ignored vk_fingerprint_unchanged
Baselines verified to also pass at the v0.6.0 tag.
These ~34 offset constants are an in-file reference for the circuit author, not part of the public API: their parent `circuit` submodules are privately declared and neither `zcash_voting` nor `vote-sdk` names any `*_PUBLIC_OFFSET`. Also demote `vote_proof::Circuit::with_van_witnesses` to `pub(super)` (only its sibling builder uses it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The chip lives at src/vote_proof/gadgets/authority_decrement.rs, not alongside circuit.rs. Three doc-comment references were missing the gadgets/ prefix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documentation, comments, and tests only — the constraint system,
gates, lookup table, and region layout are unchanged.
Doc corrections in the module-level comment:
- Renumbered constraints (6) and (7) to close the gap that ran
(5) → (8). Subsequent constraints shifted from (8)–(17) to (6)–(15).
- "Together (1)+(16) prove proposal_id is a valid index in [1,15]"
was wrong: (16) is the rsel_pow copy constraint, which does not
exclude pid=0. The non-zero gate (2) is what does. Now reads
"Together (1)+(2)".
- "Together (10)+(15) prove the old authority had that bit set" was
also wrong: (10) is about b_new, not about whether b_pid was set.
The actual argument is via the rseld recurrence and terminal gate.
Now cites "(10)+(13), given one-hot sel".
- The one-hotness and auth_new attribution lines now cite the full
supporting set including the two_pow_i ladder and b_new derivation.
- Terminal gate (13) now explicitly notes its reliance on (14)'s
one-hot property.
- Documented two implicit range checks: copy constraint (15) bounds
proposal_authority_old to 16 bits, and the same argument bounds
the returned rnew[15] to 16 bits.
- Abbreviations block now maps doc-side names (rseld, rsel_pow, ...)
to code-side struct fields (run_selected, run_sel_pow, ...).
- Constraint (8) now notes that b_new ∈ {0,1} follows implicitly
from b * (1-sel) with both boolean — no separate bool_check.
Comment / docstring corrections:
- AuthorityDecrementChip::configure now documents that callers must
have enabled a constant column via meta.enable_constant, in
addition to the existing equality requirement.
- The "fallback zero is irrelevant" comment in the pid_inv assignment
was understated. Replaced with the correct argument that the
q·(1 − pid·inv) gate evaluates to q ≠ 0 for any choice of inv
when pid = 0, so the fallback is safe.
- AUTHORITY_DECREMENT_LOOKUP_TABLE now carries a note explaining
that the (0, 1) row is load-bearing as the q_cond_6 = 0 fallback
target, even though pid = 0 is rejected by the separate non-zero
gate.
- proposal_id_inv config field is now clearly marked as an alias
for advices[2] rather than a separate column.
- "// Rows 2..17" in-code comment corrected to "// Rows 2..=16" to
match the documented row range.
Visibility tightening:
- All eight AuthorityDecrementConfig fields are now private. Nothing
outside this file ever read them; the chip's configure / load_table
/ assign API encapsulates every interaction with these wires. The
struct itself remains pub(in crate::vote_proof) so circuit.rs can
still name the type.
New / improved tests:
- auth_old_above_16_bits_fails covers proposal_authority_old values
with the 17th bit set, only-bit-16, and u64::MAX. The recomposition
equality forces auth_old into [0, 0xFFFF] as a side effect of the
bit decomposition; this test pins that load-bearing behavior.
- lookup_table_contents_frozen gained an explanatory docstring
calling out the (0, 1) entry's q_cond_6 = 0 fallback role.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 203bd4fcb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
12
to
+13
| # orchard 0.13.1 declares the MSRV and exposes the unstable governance circuit APIs. | ||
| orchard = { version = "=0.13.1", features = ["circuit", "unstable-voting-circuits"] } | ||
| orchard = { version = "0.13.1", features = ["circuit", "unstable-voting-circuits"] } |
There was a problem hiding this comment.
This changes the previous exact =0.13.1 requirement into Cargo's caret range, so downstream builds or fresh lockfiles may resolve any future 0.13.x Orchard release. The circuits import Orchard constants and unstable circuit APIs directly, so an API-compatible patch release can silently change circuit shape or verification-key fingerprints while this crate still advertises the same circuit version; for circuit-preserving releases, keep Orchard exact-pinned just like the reference IMT dependency or otherwise lock the verifying-key-critical dependency set.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
This was done intentionally here https://github.com/valargroup/voting-circuits-internal/pull/1/changes/6f03a0c256fce4c13002bcd6ecd8c0dc5ae209f9
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
2 participants
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.
Summary
Soundness
This is intended to preserve the circuit shape. The only newly documented soundness caveat is the existing delegation condition 8 behavior: a voter can under-claim by one ballot for some values, but cannot over-claim voting power.
Validation
cargo fmt --all --checkcargo test --locked --no-fail-fastcargo test --locked -- --ignored --skip row_budget --skip cost_breakdown --test-threads=1cargo test --locked --features unstable-internal-api --test imt_circuit_integration --no-fail-fastcargo test --locked --features unstable-internal-api --test imt_circuit_integration -- --ignored --test-threads=1Known follow-up before release: coordinate the breaking public API changes with downstream consumers and versioning.