Skip to content

Governance wallet support for shielded voting (zodl 3.4.0, pre-orchard 0.13)#43

Open
p0mvn wants to merge 13 commits into
mainfrom
shielded-vote-for-zodl-3.4.0
Open

Governance wallet support for shielded voting (zodl 3.4.0, pre-orchard 0.13)#43
p0mvn wants to merge 13 commits into
mainfrom
shielded-vote-for-zodl-3.4.0

Conversation

@p0mvn
Copy link
Copy Markdown

@p0mvn p0mvn commented Apr 24, 2026

Summary

Adds governance wallet methods for shielded voting on top of upstream zcash/librustzcash, rebased onto 95b000e8893bac2440b17b7ceccaff3df48fc220 — the last upstream zcash/main commit before the sapling-crypto 0.7 / orchard 0.13 migration (badfae5a45). The zodl 3.4.0 release track is still on orchard 0.12, so this base keeps us aligned with upstream maintenance while avoiding the 0.13 jump.

The public API additions:

  • pczt::roles::signer::Signer::shielded_sighash() — public getter (plus two doc-comment brace fixes near transparent_sighash).
  • WalletDb::get_unspent_orchard_notes_at_historical_height(account, height) — returns every Orchard note received at or before height and unspent as of that height. Backward-looking, unlike select_unspent_notes which is forward-looking.
  • WalletDb::generate_orchard_witnesses_at_historical_height(positions, frontier, height) — loads the wallet's Orchard shards and cap into an ephemeral MemoryShardStore, inserts the caller-provided frontier via Retention::Checkpoint, and generates Merkle witnesses for each requested position. The wallet DB is strictly read-only.
  • Two new orchard-gated SqliteClientError variants so callers can programmatically distinguish "bad frontier" from "wallet not synced through this height":
    • HistoricalFrontierInvalid(shardtree::error::InsertionError)
    • HistoricalWitnessUnavailable { position, height }

Both new WalletDb methods are governance-specific and intentionally not on the WalletRead / WalletWrite traits.

Upstreaming status

Most of the functional changes in this PR are already in flight upstream and will disappear from this branch as they land. The mapping is one-to-one with the upstream commits:

Commits on this branch Upstream status
Expose PCZT getters needed for shielded voting (Signer::shielded_sighash + doc fixes) In progress — local branch roman/pczt-0.5.x/shielded-sighash-pub ready to open against upstream. The spend_auth_sig half has already merged as zcash#2281.
zcash_client_sqlite: Add method to query historical unspent Orchard notes + the three review-feedback follow-ups (mined_height, KeyScope::encode, witness-availability doc) In review at zcash#2284 — approved by @nuttycom, all four commits mirrored one-to-one here.
zcash_client_sqlite: Add historical height witness generation + the four review-feedback follow-ups (module-level imports, MemoryShardStore, redundant-checkpoint cleanup, actionable SqliteClientError variants) In review at zcash#2283 — all five commits mirrored one-to-one here.

Once zcash#2283 and zcash#2284 merge, this PR collapses to just the PCZT shielded_sighash commit. Once that too lands upstream, the zodl track can point at upstream zcash/main pre-orchard-0.13 with zero carry-forward patches.

Temporary dependency: valar-orchard

The workspace resolves orchard through the valar-orchard crate on crates.io (via cargo's package = rename trick). See the Resolve orchard via valar-orchard on crates.io and ci: unblock cargo-vet and readme-graph for valar-orchard rename commits.

valar-orchard is upstream orchard 0.12.0 + the same post-release fixes that our previous [patch.crates-io] entry pointed at (zcash/orchard rev 6b12c77), plus the pub visibility additions that valargroup/voting-circuits needs for the shielded-voting Halo 2 circuits (constants, spec, shared_primitives gadget).

This is a temporary arrangement. Once orchard 0.13.0 ships upstream with the required pub visibility upstreamed, we will unfork and drop valar-orchard in favor of the canonical orchard crate.

Before routing our fork through valar-orchard, consumers that also depend on voting-circuits (e.g. zcash_voting) ended up with two distinct Orchard crates in their dep graph (orchard via our patch + valar-orchard from crates.io), which Cargo treats as different crates regardless of content. That forced a byte-round-trip orchard_compat bridge at the zcash_keys / pczt boundary. Collapsing both to valar-orchard deletes the bridge.

Why this base commit

95b000e is upstream zcash/main as of 2026-04-23, immediately before badfae5a45 Migrate to sapling-crypto 0.7, orchard 0.13. The fork's valargroup/librustzcash main branch has also been fast-forwarded to 95b000e so the PR diff is clean (no upstream-sync noise).

Test plan

  • cargo check -p zcash_client_sqlite --all-features --tests — OK
  • cargo check -p zcash_client_sqlite --no-default-features — OK (verifies the #[cfg(feature = "orchard")] gating on Marking / ShardTree imports)
  • cargo check --workspace --all-features --tests — OK
  • cargo clippy -p zcash_client_sqlite --all-features --all-targets -- -D warnings — OK
  • cargo test -p zcash_client_sqlite --all-features --lib historical_height:
    • wallet::commitment_tree::tests::orchard::witnesses_at_historical_height — passed
    • wallet::orchard::tests::get_unspent_orchard_notes_at_historical_height_boundary_heights — passed
  • cargo vet --lockedVetting Succeeded
  • python3 .github/helpers/check-dep-graph.py — exit 0, no output

Follow-ups (out of scope)

  • Bump zcash_voting/Cargo.toml librustzcash pins from 1bc3a279e3c5 (old tip) to this branch's tip.
  • Validate the vote-sdk join flow + iOS signer end-to-end against the new base — main concern is interaction between the new upstream witness_stabilized / rewind_to_height behavior and historical-height witness generation.
  • Thread the new HistoricalFrontierInvalid / HistoricalWitnessUnavailable variants through vote-sdk error handling so the join flow can surface a clear "sync further before voting" message to the user.
  • Unfork valar-orchard once orchard 0.13.0 lands upstream with the required pub visibility changes.

Made with Cursor

greg0x and others added 13 commits April 24, 2026 14:02
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.
…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>
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants