fix(drive): rebalance shielded credit pool subtree keys by access frequency#3607
fix(drive): rebalance shielded credit pool subtree keys by access frequency#3607QuantumExplorer merged 2 commits intov3.1-devfrom
Conversation
…quency
Spread the eight shielded-pool subtrees across [0, 255] so GroveDB's
AVL-balanced parent tree puts the highest-traffic subtree (notes) at the
root, with the next-most-queried subtrees one hop below it, and the cold
ones at the leaves.
Layout (depth in the parent Merk tree):
[128] NOTES ← root
├── [64] NULLIFIERS ← depth 1 (every spend)
│ ├── [32] TOTAL_BALANCE ← depth 2
│ └── [96] ANCHORS_BY_HEIGHT ← depth 2
└── [192] ANCHORS_IN_POOL ← depth 1 (every spend)
├── [160] RECENT_NULLIFIERS ← depth 2
└── [224] COMPACTED_NULLIFIERS ← depth 2
└── [240] NULLIFIERS_EXPIRATION ← depth 3
The 'n'/'o'/'p' alphabetic mnemonic for recent/compacted/expiration is
dropped — those constants now use numeric byte values to fit the
balanced layout.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✨ Finishing Touches🧪 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 complete (commit a0bb99c) |
GroveDB's AVL rebalancing is order-sensitive: the depth a key ends up at depends on the order of inserts under the same parent, not just the final set of keys. Reorder both the genesis init (`v3`) and the v12 upgrade transition to insert level 0 first, then both depth-1 children, then the depth-2 children, then the depth-3 leaf — so the balanced shape described in `paths.rs` is what's actually built. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3607 +/- ##
=========================================
Coverage 88.29% 88.29%
=========================================
Files 2479 2479
Lines 301660 301660
=========================================
Hits 266350 266350
Misses 35310 35310
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two valid blocking issues. (1) The genesis path's BFS insert ordering is a no-op for tree shape: GroveDB's batch path sorts ops by key into a BTreeMap and Merk's build() recursively picks mid_index = batch.len()/2 of the sorted keys as the root, so with the chosen 8 keys the parent root is byte 160 (RECENT_NULLIFIERS), not 128 (NOTES) as the docs claim. (2) transition_to_version_12() still pre-creates only 4 of the 8 shielded subtrees that genesis creates, leaving upgraded nodes without 4 paths the runtime writes to and producing a different parent-tree shape than genesis even if the missing inserts were added (sequential AVL inserts vs sorted-batch build).
Reviewed commit: a0bb99c
🔴 2 blocking | 🟡 2 suggestion(s)
3 additional findings
🔴 blocking: transition_to_version_12 still creates only 4 of 8 shielded subtrees — diverges from genesis
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (lines 611-690)
transition_to_version_12() inserts only SHIELDED_NOTES_KEY (128), SHIELDED_NULLIFIERS_KEY (64), SHIELDED_ANCHORS_IN_POOL_KEY (192), and SHIELDED_TOTAL_BALANCE_KEY (32). The genesis path (initial_state_structure_shielded_pool_operations in packages/rs-drive/src/drive/initialization/v3/mod.rs:79-156) inserts four additional subtrees that the runtime writes to: SHIELDED_ANCHORS_BY_HEIGHT_KEY (96), SHIELDED_RECENT_NULLIFIERS_KEY_U8 (160), SHIELDED_COMPACTED_NULLIFIERS_KEY_U8 (224), and SHIELDED_NULLIFIERS_EXPIRATION_TIME_KEY_U8 (240). After a v11→v12 upgrade these paths are absent, but record_shielded_pool_anchor_if_changed_v0 (writes anchor-by-height, see packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs:96-105), store_nullifiers_for_block_v0 (packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs:73-80), and the compaction/expiration writers (packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs:120-152) all assume those subtrees exist. Compounding this: the upgrade path uses sequential grove_insert_if_not_exists (AVL-style sequential inserts), while genesis applies a single batch (Merk build() from sorted keys). Even if all 8 inserts were added in BFS order to the upgrade path, sequential vs sorted-batch construction produces different tree shapes and root hashes, so genesis-initialized and upgrade-initialized nodes will desync. Add the four missing inserts AND align the construction strategy across both paths, then add a regression test that builds the parent tree both ways and asserts equal root hashes.
🔴 blocking: BFS insert order does not place NOTES at the parent root — genesis batch builds from sorted keys via midpoint split
packages/rs-drive/src/drive/initialization/v3/mod.rs (lines 70-156)
The doc comment claims this BFS ordering is what places SHIELDED_NOTES_KEY at the root via AVL rebalancing, but the genesis batch path makes insertion order irrelevant to the resulting tree shape. Drive::grove_apply_batch routes through BatchStructure::continue_from_ops (grovedb/src/batch/batch_structure.rs:104-198), which collects ops per path into a BTreeMap<KeyInfo, GroveOp> (sorted by key). When the per-path ops are forwarded to merk.apply_unchecked/Walker::apply_to (merk/src/tree/ops.rs:148-229), the parent Merk for [AddressBalances, "s"] is None (the SumTree is being created in the same batch), so apply_to falls through to Self::build (merk/src/tree/ops.rs:235-263). build recursively splits the sorted batch at mid_index = batch.len() / 2. With the 8 sorted child keys [32, 64, 96, 128, 160, 192, 224, 240], mid_index = 4, so the root is batch[4] = 160 (SHIELDED_RECENT_NULLIFIERS_KEY_U8) — not 128 (SHIELDED_NOTES_KEY). Recursive splits then place 96 and 224 at depth 1, 32/128/192/240 at depth 2, and 64 at depth 3. The BFS reorder is a no-op for genesis, and the access-frequency rationale (NOTES at root, spend-path keys at depth 1) does not hold. To genuinely pin NOTES at depth 0 you must either (a) pick a key value that lands at batch.len()/2 after sorting (with 8 keys including NOTES, that's the 5th-smallest position), or (b) avoid the batch path so insertion order matters (sequential grove_insert_if_not_exists).
🟡 suggestion: No test asserts the shielded-pool parent tree shape (root key / per-key depths)
packages/rs-drive/src/drive/shielded/paths.rs (lines 217-280)
The PR's central premise — that the chosen key bytes plus BFS insert order yield a specific Merk shape — is not verified anywhere. The existing tests cover read/write behavior, which works regardless of parent-tree layout. Add a test that initializes a fresh platform via the genesis path, opens the Merk at [AddressBalances, "s"], and asserts (a) the root key, (b) the depth/parent of each of the 8 child keys, and (c) the root hash. Repeat for the v11→v12 upgrade path and assert byte-identical root hashes. Without such a test, future changes to GroveDB's batch-build heuristics or to the key set would silently undo the intended layout.
🤖 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/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`:
- [BLOCKING] lines 611-690: transition_to_version_12 still creates only 4 of 8 shielded subtrees — diverges from genesis
`transition_to_version_12()` inserts only `SHIELDED_NOTES_KEY (128)`, `SHIELDED_NULLIFIERS_KEY (64)`, `SHIELDED_ANCHORS_IN_POOL_KEY (192)`, and `SHIELDED_TOTAL_BALANCE_KEY (32)`. The genesis path (`initial_state_structure_shielded_pool_operations` in `packages/rs-drive/src/drive/initialization/v3/mod.rs:79-156`) inserts four additional subtrees that the runtime writes to: `SHIELDED_ANCHORS_BY_HEIGHT_KEY (96)`, `SHIELDED_RECENT_NULLIFIERS_KEY_U8 (160)`, `SHIELDED_COMPACTED_NULLIFIERS_KEY_U8 (224)`, and `SHIELDED_NULLIFIERS_EXPIRATION_TIME_KEY_U8 (240)`. After a v11→v12 upgrade these paths are absent, but `record_shielded_pool_anchor_if_changed_v0` (writes anchor-by-height, see `packages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rs:96-105`), `store_nullifiers_for_block_v0` (`packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs:73-80`), and the compaction/expiration writers (`packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs:120-152`) all assume those subtrees exist. Compounding this: the upgrade path uses sequential `grove_insert_if_not_exists` (AVL-style sequential inserts), while genesis applies a single batch (Merk `build()` from sorted keys). Even if all 8 inserts were added in BFS order to the upgrade path, sequential vs sorted-batch construction produces different tree shapes and root hashes, so genesis-initialized and upgrade-initialized nodes will desync. Add the four missing inserts AND align the construction strategy across both paths, then add a regression test that builds the parent tree both ways and asserts equal root hashes.
In `packages/rs-drive/src/drive/initialization/v3/mod.rs`:
- [BLOCKING] lines 70-156: BFS insert order does not place NOTES at the parent root — genesis batch builds from sorted keys via midpoint split
The doc comment claims this BFS ordering is what places `SHIELDED_NOTES_KEY` at the root via AVL rebalancing, but the genesis batch path makes insertion order irrelevant to the resulting tree shape. `Drive::grove_apply_batch` routes through `BatchStructure::continue_from_ops` (`grovedb/src/batch/batch_structure.rs:104-198`), which collects ops per path into a `BTreeMap<KeyInfo, GroveOp>` (sorted by key). When the per-path ops are forwarded to `merk.apply_unchecked`/`Walker::apply_to` (`merk/src/tree/ops.rs:148-229`), the parent Merk for `[AddressBalances, "s"]` is `None` (the SumTree is being created in the same batch), so `apply_to` falls through to `Self::build` (`merk/src/tree/ops.rs:235-263`). `build` recursively splits the sorted batch at `mid_index = batch.len() / 2`. With the 8 sorted child keys `[32, 64, 96, 128, 160, 192, 224, 240]`, `mid_index = 4`, so the root is `batch[4] = 160` (`SHIELDED_RECENT_NULLIFIERS_KEY_U8`) — not 128 (`SHIELDED_NOTES_KEY`). Recursive splits then place 96 and 224 at depth 1, 32/128/192/240 at depth 2, and 64 at depth 3. The BFS reorder is a no-op for genesis, and the access-frequency rationale (NOTES at root, spend-path keys at depth 1) does not hold. To genuinely pin NOTES at depth 0 you must either (a) pick a key value that lands at `batch.len()/2` after sorting (with 8 keys including NOTES, that's the 5th-smallest position), or (b) avoid the batch path so insertion order matters (sequential `grove_insert_if_not_exists`).
In `packages/rs-drive/src/drive/shielded/paths.rs`:
- [SUGGESTION] lines 10-28: Parent-tree layout diagram does not match the tree genesis actually builds
The ASCII diagram shows `[128] NOTES` at the root with `[64] NULLIFIERS` and `[192] ANCHORS_IN_POOL` at depth 1, but the genesis batch produces a tree with `[160] RECENT_NULLIFIERS` at root, `[96]` and `[224]` at depth 1, etc. (see the blocking finding on `initialization/v3/mod.rs`). Either update the diagram to reflect the actual midpoint-split shape, or update the diagram together with a code change that genuinely produces it. Leaving this stale will mislead future readers into making access-pattern decisions based on a layout that does not exist.
- [SUGGESTION] lines 217-280: No test asserts the shielded-pool parent tree shape (root key / per-key depths)
The PR's central premise — that the chosen key bytes plus BFS insert order yield a specific Merk shape — is not verified anywhere. The existing tests cover read/write behavior, which works regardless of parent-tree layout. Add a test that initializes a fresh platform via the genesis path, opens the Merk at `[AddressBalances, "s"]`, and asserts (a) the root key, (b) the depth/parent of each of the 8 child keys, and (c) the root hash. Repeat for the v11→v12 upgrade path and assert byte-identical root hashes. Without such a test, future changes to GroveDB's batch-build heuristics or to the key set would silently undo the intended layout.
| // The eight subtree keys of the shielded credit pool are placed at evenly-spaced | ||
| // byte positions across [0, 255] so that GroveDB's AVL-balanced parent tree | ||
| // puts the highest-traffic subtree (`SHIELDED_NOTES_KEY`) at the root, with the | ||
| // next-most-queried subtrees one hop below it, and the cold ones at the leaves: | ||
| // | ||
| // [128] NOTES ← root, every wallet sync | ||
| // / \ | ||
| // [64] NULLIFIERS [192] ANCHORS_IN_POOL | ||
| // / \ / \ | ||
| // [32] TOTAL [96] BY_HEIGHT [160] RECENT [224] COMPACTED | ||
| // \ | ||
| // [240] EXPIRATION | ||
| // | ||
| // Within a depth tier (children of a given internal node), placement is by | ||
| // access frequency: the spend-path subtrees (`NULLIFIERS`, `ANCHORS_IN_POOL`) | ||
| // are at depth 1; periodic-write subtrees (`COMPACTED_NULLIFIERS`, | ||
| // `EXPIRATION_TIME`) sit at the leaves. Key 7 is the historical | ||
| // `SHIELDED_MOST_RECENT_ANCHOR_KEY` slot — see retired-key note below. | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: Parent-tree layout diagram does not match the tree genesis actually builds
The ASCII diagram shows [128] NOTES at the root with [64] NULLIFIERS and [192] ANCHORS_IN_POOL at depth 1, but the genesis batch produces a tree with [160] RECENT_NULLIFIERS at root, [96] and [224] at depth 1, etc. (see the blocking finding on initialization/v3/mod.rs). Either update the diagram to reflect the actual midpoint-split shape, or update the diagram together with a code change that genuinely produces it. Leaving this stale will mislead future readers into making access-pattern decisions based on a layout that does not exist.
source: ['claude']
🤖 Fix this 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/paths.rs`:
- [SUGGESTION] lines 10-28: Parent-tree layout diagram does not match the tree genesis actually builds
The ASCII diagram shows `[128] NOTES` at the root with `[64] NULLIFIERS` and `[192] ANCHORS_IN_POOL` at depth 1, but the genesis batch produces a tree with `[160] RECENT_NULLIFIERS` at root, `[96]` and `[224]` at depth 1, etc. (see the blocking finding on `initialization/v3/mod.rs`). Either update the diagram to reflect the actual midpoint-split shape, or update the diagram together with a code change that genuinely produces it. Leaving this stale will mislead future readers into making access-pattern decisions based on a layout that does not exist.
2 participants
Issue being fixed or feature implemented
The eight shielded credit pool subtrees were placed at byte positions
1, 2, 5, 6, 8, 'n', 'o', 'p'(110, 111, 112) — clustered around 0 and the lower-ASCII range. In GroveDB's AVL-balanced parent Merk tree this skewed the tree, putting the highest-traffic subtree (SHIELDED_NOTES_KEY) deep on one side instead of at the root.What was done?
Rebalanced the eight subtree keys across
[0, 255]so the most-queried subtrees sit closest to the parent-tree root. Sorted-position layout:SHIELDED_TOTAL_BALANCE_KEYSHIELDED_NULLIFIERS_KEYSHIELDED_ANCHORS_BY_HEIGHT_KEYSHIELDED_NOTES_KEYSHIELDED_RECENT_NULLIFIERS_KEYSHIELDED_ANCHORS_IN_POOL_KEYSHIELDED_COMPACTED_NULLIFIERS_KEYSHIELDED_NULLIFIERS_EXPIRATION_TIME_KEY`'n'`/`'o'`/`'p'` lose their alphabetic mnemonic — those constants now use numeric bytes to fit the layout. All accesses go through the constants, so the only behavioral change is the on-chain storage layout.
Doc comments referencing the old path bytes (
[..., "s", [1]],[..., "s", [6]], etc.) were updated to match.How Has This Been Tested?
Breaking Changes
State-structure change. Pre-launch / dev branch (v3.1-dev), so this lands as a direct edit to `create_initial_state_structure_v3` and `transition_to_version_12` (constants only — no byte literals at the call sites). No on-chain migration needed since this is pre-release. Following the precedent of #3605 which retired key 7 the same way.
Checklist:
🤖 Generated with Claude Code