From 0337019a54b028aa22cdfbeab87d8416da875253 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 7 May 2026 01:46:51 +0700 Subject: [PATCH 1/2] fix(drive,drive-abci): post-merge follow-ups for shielded anchor refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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>` 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) --- .../shielded/most_recent_anchor/v0/mod.rs | 27 +++++++++--- .../test_cases/shielded_tests.rs | 42 +++++++++++++++---- .../src/drive/shielded/estimated_costs.rs | 14 +++++-- .../drive/shielded/prune_anchors/v0/mod.rs | 22 +++++----- .../record_anchor_if_changed/v0/mod.rs | 21 ++++------ 5 files changed, 87 insertions(+), 39 deletions(-) diff --git a/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs b/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs index 29b7ef80c74..95380ca7319 100644 --- a/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs +++ b/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs @@ -59,13 +59,30 @@ impl Platform { )?; let entries = results.to_key_elements(); + // EMPTY-INDEX CONVENTION (see also + // `Drive::verify_most_recent_shielded_anchor_v0`): + // + // The `Anchor`/`Proof` `oneof` in the gRPC response has + // no third "absent" variant, so the proven and non-proven + // branches encode "no anchor recorded yet" differently: + // + // * Non-proven (this branch) — return + // `Anchor(vec![0u8; 32])`. Callers treat the all-zero + // anchor as "absent." Carries over from the + // pre-`[7]`-removal behaviour where the slot was + // initialised to `[0; 32]`; kept for wire-format + // compatibility. + // * Proven — return the GroveDB proof. The SDK runs + // `verify_most_recent_shielded_anchor_v0` against + // the same `limit 1` reverse `PathQuery`, which + // returns `Ok(None)` when the index is empty. + // + // If a future protocol revision extends the response + // with an explicit absent variant, drop the + // `vec![0u8; 32]` sentinel here in lock-step with both + // the verifier and the SDK decoder. let anchor_bytes = match entries.into_iter().next() { Some((_height_key, Element::Item(bytes, _))) => bytes, - // Empty index, or the entry isn't an Item (which - // would be a state-corruption bug elsewhere). Either - // way, return the zero-anchor sentinel — same shape - // the previous `[7]`-backed implementation used when - // the slot was uninitialised. _ => vec![0u8; 32], }; diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs index f8d611276eb..35c1c5d67bf 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs @@ -534,20 +534,48 @@ mod tests { // 3. Verify the derived "most recent anchor" — i.e. the // highest-block-height entry in the anchors-by-height // index — exists and matches one of the recorded anchors. - // There is no longer a separate "most recent anchor" slot; - // the index is the canonical source. - let most_recent_anchor = drive - .read_latest_recorded_shielded_anchor_v0(None, &platform_version.drive) - .expect("read latest recorded anchor") + // There is no longer a separate "most recent anchor" + // slot; the index is the canonical source. + // + // Read directly via the same path query the production + // handler uses, rather than reaching across crates for + // the internal `read_latest_recorded_shielded_anchor_v0` + // helper — its visibility is `pub(in crate::drive)` and + // a strategy test has no reason to push that to the + // public Drive surface. + use drive::drive::shielded::paths::shielded_latest_recorded_anchor_path_query; + let path_query = shielded_latest_recorded_anchor_path_query(); + let (results, _) = drive + .grove_get_raw_path_query( + &path_query, + None, + QueryResultType::QueryKeyElementPairResultType, + &mut vec![], + &platform_version.drive, + ) + .expect("query latest recorded shielded anchor"); + let mut entries = results.to_key_elements(); + let (_, most_recent_element) = entries + .pop() .expect("most recent anchor must exist after successful shields"); + let most_recent_anchor = if let Element::Item(bytes, _) = most_recent_element { + bytes + } else { + panic!("expected Item element in anchors-by-height tree"); + }; + assert_eq!( + most_recent_anchor.len(), + 32, + "most recent anchor must be 32 bytes" + ); assert_ne!( - most_recent_anchor.to_vec(), + most_recent_anchor, vec![0u8; 32], "most recent anchor must not be all zeros after successful shields" ); let is_known = anchor_to_height .iter() - .any(|(a, _)| *a == most_recent_anchor.to_vec()); + .any(|(a, _)| *a == most_recent_anchor); assert!( is_known, "most recent anchor must match one of the recorded anchors" diff --git a/packages/rs-drive/src/drive/shielded/estimated_costs.rs b/packages/rs-drive/src/drive/shielded/estimated_costs.rs index 581c87f4abe..1272460dff8 100644 --- a/packages/rs-drive/src/drive/shielded/estimated_costs.rs +++ b/packages/rs-drive/src/drive/shielded/estimated_costs.rs @@ -79,13 +79,19 @@ impl Drive { // 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), most recent anchor (Item) - // 9 elements total (7 subtrees + 2 items) → balanced Merk depth = ceil(log2(9)) = 4 + // expiration time (NormalTree). + // 8 elements total (7 subtrees + 1 item) → balanced Merk depth = ceil(log2(8)) = 3. + // + // The retired `SHIELDED_MOST_RECENT_ANCHOR_KEY = 7` slot used + // to add a second `Item` entry; the most-recent anchor is + // now derived from the highest-block-height entry in the + // anchors-by-height subtree, so the pool layer is one item + // smaller than before. estimated_costs_only_with_layer_info.insert( KeyInfoPath::from_known_path(shielded_credit_pool_path()), EstimatedLayerInformation { tree_type: TreeType::SumTree, - estimated_layer_count: EstimatedLevel(4, false), + estimated_layer_count: EstimatedLevel(3, false), estimated_layer_sizes: Mix { subtrees_size: Some(( 1, @@ -99,7 +105,7 @@ impl Drive { None, 7, // 7 subtrees: notes, permanent nullifiers, anchors, anchors-by-height, recent nullifiers, compacted nullifiers, expiration time )), - items_size: Some((1, 32, None, 2)), // 2 items: total balance (SumItem), most recent anchor (Item) + items_size: Some((1, 8, None, 1)), // 1 item: total balance (SumItem, i64 = 8 bytes) references_size: None, }, }, diff --git a/packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs b/packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs index ac0b72bdc37..8b60552f7ef 100644 --- a/packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs +++ b/packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs @@ -96,17 +96,19 @@ impl Drive { entries_below } else { // Exclude the entry with the highest block_height key. - // Keys are big-endian u64 → lexicographic comparison - // matches numeric comparison. - let max_key = entries_below - .iter() - .map(|(k, _)| k.clone()) - .max() + // `entries_below` came from a `Query::new()` (default + // `left_to_right = true`) over a `RangeTo` against + // `SHIELDED_ANCHORS_BY_HEIGHT_KEY`, whose keys are + // big-endian u64 — so GroveDB returns the entries in + // ascending numeric order and the live anchor is + // always the last element. Pop it off and discard; + // the remaining vec is exactly the prunable set, no + // extra scan or per-key clone. + let mut candidates = entries_below; + candidates + .pop() .expect("entries_below non-empty (guarded above)"); - entries_below - .into_iter() - .filter(|(k, _)| k != &max_key) - .collect() + candidates }; // 3. Delete from both trees. Order doesn't matter for diff --git a/packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs b/packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs index 8288c80a679..b83739f1039 100644 --- a/packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs +++ b/packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs @@ -115,19 +115,14 @@ impl Drive { /// (pool has never recorded an anchor — chain is at genesis or /// no shielded ops yet). /// - /// Single source of truth for "what's the most-recent anchor on - /// this chain right now": - /// - /// - `record_shielded_pool_anchor_if_changed_v0` calls this to - /// decide whether the anchor changed this block. - /// - `Platform::query_most_recent_shielded_anchor_v0` builds the - /// same path query against `grove_get_proved_path_query` so - /// the SDK's verifier can replay it byte-for-byte. - /// Public so drive-abci's strategy tests + the - /// `getMostRecentShieldedAnchor` non-proven query path can reach - /// it; both are inside the workspace and would otherwise have to - /// duplicate the path-query construction. - pub fn read_latest_recorded_shielded_anchor_v0( + /// Used by `record_shielded_pool_anchor_if_changed_v0` to decide + /// whether the anchor changed this block. The drive-abci handler + /// (`Platform::query_most_recent_shielded_anchor_v0`) and SDK + /// verifier (`Drive::verify_most_recent_shielded_anchor_v0`) + /// share the same `PathQuery` shape via + /// `shielded_latest_recorded_anchor_path_query`, but operate on + /// proofs rather than this raw helper. + pub(in crate::drive) fn read_latest_recorded_shielded_anchor_v0( &self, transaction: grovedb::TransactionArg, drive_version: &dpp::version::drive_versions::DriveVersion, From 7795b955b4403efa5a95cf476446ecd71d55d440 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 7 May 2026 02:48:53 +0700 Subject: [PATCH 2/2] fix(drive-abci): non-proven most-recent-anchor surfaces corrupted entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../query/shielded/most_recent_anchor/v0/mod.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs b/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs index 95380ca7319..7d144afe42f 100644 --- a/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs +++ b/packages/rs-drive-abci/src/query/shielded/most_recent_anchor/v0/mod.rs @@ -11,6 +11,7 @@ use dpp::check_validation_result_with_data; use dpp::validation::ValidationResult; use dpp::version::PlatformVersion; use drive::drive::shielded::paths::shielded_latest_recorded_anchor_path_query; +use drive::error::drive::DriveError; use drive::grovedb::query_result_type::QueryResultType; use drive::grovedb::Element; use drive::util::grove_operations::GroveDBToUse; @@ -81,9 +82,23 @@ impl Platform { // with an explicit absent variant, drop the // `vec![0u8; 32]` sentinel here in lock-step with both // the verifier and the SDK decoder. + // Split the three states explicitly. A non-`Item` + // element under `SHIELDED_ANCHORS_BY_HEIGHT_KEY` is a + // state-corruption signal — the prove-mode verifier + // (`verify_most_recent_shielded_anchor_v0`) and the + // raw-read helper + // (`Drive::read_latest_recorded_shielded_anchor_v0`) + // both surface this as `CorruptedProof` / + // `CorruptedElementType`, so the non-prove path must + // not silently fold it into the empty-index sentinel. let anchor_bytes = match entries.into_iter().next() { Some((_height_key, Element::Item(bytes, _))) => bytes, - _ => vec![0u8; 32], + Some(_) => { + return Err(Error::Drive(drive::error::Error::Drive( + DriveError::CorruptedElementType("anchors-by-height entry is not an Item"), + ))); + } + None => vec![0u8; 32], }; GetMostRecentShieldedAnchorResponseV0 {