fix(drive,drive-abci): post-merge follow-ups for shielded anchor refactor#3606
Conversation
…ctor Four review findings from the automated review of #3605, all verified valid against the merged code on v3.1-dev. 1. [BLOCKING] estimated_costs.rs: shielded credit pool layer info still modeled the pre-PR layout — `EstimatedLevel(4, false)`, `items_size: Some((1, 32, None, 2))`, comment "9 elements (7 subtrees + 2 items)". After retiring `[7]` the pool actually holds 7 subtrees + 1 item (the `SHIELDED_TOTAL_BALANCE_KEY` SumItem). Updated to `EstimatedLevel(3, false)` (ceil(log2(8)) = 3) and `items_size: Some((1, 8, None, 1))` (one i64 SumItem). Stateless fee estimation for shielded ops would otherwise over- report pool depth + item count by one each. 2. prune_anchors_v0 was building a `Vec<Vec<u8>>` of every key below cutoff just to find the maximum, then walking the collection again to filter that key out — O(N) extra work + a per-entry allocation on a path that runs every block. The `entries_below` vec comes from a `Query::new()` (default `left_to_right = true`) over a `RangeTo`, so GroveDB returns the entries in ascending big-endian-u64 → numeric order; the live anchor is the last element. Pop it and discard, no scan no allocation. 3. The `getMostRecentShieldedAnchor` non-proven branch returns `Anchor(vec![0u8; 32])` for an empty index; the proven branch returns a proof that `verify_most_recent_shielded_anchor_v0` maps to `Ok(None)`. Asymmetric handling that only stays coherent if the SDK's response decoder treats the all-zeros anchor as "absent" — preserved for wire-format compat. Added a comment block at the sentinel-emitting site cross-referencing the verifier and noting that any future "absent" variant on the response message would need to retire the sentinel in lock-step. 4. `Drive::read_latest_recorded_shielded_anchor_v0` was bumped to `pub` solely to let the strategy test reach across crates, and the doc justified that with a caller (the non-proven query path) that doesn't actually use it. Reverted visibility to `pub(in crate::drive)` to match `record_shielded_pool_anchor_if_changed_v0`, fixed the doc to describe the real callers, and inlined the strategy test's read against the same shared `PathQuery` (`shielded_latest_recorded_anchor_path_query`) the production handler uses. Internal storage helpers don't need to bleed onto the public Drive surface for test convenience. 60 drive shielded + 22 verify-shielded + 54 drive-abci shielded query tests pass; clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAnchor handling and storage were adjusted: the shielded credit pool layout was simplified (9→8 elements, depth 4→3), anchor derivation moved to the anchors-by-height subtree, helper visibility tightened, tests switched to path-query-based anchor reads, and the non‑proved query path now errors (CorruptedElementType) on unexpected element types instead of returning the zero-anchor sentinel. ChangesShielded Anchor Management Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Review GateCommit:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3606 +/- ##
============================================
- Coverage 88.29% 88.29% -0.01%
============================================
Files 2479 2479
Lines 301660 301659 -1
============================================
- Hits 266350 266346 -4
- Misses 35310 35313 +3
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted post-merge follow-up to PR #3605: fixes the shielded-pool estimated-costs metadata to match the new 8-element layout, swaps the prune-anchors max scan for a .pop() on already-sorted results, documents the empty-index sentinel convention, and reverts a helper visibility. All four changes verified correct against source. One real asymmetry remains: the non-proven query path silently collapses corrupted entries to the empty-index sentinel, while the proven path returns CorruptedProof.
Reviewed commit: 0337019
🟡 1 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
💬 nitpick: Pin `left_to_right = true` so `.pop()` correctness is locally self-checking
packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs (lines 45-46)
The new .pop() strategy at line 109 is only correct because Query::new() defaults left_to_right to true and the keys are big-endian u64, so GroveDB returns ascending numeric order and the live anchor is the last element. The comment at lines 99-106 explains this, but the assumption isn't pinned in code: a future refactor of the query — or a change to GroveDB's default — would silently flip which entry survives pruning, producing the exact desync this function exists to prevent. Setting below_query.left_to_right = true; explicitly after construction makes the invariant local and self-checking with no runtime cost.
💡 Suggested change
let mut below_query = Query::new();
below_query.left_to_right = true;
below_query.insert_item(QueryItem::RangeTo(..cutoff_height.to_be_bytes().to_vec()));
💬 nitpick: No regression test pins the shielded credit pool layer shape (depth + item count)
packages/rs-drive/src/drive/shielded/estimated_costs.rs (lines 90-112)
This file lacks a unit/integration test asserting that add_estimation_costs_for_shielded_pool_operations registers the pool layer with the actual on-disk shape. The original mismatch this PR fixes (depth 4, items=2 after [7] was retired) shipped because no test compared the registered EstimatedLayerInformation against the production tree's actual subtree/item counts. A small assertion-style test — insert all real subtree keys + the SumItem in setup_drive_with_initial_state_structure, then read the tree and assert EstimatedLevel(ceil(log2(n)), false) and items_size.weight == observed_items — would catch the next time someone adds or removes a slot from the pool layer without updating this file. The PR fixes the current mismatch by hand; a structural test would prevent the next round.
🤖 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-abci/src/query/shielded/most_recent_anchor/v0/mod.rs`:
- [SUGGESTION] lines 84-87: Non-proven path silently maps corrupted anchor entries to the empty-index sentinel
The new comment block documents the empty-index convention but the catch-all `_ =>` arm still flattens two semantically distinct cases into `vec![0u8; 32]`: (1) a genuinely empty index, and (2) a non-`Item` element under `SHIELDED_ANCHORS_BY_HEIGHT_KEY`, which would indicate state corruption. The proven path (`verify_most_recent_shielded_anchor_v0` at lines 50-54) explicitly returns `ProofError::CorruptedProof` for the same condition, and `read_latest_recorded_shielded_anchor_v0` returns `DriveError::CorruptedElementType` — so the two API modes diverge on the same underlying state, and clients hitting the non-proven gRPC path lose the ability to distinguish absence from a broken invariant. The pre-PR comment used to document this case explicitly; the new comment only covers the empty-index path. Either split the match so a non-`Item` returns `DriveError::CorruptedElementType` (matching the other read paths), or restore an explicit note in the comment that the catch-all also masks the corruption case. If applying the concrete suggestion, wrap `DriveError::CorruptedElementType` in the outer `drive::error::Error::Drive` variant required by the drive-abci error type.
…ries
The previous catch-all `_ => vec![0u8; 32]` arm collapsed two
semantically distinct states into the empty-index sentinel:
* `None` — index genuinely empty (never recorded an anchor).
* `Some((_, non-`Item`))` — state corruption under
`SHIELDED_ANCHORS_BY_HEIGHT_KEY`.
The proven path (`verify_most_recent_shielded_anchor_v0`) already
surfaces the second case as `ProofError::CorruptedProof`; the raw
read helper (`Drive::read_latest_recorded_shielded_anchor_v0`)
surfaces it as `DriveError::CorruptedElementType`. Folding it into
the zero sentinel on the non-prove gRPC path made the two API
modes diverge on the same underlying state — clients couldn't tell
absence from a broken invariant.
Split the match into three explicit arms; corruption now returns
`Error::Drive(drive::error::Error::Drive(DriveError::CorruptedElementType(...)))`,
matching the rest of the read paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs (1)
63-93: 💤 Low valueOptional: consolidate the two adjacent comment blocks.
The EMPTY-INDEX CONVENTION block (lines 63–84) and the corruption-handling block (lines 85–93) are stacked without a separator and partially restate context (both reference the proven-path verifier). Consider merging them into a single block with two sub-sections (e.g., "Empty index" and "Corruption") so the rationale for each match arm is easier to scan and future edits don't drift between the two blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs` around lines 63 - 93, Merge the two adjacent comment blocks into a single consolidated block that has two short subsections (e.g., "Empty index" and "Corruption"): in "Empty index" describe the EMPTY-INDEX CONVENTION and the vec![0u8; 32] sentinel and how the proven path uses verify_most_recent_shielded_anchor_v0 returning Ok(None) for an empty index; in "Corruption" state the rule that a non-`Item` element under SHIELDED_ANCHORS_BY_HEIGHT_KEY is a state-corruption signal and reference the prove-mode verifier (verify_most_recent_shielded_anchor_v0) and the raw-read helper (Drive::read_latest_recorded_shielded_anchor_v0) which surface this as CorruptedProof / CorruptedElementType, making clear the non-prove path must not fold corrupted elements into the empty-index sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs`:
- Around line 63-93: Merge the two adjacent comment blocks into a single
consolidated block that has two short subsections (e.g., "Empty index" and
"Corruption"): in "Empty index" describe the EMPTY-INDEX CONVENTION and the
vec![0u8; 32] sentinel and how the proven path uses
verify_most_recent_shielded_anchor_v0 returning Ok(None) for an empty index; in
"Corruption" state the rule that a non-`Item` element under
SHIELDED_ANCHORS_BY_HEIGHT_KEY is a state-corruption signal and reference the
prove-mode verifier (verify_most_recent_shielded_anchor_v0) and the raw-read
helper (Drive::read_latest_recorded_shielded_anchor_v0) which surface this as
CorruptedProof / CorruptedElementType, making clear the non-prove path must not
fold corrupted elements into the empty-index sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a5c80a9-1841-42aa-bfec-f6a0a1f4613e
📒 Files selected for processing (1)
packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs
|
Self Reviewed |
2 participants
Issue being fixed or feature implemented
Follow-up review of #3605 (retire
SHIELDED_MOST_RECENT_ANCHOR_KEY, derive most-recent from[8], never empty the index under prune). Four findings from the automated review, all verified valid against the merged v3.1-dev code.What was done?
1. [BLOCKING]
estimated_costs.rs: layout was still pre-PRadd_estimation_costs_for_shielded_pool_operationsregistered the shielded credit pool with the old shape —EstimatedLevel(4, false),items_size: Some((1, 32, None, 2)), comment claiming "9 elements (7 subtrees + 2 items)" — even though[7]is gone. The pool now holds 7 subtrees + 1 item (theSHIELDED_TOTAL_BALANCE_KEYSumItem):EstimatedLevel(4, false)→EstimatedLevel(3, false)(ceil(log2(8)) = 3)items_size: Some((1, 32, None, 2))→Some((1, 8, None, 1))(one i64 SumItem, 8 bytes)This metadata feeds
ShieldedPoolOperationType::into_low_level_drive_operations()whenever shielded ops are costed in estimation mode (apply=false), so without this fix every shielded note/nullifier/balance fee estimate would have over-reported pool depth + item count by one each.2.
prune_anchors_v0: use.pop()instead of recomputingmax_keyentries_belowcomes fromgrove_get_raw_path_queryover aQuery::new()(defaultleft_to_right = true) withRangeToagainstSHIELDED_ANCHORS_BY_HEIGHT_KEY, whose keys are big-endian u64 — so GroveDB returns the entries in ascending numeric order. The previous code cloned every key into a newVec<u8>to find the maximum, then walked the vec again to filter that key out — O(N) extra work plus per-entry allocation on a path that runs every block..pop()on the already-sorted result accomplishes the same thing with zero scans or allocations.3.
query_most_recent_shielded_anchor_v0: document the empty-index conventionThe non-proven branch returns
Anchor(vec![0u8; 32])when[8]is empty; the proven branch returns a proof thatverify_most_recent_shielded_anchor_v0decodes toOk(None). Asymmetric handling that only stays coherent if the SDK's response decoder keeps treating the all-zeros anchor as "absent." The right long-term fix is a third "absent" variant on the gRPConeof, but that's a wire-format change. For now, added a comment block at the sentinel-emitting site cross-referencing the verifier and flagging the lock-step requirement, so the implicit convention isn't invisible to future maintainers.4.
read_latest_recorded_shielded_anchor_v0: revert to crate-localThe helper was bumped to
pubpurely so the drive-abci strategy test could reach across crates. The doc justified that visibility with a caller (the non-provengetMostRecentShieldedAnchorpath) that doesn't actually use it — that path callsgrove_get_raw_path_querydirectly onshielded_latest_recorded_anchor_path_query()and parses the result inline. Reverted topub(in crate::drive)matchingrecord_shielded_pool_anchor_if_changed_v0, fixed the doc, and inlined the strategy test's read against the same sharedPathQuerythe production handler uses. Internal storage helpers don't need to bleed onto the public Drive surface for test convenience.How Has This Been Tested?
cargo test -p drive --lib drive::shielded→ 60 passedcargo test -p drive --lib verify::shielded→ 22 passedcargo test -p drive-abci --lib query::shielded→ 54 passedcargo clippy -p drive -p drive-abci --all-targetscleancargo fmt --allcleanBreaking Changes
None. The estimated-costs fix changes fee-estimation outputs by one Merk depth level + one item slot for shielded pool root-layer operations — small, in the direction of "now correct" — but doesn't affect on-chain state shape or RPC wire format. v3.1-dev hasn't shipped, so the v0 method bodies are amended in place.
Checklist:
Related
Follows up #3605; unblocks the spend rows of #3603 once the regtest restarts past the prune fix landing.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation