zcash_client_sqlite: Add historical height witness generation by p0mvn · Pull Request #2283 · zcash/librustzcash

p0mvn · 2026-04-11T18:55:08Z

Summary

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.

See analysis of the implementation choice here

Branching note

This PR targets maint/zcash_client_sqlite-0.19.x as a
SemVer-compatible addition (new public method, no breaking changes).

How We Use It

Token Holder Voting.

Generate witnesses at a snapshot height to delegate voting authority.

See full summary of the integration: https://hackmd.io/@TJGUVkqNQYieiMqJQQBDeA/rJ0mU1v3Wl

Changes

zcash_client_sqlite/src/wallet/commitment_tree.rs — new
pub(crate) fn generate_orchard_witnesses_at_historical_height plus
private helpers create_orchard_tree_tables / copy_orchard_tree_data
that clone the shard-tree schema and data into an ephemeral in-memory DB.
DDL reuses the canonical TABLE_ORCHARD_TREE_* constants from db.rs.
zcash_client_sqlite/src/lib.rs — public
WalletDb::generate_orchard_witnesses_at_historical_height method
that delegates to the above.
zcash_client_sqlite/CHANGELOG.md — added entry under
[Unreleased].
Unit test (witnesses_at_historical_height) verifying the generated
witness root matches the expected frontier root.

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

@nuttycom

…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>

nuttycom

A couple of the error types now seem incorrect after switching to use the in-memory shard store.

nuttycom · 2026-04-24T17:16:08Z

+        },
+    )
+    .map_err(|e| {
+        SqliteClientError::CorruptedData(format!("failed to insert frontier nodes: {}", e))


This now seems like the wrong error; indeed, is SqliteClientError the correct error type for this function at all?

All other methods in the crate returned SqliteClientError error type, so this was chosen for consistency with the pattern.

I have improved the errors so that the caller can respond to them. However, I think that it would be best to keep SqliteClientError for consistency with the crate patterns.

Concretely, c68d769 adds two orchard-gated variants that callers can match on:

SqliteClientError::HistoricalFrontierInvalid(InsertionError) - the caller supplied a bad frontier.

SqliteClientError::HistoricalWitnessUnavailable { position, height } - the wallet isn't synced through height, the checkpoint was pruned, or position isn't in the wallet.
Shard-read failures continue to surface as SqliteClientError::CommitmentTree.

If we want this changed, I think it would make sense to file a tech debt issue that would refactor the errors in the crate as a whole.

Please let me know if there is an alternative recommendation.

@nuttycom

…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>

@nuttycom

…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>

@nuttycom

…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>

p0mvn · 2026-04-24T18:42:57Z

@nuttycom Thanks for the careful review. I pushed two follow-up commits addressing all four inline comments:

7043dd1 removes the redundant add_checkpoint (L1264).
c68d769 replaces the misleading CorruptedData stringification with two new orchard-gated SqliteClientError variants so callers can respond programmatically (L1259, L1277, L1284):
- HistoricalFrontierInvalid(InsertionError) - caller-supplied frontier inconsistent with the wallet's shards.
- HistoricalWitnessUnavailable { position, height } - no witness available (wallet likely not synced through height, checkpoint pruned, or position not in wallet).

I kept the errors as SqliteClientError variants rather than introducing a bespoke error type, for consistency with the rest of the crate. Please see my reply on L1259 for the reasoning.

Happy to split them out into a dedicated error type if you'd prefer. This seemed the more conservative of the two options.

Ready for another look when you have a moment.

@nuttycom

…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>

@nuttycom

…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>

@nuttycom

…kpoint pruning safety `generate_orchard_witnesses_at_historical_height` inserts the caller- supplied frontier as `Retention::Checkpoint { id: height, .. }`, which internally calls `add_checkpoint` and then `prune_excess_checkpoints`. A reviewer raised the concern that, if the in-memory store carried pre-existing checkpoints, the freshly added one could be pruned away immediately and `witness_at_checkpoint_id` would then return `None`, surfacing as `HistoricalWitnessUnavailable`. Today this cannot happen: `MemoryShardStore::empty()` starts with zero checkpoints, `put_shard`/`put_cap` do not touch the checkpoint map (the `CHECKPOINT` retention flags inside shard leaves are independent of `checkpoint_count()`), and `max_checkpoints` is `1`, so the inserted checkpoint survives `1 > 1 == false`. The test added here pins that invariant in CI: - Build a wallet `ShardTree` with `WALLET_MAX_CHECKPOINTS = 100`, append a `Marked` note at position 0 plus an `Ephemeral` filler, mirror them into a parallel `Frontier` to capture the ground-truth state at `historical_height = 10`, and checkpoint. - Append 249 more `Ephemeral` blocks (each with its own checkpoint) to force the wallet pruner to evict the historical checkpoint from the SQLite checkpoint table. - Assert `min_checkpoint_id > historical_height` as a precondition, so the test fails loudly if a future tweak (shrinking `TOTAL_BLOCKS`, growing `WALLET_MAX_CHECKPOINTS`) accidentally leaves the historical checkpoint alive in the DB. - Call `generate_orchard_witnesses_at_historical_height` with the captured frontier and verify the reconstructed witness reproduces `frontier_tree.root()`. This test will start failing the moment someone (a) pre-loads wallet checkpoints into the in-memory store, (b) adds an extra `add_checkpoint` call alongside the frontier insertion, or (c) drops `max_checkpoints` below `1`. Also annotate the production code with a "Pruning-safety invariant" comment block explaining the `1 > 1 == false` reasoning and naming this regression test, so a future maintainer who modifies the in-memory checkpoint setup is pointed at the test that will catch the mistake. No public API change; no behavior change. Addresses review feedback from @nuttycom on zcash#2283. Made-with: Cursor Co-Authored-By: Claude <noreply@anthropic.com>

nuttycom

utACK 5e08af9

nuttycom · 2026-04-24T21:28:06Z

@p0mvn the merge of your other PR caused conflicts here, can you please resolve them?

p0mvn · 2026-04-24T21:31:26Z

Yes, on it. Thanks for the reviews

…tness-historical Resolves conflicts in zcash_client_sqlite/CHANGELOG.md and zcash_client_sqlite/src/lib.rs, both caused by get_unspent_orchard_notes_at_historical_height (added upstream in zcash#2284) and generate_orchard_witnesses_at_historical_height (added in this branch) landing side-by-side in the same `## [Unreleased] ### Added` block and the same `#[cfg(feature = "orchard")] impl WalletDb<...>` block. Both methods are kept; the upstream method is listed/declared first to preserve the upstream ordering, with the witness-generation API following it. The cross-reference in get_unspent_orchard_notes_at_historical_height to the companion generate_orchard_witnesses_at_historical_height now points at a method that actually exists on this branch. Co-Authored-By: Claude <noreply@anthropic.com> Made-with: Cursor

p0mvn · 2026-04-24T21:45:01Z

@nuttycom conflicts have been resolved. Thank you again

nuttycom

re-utACK 2a1ee68; once CI passes I'll push this directly to avoid an extraneous merge commit.

p0mvn · 2026-04-24T22:20:54Z

re-utACK 2a1ee68; once CI passes I'll push this directly to avoid an extraneous merge commit.

Ah, my bad on the merge commit instead of rebase.

Tracking the rules now, but I am going to avoid resetting/rebasing myself to keep CI running.

p0mvn marked this pull request as ready for review April 11, 2026 18:57

p0mvn mentioned this pull request Apr 11, 2026

zcash_client_sqlite: Add WalletDb::get_unspent_orchard_notes_at_historical_height #2284

Merged

p0mvn added 2 commits April 23, 2026 08:54

nuttycom force-pushed the roman/zcash_client_sqlite-0.19.x/generate-witness-historical branch from 7b5d87d to a40b437 Compare April 23, 2026 14:54

nuttycom changed the base branch from maint/zcash_client_sqlite-0.19.x to main April 23, 2026 14:55

nuttycom reviewed Apr 23, 2026

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/commitment_tree.rs Outdated

p0mvn requested a review from nuttycom April 23, 2026 18:57

p0mvn mentioned this pull request Apr 23, 2026

zcash_client_sqlite: Use MemoryShardStore for historical witness generation valargroup/librustzcash#38

Merged

3 tasks

greg0x mentioned this pull request Apr 24, 2026

Add support for shielded voting #2212

Closed

nuttycom reviewed Apr 24, 2026

View reviewed changes

p0mvn mentioned this pull request Apr 24, 2026

Governance wallet support for shielded voting (rebased onto 95b000e, pre-orchard 0.13) valargroup/librustzcash#40

Closed

p0mvn and others added 2 commits April 24, 2026 15:18

p0mvn requested a review from nuttycom April 24, 2026 18:58

This was referenced Apr 24, 2026

Governance wallet support for shielded voting (zodl 3.4.0, pre-orchard 0.13) valargroup/librustzcash#42

Closed

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

Open

nuttycom reviewed Apr 24, 2026

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/commitment_tree.rs

p0mvn requested a review from nuttycom April 24, 2026 21:02

nuttycom previously approved these changes Apr 24, 2026

View reviewed changes

nuttycom enabled auto-merge April 24, 2026 21:25

auto-merge was automatically disabled April 24, 2026 21:39
Head branch was pushed to by a user without write access

p0mvn dismissed nuttycom’s stale review via 2a1ee68 April 24, 2026 21:39

p0mvn requested a review from nuttycom April 24, 2026 21:43

nuttycom approved these changes Apr 24, 2026

View reviewed changes

nuttycom merged commit 6cd8262 into zcash:main Apr 24, 2026
48 checks passed

nuttycom deleted the roman/zcash_client_sqlite-0.19.x/generate-witness-historical branch April 24, 2026 22:41

This was referenced Apr 25, 2026

Migrate to orchard 0.13 + librustzcash zcash/main valargroup/zcash_voting#22

Merged

Migrate libzcashlc to orchard 0.13 + librustzcash zcash/main valargroup/zcash-swift-wallet-sdk#25

Closed

Conversation

p0mvn commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Branching note

How We Use It

Changes

Uh oh!

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nuttycom Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

p0mvn Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

p0mvn commented Apr 24, 2026

Uh oh!

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

nuttycom commented Apr 24, 2026

Uh oh!

p0mvn commented Apr 24, 2026

Uh oh!

p0mvn commented Apr 24, 2026

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

p0mvn commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

p0mvn commented Apr 11, 2026 •

edited

Loading

p0mvn Apr 24, 2026 •

edited

Loading

p0mvn commented Apr 24, 2026 •

edited

Loading