Skip to content

fix(drive,drive-abci): retire SHIELDED_MOST_RECENT_ANCHOR_KEY; derive most-recent from [8] and never empty it#3605

Merged
QuantumExplorer merged 1 commit intov3.1-devfrom
platform-wallet/shielded-prune-keep-recent
May 6, 2026
Merged

fix(drive,drive-abci): retire SHIELDED_MOST_RECENT_ANCHOR_KEY; derive most-recent from [8] and never empty it#3605
QuantumExplorer merged 1 commit intov3.1-devfrom
platform-wallet/shielded-prune-keep-recent

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer commented May 6, 2026

Issue being fixed or feature implemented

The shielded credit pool's anchor lookup table ([..., \"s\", [6]])
could be silently emptied by prune while the pool was still live,
freezing every shielded spend with InvalidAnchorError until a
new shield op refreshed state. Verified via grpcurl on a stuck
regtest:

$ grpcurl ... getShieldedPoolState   → 60_000_000_000 credits (pool live)
$ grpcurl ... getMostRecentShieldedAnchor → fb8a9c94…  (live anchor reported)
$ grpcurl ... getShieldedAnchors        → {}            (lookup table empty)
$ broadcast spend → InvalidAnchorError { anchor: fb8a9c94… }

Reproducer: shield once, mine ≥ shielded_anchor_retention_blocks
(1000) blocks of inactivity, attempt any spend.

What was done?

Two structural changes that together remove the failure mode.

1. Retire SHIELDED_MOST_RECENT_ANCHOR_KEY = 7

The pool kept the live anchor in two places:

  • [..., \"s\", [6]]SHIELDED_ANCHORS_IN_POOL_KEY, the
    lookup table that validate_anchor_exists reads. Written and
    pruned together with [8].
  • [..., \"s\", [7]]SHIELDED_MOST_RECENT_ANCHOR_KEY, a
    redundant single-slot mirror of the latest anchor. Written by
    record_shielded_pool_anchor_if_changed, never touched by
    prune
    .

record_* was guarded by anchor changed since last record. Once
the pool sat idle for ≥ 1000 blocks, the anchor stopped changing
record_* no-op'd every block → [7] retained its value but
prune emptied [6]+[8] by height range, with no awareness of
which anchor was live. End state: [7] says X, [6] is empty,
every spend bundle that anchors at X (i.e. every valid one)
hits InvalidAnchorError.

The slot is genuinely redundant: the most-recent anchor is just
the value of the highest-block-height entry in [8]. Removing
it eliminates the duplicate state that made the desync possible.

  • paths.rs: remove the constant; leave a comment block in its
    place documenting why key 7 is retired.
  • initialization/v3/mod.rs: drop the [7] init insert.
  • record_anchor_if_changed/v0: read latest from [8] via a
    limit 1 reverse query; insert into [6]+[8] only when
    changed. Drop the stale != [0; 32] guard — it was a
    defense against [7]'s uninitialised state, not a real
    "empty pool" gate. The Sinsemilla empty root is a non-zero
    hash and recording it on first block-end after init is
    harmless (no notes against an empty pool to spend).
  • query/shielded/most_recent_anchor/v0 + the matching
    verify/shielded/verify_most_recent_shielded_anchor/v0
    replay the same limit 1 reverse PathQuery so proofs and
    verifier line up byte-for-byte.

A new Drive::read_latest_recorded_shielded_anchor_v0 helper
encapsulates the reverse query so record / non-proven query
/ strategy tests all share the canonical PathQuery (defined
once in shielded_latest_recorded_anchor_path_query).

2. Prune never empties [8]

prune_shielded_pool_anchors_v0 now preserves the
highest-block-height entry whenever every entry in [8] is
below the cutoff (i.e. the live anchor itself is older than
retention_blocks). The retention invariant relaxes to keep
at most one stale anchor while the pool sits idle past the
retention window
— bounded, harmless, and the difference
between "validator can find the live anchor" and "every spend
fails." Cheap: probes for any entry ≥ cutoff with limit: 1 so
the special case is only entered when the live anchor is
already old.

How Has This Been Tested?

  • 3043 drive lib tests pass (prune + record + verify
    modules):
    • record_on_empty_pool_records_the_sinsemilla_empty_root
    • record_after_note_insert_stores_anchor
    • record_idempotent_when_anchor_unchanged
    • read_latest_returns_highest_height_entry
    • prune_keeps_highest_when_all_below_cutoff ← the desync
      regression test
    • prune_keeps_single_old_entry
    • prune_removes_all_below_cutoff_when_a_recent_anchor_exists
    • prune_preserves_all_at_or_above_cutoff
    • should_prove_and_verify_most_recent_shielded_anchor_present
    • should_prove_and_verify_most_recent_shielded_anchor_absent
    • highest_block_height_wins
  • 54 drive-abci shielded query tests pass.
  • 22 drive verify-shielded tests pass.
  • 15 drive-abci shielded execution tests pass.
  • cargo clippy -p drive -p drive-abci --all-targets clean.
  • cargo fmt --all clean.

Breaking Changes

State-shape change to the shielded credit pool:

  • Key 7 is no longer initialised on fresh pools, no longer
    written by record_*, and no longer read anywhere. Existing
    state that has a [7] entry from before this PR is left as
    a dead fossil (no migration needed).
  • getMostRecentShieldedAnchor proofs change shape ([8]
    reverse vs. [7] single-key). SDK clients that verify
    proofs see the new shape via the matching change to
    Drive::verify_most_recent_shielded_anchor_v0 — no
    consumer-visible API change at the gRPC layer (response
    message is unchanged).
  • Pre-mainnet: v3.1-dev hasn't shipped, so the v0 method
    bodies are amended in place rather than gated behind a new
    record_shielded_pool_anchor / prune_shielded_pool_anchors
    feature version.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

Related

Surfaced while testing PR #3603 (shielded send wiring) on a regtest that had been idle for ≥ retention_blocks.
That PR is blocked on this fix landing — see its updated description for the diagnostic trail.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added height-indexed anchor query capability for shielded pool operations.
  • Bug Fixes

    • Improved anchor pruning logic to preserve at least one anchor entry.
    • Enhanced anchor verification to correctly handle height-based anchor storage.
  • Refactor

    • Updated shielded anchor storage mechanism from single-key to path-query-based approach for improved consistency and efficiency.

… most-recent from [8] and never empty it

The shielded pool kept the live anchor in two places:

  * `[..., "s", [6]]` (`SHIELDED_ANCHORS_IN_POOL_KEY`) — the lookup
    table validate_anchor_exists reads, written and pruned together
    with `[8]`.
  * `[..., "s", [7]]` (`SHIELDED_MOST_RECENT_ANCHOR_KEY`) — a
    redundant single-slot mirror of the latest anchor, written by
    record_shielded_pool_anchor_if_changed and never touched by
    prune.

After ≥ retention_blocks (1000) of shielded inactivity, prune
removed the only entry from `[6]`/`[8]`, but `[7]` retained its
value because record was guarded by "anchor changed since last
record" and the anchor hadn't changed for 1000 blocks. The
state ended up with:

  * getMostRecentShieldedAnchor → live anchor (from `[7]`)
  * getShieldedAnchors → empty (from `[6]`)
  * validate_anchor_exists → false → every spend rejected with
    InvalidAnchorError until a new shield op refreshed `[6]`.

Two changes that together remove the failure mode:

1. Drop `[7]` entirely. The most-recent anchor is now derived
   from a `limit 1` reverse query against `[8]` — single source
   of truth, can't desync.
   - paths.rs: remove the constant; document the retirement.
   - initialization/v3: drop the `[7]` init insert.
   - record_anchor_if_changed/v0: read latest from `[8]`,
     compare, insert `[6]`+`[8]` when changed. Drop the
     stale `!= [0; 32]` guard (which was a defense against
     `[7]`'s uninitialised state, not a real "empty pool"
     gate — the Sinsemilla empty root is non-zero).
   - query/shielded/most_recent_anchor/v0 + verify
     equivalent: replay the same `limit 1` reverse path query
     so proofs match.

2. prune_shielded_pool_anchors_v0 now preserves the highest
   `[8]` entry whenever pruning would otherwise empty the
   index (i.e. every entry is below cutoff). The retention
   invariant relaxes to "keep at most one stale anchor when
   the pool sits idle past the retention window" — bounded
   and acceptable, vs. the old behavior of "empty the lookup
   table and freeze every spend." Probes for any entry ≥
   cutoff with `limit 1` to skip the special case when the
   live anchor is already recent.

A new `Drive::read_latest_recorded_shielded_anchor_v0` helper
encapsulates the reverse query so record/query/verify all share
the canonical `PathQuery` (in `shielded_latest_recorded_anchor_path_query`).
The strategy test that previously asserted on `[7]` now reads
the helper instead.

Five new unit tests cover the regression:
- `record_on_empty_pool_records_the_sinsemilla_empty_root`
- `record_idempotent_when_anchor_unchanged`
- `read_latest_returns_highest_height_entry`
- `prune_keeps_highest_when_all_below_cutoff` (the desync case)
- `prune_keeps_single_old_entry`

3043 drive lib tests + 22 verify-shielded + 54 drive-abci
shielded query + 15 shielded-common tests pass; clippy clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v3.1.0 milestone May 6, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

Refactors shielded anchor storage from a dedicated "most recent" key-value slot to a height-indexed log model. Introduces shielded_latest_recorded_anchor_path_query to derive the most recent anchor from anchors stored by height. Updates initialization, record, prune, verify, and query paths accordingly.

Changes

Shielded Anchor Storage Refactor

Layer / File(s) Summary
Path Query Definitions
packages/rs-drive/src/drive/shielded/paths.rs
New shielded_latest_recorded_anchor_path_query() function constructs a PathQuery with limit-1 reverse query over anchors-by-height path. Removes SHIELDED_MOST_RECENT_ANCHOR_KEY constant; documents that most-recent anchor now derives from height-based log.
Storage Initialization
packages/rs-drive/src/drive/initialization/v3/mod.rs
Adds anchors-by-height index initialization under shielded credit pool; removes previous most-recent anchor slot initialization.
Record Anchor Logic
packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs
Updates record_shielded_pool_anchor_if_changed_v0 to read latest recorded anchor via canonical log query, compare against current anchor, and write to both anchors and anchors-by-height trees. Removes writes to dedicated most-recent-anchor key. Extends tests to validate new canonical-log behavior.
Prune Anchor Logic
packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs
Refactors prune_shielded_pool_anchors_v0 to use by-height path helpers; builds separate queries for below-cutoff and above-cutoff entries; adds logic to preserve at least one anchor. Includes new tests for pruning edge cases (below-cutoff removal, highest-entry preservation).
Verification Logic
packages/rs-drive/src/verify/shielded/verify_most_recent_shielded_anchor/v0/mod.rs
Updates verify_most_recent_shielded_anchor_v0 to use shielded_latest_recorded_anchor_path_query and decode 32-byte anchors from proved values. Replaces zero-anchor normalization with height-based tie-breaking. Adds tests for seed-by-height and highest-height-wins scenarios.
Query Handler
packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs
Updates query_most_recent_shielded_anchor_v0 to use path-query-based approach via shielded_latest_recorded_anchor_path_query; prove branch returns Proof via grove_get_proved_path_query; non-prove branch uses grove_get_raw_path_query and extracts anchor or defaults to zero.
Test & Integration
packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs, packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs, packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs, packages/rs-drive/src/verify/shielded/verify_most_recent_shielded_anchor/v0/mod.rs
Updates test imports to use new anchor-path constants and read_latest_recorded_shielded_anchor_v0 API; validates anchor retrieval, membership checks, and pruning behavior under the new by-height model.

Sequence Diagram

sequenceDiagram
    participant Client
    participant QueryHandler as Query Handler
    participant Storage as Storage<br/>(anchors_path,<br/>anchors_by_height_path)
    participant PathQuery as PathQuery<br/>(limit-1 reverse)
    participant Verifier as Verifier

    Note over Client,Verifier: New Flow: Height-Indexed Anchor Log

    Client->>QueryHandler: query_most_recent_shielded_anchor_v0()
    QueryHandler->>QueryHandler: Construct shielded_latest_recorded_anchor_path_query()
    QueryHandler->>PathQuery: Create limit-1 reverse query over anchors_by_height
    QueryHandler->>Storage: grove_get_proved_path_query(path_query)
    Storage-->>QueryHandler: proved_key_values (highest height anchor)
    QueryHandler->>QueryHandler: Extract 32-byte anchor from result
    QueryHandler-->>Client: Return Proof(anchor) or Proof(zero)

    Note over Client,Verifier: Anchor Recording

    Client->>Storage: record_anchor_if_changed_v0(current_anchor)
    Storage->>Storage: Read latest_recorded via path_query
    alt Anchor changed
        Storage->>Storage: Write current_anchor → anchors_path
        Storage->>Storage: Write block_height → anchors_by_height_path
    end
    Storage-->>Client: Anchor recorded at height

    Note over Client,Verifier: Verification

    Client->>Verifier: verify_most_recent_shielded_anchor_v0(proof)
    Verifier->>PathQuery: Use shielded_latest_recorded_anchor_path_query
    Verifier->>Verifier: Match proved_key_values, decode 32-byte anchor
    Verifier->>Verifier: Select highest height if multiple matches
    Verifier-->>Client: Verified anchor or error
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes


🐰 The anchors now bloom by height,
No single slot, but indexed light,
Reverse queries find the crown,
The newest anchor in town,
A shielded pool of organized might! ✨🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main structural change: retiring SHIELDED_MOST_RECENT_ANCHOR_KEY and deriving most-recent from the anchors-by-height index, which matches the PR's primary objectives and all file changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch platform-wallet/shielded-prune-keep-recent

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented May 6, 2026

✅ Review complete (commit 6dfa0fb)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

❌ Patch coverage is 95.03106% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.29%. Comparing base (4ca63a1) to head (6dfa0fb).
⚠️ Report is 2 commits behind head on v3.1-dev.

Files with missing lines Patch % Lines
.../drive/shielded/record_anchor_if_changed/v0/mod.rs 93.50% 10 Missing ⚠️
...elded/verify_most_recent_shielded_anchor/v0/mod.rs 90.24% 4 Missing ⚠️
...ci/src/query/shielded/most_recent_anchor/v0/mod.rs 87.50% 1 Missing ⚠️
...s-drive/src/drive/shielded/prune_anchors/v0/mod.rs 99.05% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           v3.1-dev    #3605    +/-   ##
==========================================
  Coverage     88.29%   88.29%            
==========================================
  Files          2479     2479            
  Lines        301541   301660   +119     
==========================================
+ Hits         266231   266350   +119     
  Misses        35310    35310            
Components Coverage Δ
dpp 87.95% <ø> (ø)
drive 87.37% <95.22%> (+0.01%) ⬆️
drive-abci 90.25% <87.50%> (-0.01%) ⬇️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.17% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 55.66% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member Author

@QuantumExplorer QuantumExplorer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self Reviewed

@QuantumExplorer QuantumExplorer merged commit 7b23bc7 into v3.1-dev May 6, 2026
22 checks passed
@QuantumExplorer QuantumExplorer deleted the platform-wallet/shielded-prune-keep-recent branch May 6, 2026 18:08
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Solid fix to the SHIELDED_MOST_RECENT_ANCHOR_KEY desync. The shared shielded_latest_recorded_anchor_path_query helper makes record/query/verify byte-aligned, and prune now correctly preserves the highest entry. One blocker: add_estimation_costs_for_shielded_pool_operations in estimated_costs.rs still describes the pre-PR pool shape (9 elements, 2 items) and was not updated for the retired [7] slot. Several quality/doc nits worth folding in before merge.

Reviewed commit: 6dfa0fb

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 3 nitpick(s)

7 additional findings

🔴 blocking: Shielded fee estimation still models the retired key-7 slot

packages/rs-drive/src/drive/shielded/estimated_costs.rs (lines 78-106)

This PR removed the SHIELDED_MOST_RECENT_ANCHOR_KEY = 7 Item([u8;32]) slot from the pool, but add_estimation_costs_for_shielded_pool_operations still registers the pool with the pre-PR layout: comment says 9 elements total (7 subtrees + 2 items), EstimatedLevel(4, false), items_size: Some((1, 32, None, 2)), and the inline comment lists most recent anchor (Item) as the second item. After this PR the pool actually contains 7 subtrees + 1 item (the SHIELDED_TOTAL_BALANCE_KEY SumItem) — ceil(log2(8)) = 3, and only one item slot.

This metadata feeds ShieldedPoolOperationType::into_low_level_drive_operations() whenever shielded ops are costed in estimation mode (apply=false), so every shielded note/nullifier/balance fee estimate continues to reflect the old shape. The state-shape change is the whole point of the PR; the cost estimator needs to be updated in lockstep, otherwise stateless fee estimation is structurally lying about pool size and item count.

💡 Suggested change
        // Shielded credit pool: [AddressBalances, "s"]
        // SumTree containing: notes (CommitmentTree), permanent nullifiers (ProvableCountTree),
        // total balance (SumItem), anchors (NormalTree), anchors-by-height (NormalTree),
        // recent nullifiers (CountSumTree), compacted nullifiers (NormalTree),
        // expiration time (NormalTree)
        // 8 elements total (7 subtrees + 1 item) → balanced Merk depth = ceil(log2(8)) = 3
        estimated_costs_only_with_layer_info.insert(
            KeyInfoPath::from_known_path(shielded_credit_pool_path()),
            EstimatedLayerInformation {
                tree_type: TreeType::SumTree,
                estimated_layer_count: EstimatedLevel(3, false),
                estimated_layer_sizes: Mix {
                    subtrees_size: Some((
                        1,
                        SomeSumTrees {
                            sum_trees_weight: 0,
                            big_sum_trees_weight: 0,
                            count_trees_weight: 1, // permanent nullifiers (ProvableCountTree)
                            count_sum_trees_weight: 1, // recent nullifiers (CountSumTree)
                            non_sum_trees_weight: 5, // notes (CommitmentTree), anchors, anchors-by-height, compacted nullifiers, expiration time
                        },
                        None,
                        7, // 7 subtrees: notes, permanent nullifiers, anchors, anchors-by-height, recent nullifiers, compacted nullifiers, expiration time
                    )),
                    items_size: Some((1, 32, None, 1)), // 1 item: total balance (SumItem)
                    references_size: None,
                },
            },
        );
🟡 suggestion: Use `.pop()` on the already-sorted result instead of recomputing max_key

packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs (lines 95-110)

entries_below comes from grove_get_raw_path_query over a Query::new() (default left_to_right = true) with RangeTo, so GroveDB returns the entries in ascending key order (big-endian u64 → numeric ordering). The current code clones every key into a new Vec<u8> just to find the maximum, then walks the vector again to filter that key out — both O(N) extra work plus an allocation per entry on a path that runs every block.

Since the highest-block-height entry is always the last element, entries_below.pop() (and dropping the popped value) accomplishes the same thing and makes the intent obvious.

💡 Suggested change
        let to_delete: Vec<(Vec<u8>, Element)> = if any_above_cutoff {
            entries_below
        } else {
            // Range-query results come back in ascending key order;
            // the live anchor is the last element. Drop it; delete
            // the rest.
            let mut entries_below = entries_below;
            entries_below.pop();
            entries_below
        };
🟡 suggestion: Asymmetric empty-index handling between proven and non-proven branches

packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs (lines 61-78)

When the anchors-by-height index is empty:

  • The non-proven branch returns Anchor(vec![0u8; 32]) and relies on a downstream response decoder to translate the all-zeros sentinel into None.
  • The proven branch returns Proof(...); the SDK then runs verify_most_recent_shielded_anchor_v0, which (in this PR) returns Ok(None) for the genuinely-empty case — no zero-sentinel translation involved.

The two paths only stay aligned if the SDK's response decoder convention is preserved exactly. Now that the storage shape uses real absence (rather than a pre-initialised Item([0;32]) slot), the cleanest fix is to extend the gRPC response with an explicit "absent" variant alongside Anchor/Proof, or to make Anchor an Option. At minimum, add a comment cross-referencing the SDK decoder so the coupling isn't invisible to future maintainers.

🟡 suggestion: `read_latest_recorded_shielded_anchor_v0` is `pub` purely for test convenience; doc cites a caller that doesn't use it

packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs (lines 126-134)

This helper is exposed as pub. The doc comment justifies that visibility with two callers: "drive-abci's strategy tests + the getMostRecentShieldedAnchor non-proven query path." The first is real; the second is not — query_most_recent_shielded_anchor_v0 calls grove_get_raw_path_query directly on shielded_latest_recorded_anchor_path_query() and never goes through this helper (it returns a vec![0u8; 32] sentinel; this helper returns Option<[u8;32]>).

So the only real reason this is pub rather than pub(in crate::drive) (matching record_shielded_pool_anchor_if_changed_v0) is to let the drive-abci strategy test reach across crates. Exposing internal storage helpers on the public Drive API surface for test convenience forces forward-compat guarantees on something that has no business being part of the SDK contract. Options: (a) #[doc(hidden)] + test-only documentation, (b) gate behind a testing-helpers feature consumed by drive-abci's [dev-dependencies], or (c) keep pub(in crate::drive) and have the strategy test seed/read [8] directly. At minimum, fix the doc comment so it doesn't claim a caller that doesn't exist.

💬 nitpick: Dispatcher docstring still describes the pre-PR contract

packages/rs-drive/src/drive/shielded/record_anchor_if_changed/mod.rs (lines 10-17)

The public dispatcher's docstring still says: "compares it to the most recent stored anchor, and if different (and non-zero) writes entries to the anchors tree, anchors-by-height tree, and updates the most recent anchor item." After this PR there is no [7] slot to update, and the != [0; 32] guard is gone — the new v0 records the Sinsemilla empty root from the very first block-end. The v0 file's docstring was updated; this caller-facing one was missed, so rust-doc on Drive::record_shielded_pool_anchor_if_changed shows the obsolete contract.

💡 Suggested change
    /// Records the current shielded pool anchor if the commitment tree changed
    /// this block.
    ///
    /// Reads the current Sinsemilla anchor from the CommitmentTree and compares
    /// it to the latest entry in the anchors-by-height index (`[..., "s", [8]]`,
    /// derived via a `limit 1` reverse query). If different, writes the new
    /// anchor to both the anchors tree (`[..., [6]]`) and the anchors-by-height
    /// index (`[..., [8]]`). There is no separate "most recent anchor" slot —
    /// the by-height index is the canonical source.
    ///
    /// # Parameters
    /// - `block_height`: The current block height
    /// - `transaction`: The GroveDB transaction
    /// - `platform_version`: The platform version for dispatch
💬 nitpick: Comment points readers at `Drive::query_most_recent_shielded_anchor`, but that function lives on `Platform`

packages/rs-drive/src/drive/shielded/paths.rs (lines 23-29)

The retire-key-7 comment block points readers at Drive::query_most_recent_shielded_anchor. There is no such method on Drive. query_most_recent_shielded_anchor_v0 is implemented on Platform<C> in packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs. The Drive-side equivalent is read_latest_recorded_shielded_anchor_v0 (or the canonical shielded_latest_recorded_anchor_path_query defined just below). Minor, but the wrong receiver makes the pointer hard to follow.

💡 Suggested change
// Key 7 was previously `SHIELDED_MOST_RECENT_ANCHOR_KEY`, a redundant
// `Item([u8;32])` slot mirroring the latest entry in
// `SHIELDED_ANCHORS_BY_HEIGHT_KEY`. It was removed because the duplicated
// state could (and did) drift out of sync with the anchors tree under prune,
// leaving the validator's lookup table empty while the pool was still live.
// The most-recent anchor is now derived from `[8]` via a `limit 1` reverse
// query — see `shielded_latest_recorded_anchor_path_query` below and its
// three call sites: `Drive::read_latest_recorded_shielded_anchor_v0`,
// `Platform::query_most_recent_shielded_anchor_v0`, and
// `Drive::verify_most_recent_shielded_anchor_v0`.
💬 nitpick: Recording the Sinsemilla empty root is now load-bearing on "can't be spent" — assert it explicitly

packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs (lines 64-79)

The PR intentionally drops the current_anchor_bytes != [0u8; 32] guard, with the rationale that "the Sinsemilla empty root is a non-zero hash and recording it on first block-end after init is harmless (no notes against an empty pool to spend)." The new record_on_empty_pool_records_the_sinsemilla_empty_root test confirms the empty root is stored in [6] (i.e. validate_anchor_exists(empty_root) → true).

The safety claim — that no spend can actually succeed against the empty-root anchor — is enforced elsewhere (the spend's zk-proof would need to demonstrate ownership of a note in an empty tree). It's a consensus-critical invariant the PR now relies on, and it isn't exercised by any test in this diff. A defensive test that constructs a spend bundle anchored at the empty root and asserts it is rejected (by zk-proof validation, not by InvalidAnchorError) would harden this against future regressions in upstream zk-proof crates and document the load-bearing assumption.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-drive/src/drive/shielded/estimated_costs.rs`:
- [BLOCKING] lines 78-106: Shielded fee estimation still models the retired key-7 slot
  This PR removed the `SHIELDED_MOST_RECENT_ANCHOR_KEY = 7` `Item([u8;32])` slot from the pool, but `add_estimation_costs_for_shielded_pool_operations` still registers the pool with the pre-PR layout: comment says `9 elements total (7 subtrees + 2 items)`, `EstimatedLevel(4, false)`, `items_size: Some((1, 32, None, 2))`, and the inline comment lists `most recent anchor (Item)` as the second item. After this PR the pool actually contains 7 subtrees + 1 item (the `SHIELDED_TOTAL_BALANCE_KEY` SumItem) — `ceil(log2(8)) = 3`, and only one item slot.

This metadata feeds `ShieldedPoolOperationType::into_low_level_drive_operations()` whenever shielded ops are costed in estimation mode (`apply=false`), so every shielded note/nullifier/balance fee estimate continues to reflect the old shape. The state-shape change is the whole point of the PR; the cost estimator needs to be updated in lockstep, otherwise stateless fee estimation is structurally lying about pool size and item count.

In `packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs`:
- [SUGGESTION] lines 95-110: Use `.pop()` on the already-sorted result instead of recomputing max_key
  `entries_below` comes from `grove_get_raw_path_query` over a `Query::new()` (default `left_to_right = true`) with `RangeTo`, so GroveDB returns the entries in ascending key order (big-endian `u64` → numeric ordering). The current code clones every key into a new `Vec<u8>` just to find the maximum, then walks the vector again to filter that key out — both O(N) extra work plus an allocation per entry on a path that runs every block.

Since the highest-block-height entry is always the last element, `entries_below.pop()` (and dropping the popped value) accomplishes the same thing and makes the intent obvious.

In `packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs`:
- [SUGGESTION] lines 61-78: Asymmetric empty-index handling between proven and non-proven branches
  When the anchors-by-height index is empty:

- The non-proven branch returns `Anchor(vec![0u8; 32])` and relies on a downstream response decoder to translate the all-zeros sentinel into `None`.
- The proven branch returns `Proof(...)`; the SDK then runs `verify_most_recent_shielded_anchor_v0`, which (in this PR) returns `Ok(None)` for the genuinely-empty case — no zero-sentinel translation involved.

The two paths only stay aligned if the SDK's response decoder convention is preserved exactly. Now that the storage shape uses real absence (rather than a pre-initialised `Item([0;32])` slot), the cleanest fix is to extend the gRPC response with an explicit "absent" variant alongside `Anchor`/`Proof`, or to make `Anchor` an `Option`. At minimum, add a comment cross-referencing the SDK decoder so the coupling isn't invisible to future maintainers.

In `packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs`:
- [SUGGESTION] lines 126-134: `read_latest_recorded_shielded_anchor_v0` is `pub` purely for test convenience; doc cites a caller that doesn't use it
  This helper is exposed as `pub`. The doc comment justifies that visibility with two callers: "drive-abci's strategy tests + the `getMostRecentShieldedAnchor` non-proven query path." The first is real; the second is not — `query_most_recent_shielded_anchor_v0` calls `grove_get_raw_path_query` directly on `shielded_latest_recorded_anchor_path_query()` and never goes through this helper (it returns a `vec![0u8; 32]` sentinel; this helper returns `Option<[u8;32]>`).

So the only real reason this is `pub` rather than `pub(in crate::drive)` (matching `record_shielded_pool_anchor_if_changed_v0`) is to let the drive-abci strategy test reach across crates. Exposing internal storage helpers on the public `Drive` API surface for test convenience forces forward-compat guarantees on something that has no business being part of the SDK contract. Options: (a) `#[doc(hidden)]` + test-only documentation, (b) gate behind a `testing-helpers` feature consumed by drive-abci's `[dev-dependencies]`, or (c) keep `pub(in crate::drive)` and have the strategy test seed/read `[8]` directly. At minimum, fix the doc comment so it doesn't claim a caller that doesn't exist.

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