Governance wallet support for shielded voting (rebased onto 95b000e, pre-orchard 0.13)#40
Closed
p0mvn wants to merge 13 commits into
Closed
Governance wallet support for shielded voting (rebased onto 95b000e, pre-orchard 0.13)#40p0mvn wants to merge 13 commits into
p0mvn wants to merge 13 commits into
Conversation
Co-Authored-By: roman <roman@osmosis.team> Co-Authored-By: Claude Code <noreply@anthropic.com> Remove duplicate shielded_sighash getter in pczt signer Bad merge left two identical methods. Keep the one with the fuller doc comment, fix stray braces in doc comments on nearby methods.
2 tasks
…otes `WalletDb::get_unspent_orchard_notes_at_historical_height` returns all Orchard notes that existed and were unspent at a given block height for a specified account. Unlike `select_unspent_notes` (which applies confirmation, dust, and expiry filters for transaction construction), this provides an unfiltered view of the historical note set—useful for auditing or reconstructing wallet state at a past point in time. Made-with: Cursor
Add `WalletDb::generate_orchard_witnesses_at_historical_height`, which produces Orchard Merkle witnesses anchored at an earlier tree state. This is needed for applications such as token-holder voting, where the wallet tree has advanced past the height at which witnesses are required. The implementation copies shard and cap data into an ephemeral in-memory database, inserts the caller-provided frontier as a checkpoint, and generates a witness for each requested note position. The wallet DB is strictly read-only. Made-with: Cursor
Moves `Marking`, `ShardTree`, `Checkpoint`, and `ORCHARD_SHARD_HEIGHT` imports out of `generate_orchard_witnesses_at_historical_height` to the module-level import block, consistent with the rest of the codebase. Made-with: Cursor
…ration Replaces the in-memory SQLite scratch DB used by `generate_orchard_witnesses_at_historical_height` with shardtree's `MemoryShardStore`. The wallet DB shards and cap are loaded directly into the in-memory `ShardStore`, avoiding the round-trip through SQLite DDL, row-by-row blob copying, and the dependency on `TABLE_ORCHARD_TREE_*` schema constants. Behavior is preserved: the same `LocatedPrunableTree` values end up in the store (both paths go through `get_shard`/`get_cap`), the ShardTree algorithms are unchanged, and the public API is identical. A side benefit is that corrupt shard blobs are now detected at load time rather than lazily on first access. Addresses review feedback from @nuttycom on zcash#2283. Made-with: Cursor Co-Authored-By: Claude <noreply@anthropic.com>
p0mvn
force-pushed
the
roman/shielded-vote-for-zodl-3.4.0-rebased
branch
from
11533e8 to
d7ade3e
Compare
…tness generation
`ShardTree::insert_frontier_nodes` registers a checkpoint internally
when called with `Retention::Checkpoint { id, .. }`, so the explicit
`tree.store_mut().add_checkpoint(height, ...)` immediately after it
was dead weight at best and confusing at worst: a reader is left
wondering whether the frontier insertion really registered the
checkpoint and why we're doing it again.
Drop the redundant call (and the now-unused `frontier_position`
binding) and replace the "Insert frontier + checkpoint" comment with
one that names `Retention::Checkpoint` as the mechanism doing the
work.
Also move the `Marking` and `ShardTree` imports behind
`#[cfg(feature = "orchard")]`: they are only used inside the
orchard-gated function, and the top-level imports produced
`unused_imports` warnings under `cargo check --no-default-features`.
Behavior is unchanged in all feature configurations.
Addresses review feedback from @nuttycom on zcash#2283.
Made-with: Cursor
Co-Authored-By: Claude <noreply@anthropic.com>
…itnesses_at_historical_height
Previously the function funneled every internal `ShardTree` failure
through `SqliteClientError::CorruptedData(String)`, including the
`(wallet may need to sync through historical height)` hint. That was
misleading on two counts:
- `CorruptedData` is documented for wallet-database corruption, not
for recoverable caller conditions such as a frontier mismatch or an
out-of-sync wallet.
- Collapsing every case to a formatted string forces the caller to
string-match on the message to tell "the wallet hasn't reached this
height yet" from "the supplied frontier is wrong", which is both
fragile and not actionable.
This commit adds two `orchard`-gated variants to `SqliteClientError`
that name the remaining failure modes precisely:
- `HistoricalFrontierInvalid(shardtree::error::InsertionError)` — the
caller-supplied frontier is inconsistent with the shard data
reconstructed from the wallet. Wraps the underlying
`InsertionError` and surfaces it through `Error::source`.
- `HistoricalWitnessUnavailable { position, height }` — no witness
can be produced for the specified position at the specified height
(most commonly because the wallet has not synced through that
height, the checkpoint was pruned, or the position does not belong
to the wallet).
Shard-read failures already had a home in
`SqliteClientError::CommitmentTree`, so those now flow through it
directly via the existing `From<ShardTreeError<commitment_tree::Error>>`
impl instead of being stringified. The only `ShardTreeError` variant
that cannot occur in this function is `Storage(Infallible)` — the
in-memory store's error type is `Infallible`, so those arms are
`match inf {}`.
Docs for `WalletDb::generate_orchard_witnesses_at_historical_height`
and the internal `pub(crate)` helper now include `# Errors` sections
that enumerate the concrete variants, and `CHANGELOG.md` records the
new public variants.
`sqlite_client_error_to_wallet_migration_error` is updated to
exhaustively match these new variants (both `unreachable!()` — we do
not generate historical witnesses in migrations).
Addresses review feedback from @nuttycom on zcash#2283.
Made-with: Cursor
Co-Authored-By: Claude <noreply@anthropic.com>
Changes the workspace-level `orchard` dep from the crates.io `orchard`
package (patched to zcash/orchard git rev 6b12c77) to the
`valar-orchard` package on crates.io, aliased locally to `orchard` via
cargo's `package =` rename trick. Every member keeps its existing
`orchard.workspace = true` line and its source-level `use orchard::…`
imports — no other changes needed.
Why
---
`valar-orchard` is upstream 0.12.0 + the same post-release fixes up to
zcash/orchard 6b12c77 that the previous `[patch.crates-io] orchard`
entry pointed at, plus the `pub` visibility additions that
valargroup/voting-circuits needs for the shielded-voting Halo 2
circuits (constants, spec, shared_primitives gadget).
Before this change, consumers that also depend on `voting-circuits`
(e.g. `zcash_voting`) ended up with two distinct Orchard crates in
their dep-graph: `orchard` (from this fork, via our patch) and
`valar-orchard` (pulled by `voting-circuits` from crates.io). Cargo
treats them as different crates regardless of content, which forced a
byte-round-trip `orchard_compat` bridge at the zcash_keys / pczt
boundary. Routing our fork through `valar-orchard` too collapses that
back to a single node and deletes the bridge.
The `[patch.crates-io] orchard = { git = "zcash/orchard", rev = "6b12c77…" }`
entry is obsolete after this change (no dep resolves crates.io
`orchard` anymore) and is removed.
Verified: `cargo check --workspace` clean. `valar-orchard v0.12.0`
appears in the build graph; no `orchard v0.12.x` or git `orchard`
entry.
Made-with: Cursor
The workspace now resolves `orchard` through the `valar-orchard` package
on crates.io via `package = "valar-orchard"`. Two CI jobs keyed on the
original crate name and needed to learn about the rename:
- supply-chain/config.toml: add a `safe-to-deploy` exemption for
`valar-orchard 0.12.0` (matches the criteria previously granted to
`orchard 0.12.0@git:b0bf2670e2…`) and drop the now-unreachable
orchard@git exemption: nothing in Cargo.lock resolves to it anymore.
`corez 0.1.1` is already exempted via the upstream `core2 → corez`
migration on this base, so no change needed there.
- .github/helpers/check-dep-graph.py: add a `PACKAGE_NAME_REMAP` table
that maps cargo-tree's package names back to the aliases used in the
README mermaid graph, and apply it while building `cargo_edges`.
Without this, every README edge mentioning `orchard` looks stale
because `cargo tree -f ' {p}'` reports the real package name
(`valar-orchard`), and every real edge mentioning `valar-orchard` is
silently dropped because it is not in `CRATES_IN_GRAPH`.
Verified locally:
- `cargo vet --locked` → `Vetting Succeeded (262 fully audited,
120 partially audited, 294 exempted)`
- `python3 .github/helpers/check-dep-graph.py` → exit 0, no output
Made-with: Cursor
…d-on-40 Resolve orchard via valar-orchard on crates.io (rebased onto PR #40)
…uery Switch `get_unspent_orchard_notes_at_historical_height` to filter on `transactions.mined_height` instead of `transactions.block`, and drop the now-redundant `IS NOT NULL` guards on both the receive and spend sides. `block` is set only after the containing compact block has been scanned, whereas `mined_height` is set as soon as the wallet learns of the mined height (including through paths such as transparent UTXO graph retrieval). For the notes this query can return the two are equivalent in practice (`nf` and `commitment_tree_position` already require a scan of the receiving block), but `mined_height` is the right column to filter on semantically, and matches the existing `mark_orchard_note_spent` and `get_spendable_note` queries. The `IS NOT NULL` guards were dead code: `NULL <= :height` is always false in SQLite, so `mined_height <= :height` already excludes transactions the wallet has not observed as mined. Addresses review feedback from @nuttycom on zcash#2284. Made-with: Cursor Co-Authored-By: Claude <noreply@anthropic.com>
…spent-notes query Replace `rn.recipient_key_scope IN (0, 1)` with an `IN` clause that interpolates `KeyScope::EXTERNAL.encode()` and `KeyScope::INTERNAL.encode()` via `format!`, matching the pattern used in `wallet/transparent.rs` and `wallet/init/migrations/transparent_gap_limit_handling.rs`. This keeps the SQL filter in sync with the `KeyScope` definition if its encoding ever changes. Addresses review feedback from @nuttycom on zcash#2284. Made-with: Cursor Co-Authored-By: Claude <noreply@anthropic.com>
…n for historical note getter Clarify in the rustdoc of both the `pub(crate)` helper in `wallet::orchard` and the public `WalletDb::get_unspent_orchard_notes_at_historical_height` wrapper that this function does not verify whether a Merkle witness can be constructed for each returned note at `height`. Witness construction is a separate concern intended to be handled by callers; as an example, a companion `WalletDb::generate_orchard_witnesses_at_historical_height` method returns an actionable error for any position the wallet cannot witness at the given height (wallet not synced through that height, checkpoint pruned, or position not in the wallet). The companion method is referenced by name only rather than via an intra-doc link because it is introduced in a separate change. Addresses review feedback from @nuttycom on zcash#2284. Made-with: Cursor Co-Authored-By: Claude <noreply@anthropic.com>
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.
Supersedes #36 (draft). Same feature content, rebased onto
95b000e8893bac2440b17b7ceccaff3df48fc220— the last upstreamzcash/maincommit before thesapling-crypto 0.7/orchard 0.13migration (badfae5a45). The zodl 3.4.0 release track is still on orchard 0.12 (viavalar-orchard), so this base keeps us aligned with upstream maintenance while avoiding the 0.13 jump.Commit structure
PR #36's single "governance wallet methods" commit has been split into the commits that are being upstreamed individually, matching zcash#2283 and zcash#2284 one-to-one. This PR is the sum of everything needed for zodl 3.4.0:
Signer::shielded_sighash+ doc fixes)shielded_sighashhalf of the original PCZT changes; thespend_auth_sighalf is already upstream via zcash#2281 (merged into 95b000e)MemoryShardStoreadd_checkpointin historical witness generationRetention::Checkpointalready registers; also feature-gatesMarking/ShardTreeimports behind#[cfg(feature = "orchard")]to silenceunused_importswarnings under--no-default-featuresgenerate_orchard_witnesses_at_historical_heightCorruptedData(String)with two new orchard-gatedSqliteClientErrorvariants (HistoricalFrontierInvalid(InsertionError),HistoricalWitnessUnavailable { position, height }); shard-read failures now flow throughSqliteClientError::CommitmentTreedirectlyWhen zcash#2283 and zcash#2284 merge upstream, this PR collapses to just the PCZT
shielded_sighashcommit. Once that too lands upstream (there is already a matching branchroman/pczt-0.5.x/shielded-sighash-pub), the zodl track can point at upstreamzcash/mainpre-orchard-0.13 with zero carry-forward patches.Summary
pczt::roles::signer::Signer::shielded_sighashgetter (plus two doc-comment brace fixes neartransparent_sighash).WalletDb::get_unspent_orchard_notes_at_historical_height(account, height)— returns all Orchard notes received at-or-beforeheightand unspent as of that height. Backward-looking, unlikeselect_unspent_noteswhich is forward-looking.WalletDb::generate_orchard_witnesses_at_historical_height(positions, frontier, height)— loads the wallet's Orchard shards and cap into an ephemeralMemoryShardStore, inserts the caller-provided frontier viaRetention::Checkpoint, and generates Merkle witnesses. Wallet DB is strictly read-only.orchard-gatedSqliteClientErrorvariants so callers can programmatically distinguish "bad frontier" from "wallet not synced through this height":HistoricalFrontierInvalid(shardtree::error::InsertionError)HistoricalWitnessUnavailable { position, height }Both new methods are governance-specific and intentionally NOT on the
WalletRead/WalletWritetraits.Why this base commit
95b000eis upstreamzcash/mainas of the 2026-04-23 release, immediately beforebadfae5a45 Migrate to sapling-crypto 0.7, orchard 0.13. The fork'svalargroup/librustzcashmainbranch has also been fast-forwarded to95b000eso the PR diff is clean (no upstream-sync noise).Test plan
cargo check -p zcash_client_sqlite --all-features --tests— OKcargo check -p zcash_client_sqlite --no-default-features— OK (verifies the#[cfg(feature = "orchard")]gating onMarking/ShardTreeimports)cargo check --workspace --all-features --tests— OKcargo test -p zcash_client_sqlite --all-features --lib historical_height:wallet::commitment_tree::tests::orchard::witnesses_at_historical_height— passedwallet::orchard::tests::get_unspent_orchard_notes_at_historical_height_boundary_heights— passedFollow-ups (out of scope)
zcash_voting/Cargo.tomllibrustzcash pins from1bc3a279e3c5(old tip) to this branch's tip (b1fd19fb8b).vote-sdkjoin flow + iOS signer end-to-end against the new base — main concern is interaction between the new upstreamwitness_stabilized/rewind_to_heightbehavior and historical-height witness generation.HistoricalFrontierInvalid/HistoricalWitnessUnavailablevariants throughvote-sdkerror handling so the join flow can surface a clear "sync further before voting" message to the user.