fix(insert): restore v4.1.0 non-batch dispatch for count/sum trees (protocol-v11 consensus) - #757
Closed
shumkov wants to merge 1 commit into
Closed
fix(insert): restore v4.1.0 non-batch dispatch for count/sum trees (protocol-v11 consensus)#757shumkov wants to merge 1 commit into
shumkov wants to merge 1 commit into
Conversation
…rotocol-v11 consensus) CountSumTree, ProvableCountTree and ProvableCountSumTree must be inserted via the plain-value path (Op::Put), not the layered-subtree path (Op::PutLayeredReference). PR #752 accidentally moved them into the layered arm of add_element_on_transaction, which changes the parent node's value_hash from value_hash(serialized) to combine_hash(value_hash(serialized), NULL_HASH) and therefore the grovedb root -- breaking protocol-v11 consensus on replay (testnet block 245,344: transition_to_version_11 inserts an empty_provable_count_sum_tree and an empty_count_sum_tree). Restores the grovedb v4.1.0 behavior frozen into the v11 activation chain. The v12-only ProvableSumTree / ProvableCountProvableSumTree keep the layered behavior (never on consensus). Adds a regression test pinning the root. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR refactors consensus-critical element insertion routing in ChangesProtocol v11 Consensus-Critical Tree Insertion Routing
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
grovedb/src/operations/insert/mod.rs (3)
629-630: 💤 Low valueDrop the
eprintln!debug output.These print on every test run and add noise to CI logs; the golden
assert_eq!already reports the actual root on failure.♻️ Proposed cleanup
- eprintln!("root_1 (control sum_tree) = {root_1:?}"); - eprintln!("root_2 (provable_count_sum_tree ins) = {root_2:?}"); -🤖 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 `@grovedb/src/operations/insert/mod.rs` around lines 629 - 630, Remove the temporary debug prints that print roots to stderr: delete the two eprintln! calls that reference root_1 and root_2 (the lines printing "root_1 (control sum_tree) = {root_1:?}" and "root_2 (provable_count_sum_tree ins) = {root_2:?}"). Keep the assertion(s) intact (the existing assert_eq! that compares the roots) so failures still report values via the test framework.
288-300: 💤 Low valueConsider relocating the consensus-critical note to the
Itemarm.This block documents
CountSumTree/ProvableCountTree/ProvableCountSumTree, but those variants are routed in theItemarm (Lines 359-374), not in the layered arm immediately below it. There is already a (shorter) note at Lines 356-358. A reader scanning the layered arm sees a long note about variants that aren't there. Either move the detailed rationale down to theItemarm or trim this block to a one-line pointer to keep the routing self-explanatory.🤖 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 `@grovedb/src/operations/insert/mod.rs` around lines 288 - 300, Move or trim the long consensus-critical comment so it sits next to the actual insertion code path for those variants: either relocate the detailed paragraph from the layered-subtree arm to the `Item` arm (the match arm handling `Op::Put` / `Item` where `CountSumTree`, `ProvableCountTree`, and `ProvableCountSumTree` are routed), or replace the layered-arm block with a single-line pointer referencing the full rationale in the `Item` arm; ensure references to the specific variants (`CountSumTree`, `ProvableCountTree`, `ProvableCountSumTree`) and the reason they must go through the plain-value `Item`/`Op::Put` path remain adjacent to the `Item` arm handling insertion.
597-648: ⚡ Quick winExtend the regression guard to the other two consensus-critical trees.
This PR changes non-batch dispatch for three types —
CountSumTree,ProvableCountTree, andProvableCountSumTree— and the comment at Lines 587-588 calls out bothempty_provable_count_sum_tree(CLEAR_ADDRESS_POOL) andempty_count_sum_tree(ADDRESS_BALANCES) as v11 insertions. The test only pinsProvableCountSumTree, leavingCountSumTreeandProvableCountTreerouting unguarded against a future re-regression to the layered path. Adding the golden root for at leastempty_count_sum_treewould close that gap.Want me to draft the additional
empty_count_sum_tree/ProvableCountTreeassertions (you'd fill in the golden hashes from a passing run)?🤖 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 `@grovedb/src/operations/insert/mod.rs` around lines 597 - 648, The test currently only asserts the golden root for empty_provable_count_sum_tree; extend the regression guard by inserting and asserting golden roots for the other two consensus-critical trees: call db.insert with Element::empty_count_sum_tree() (ADDRESS_BALANCES) and Element::empty_provable_count_tree() (CLEAR_ADDRESS_POOL variant) similar to the existing provable case, capture their root hashes (e.g., root_count_sum, root_provable_count), and add assert_eq! checks against new GOLDEN_* constants; update the test function provable_count_sum_tree_insert_preserves_v11_consensus_root to include these additional insertions and assertions so CountSumTree and ProvableCountTree routing cannot regress to layered path behavior.
🤖 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 `@grovedb/src/operations/insert/mod.rs`:
- Around line 629-630: Remove the temporary debug prints that print roots to
stderr: delete the two eprintln! calls that reference root_1 and root_2 (the
lines printing "root_1 (control sum_tree) = {root_1:?}" and "root_2
(provable_count_sum_tree ins) = {root_2:?}"). Keep the assertion(s) intact (the
existing assert_eq! that compares the roots) so failures still report values via
the test framework.
- Around line 288-300: Move or trim the long consensus-critical comment so it
sits next to the actual insertion code path for those variants: either relocate
the detailed paragraph from the layered-subtree arm to the `Item` arm (the match
arm handling `Op::Put` / `Item` where `CountSumTree`, `ProvableCountTree`, and
`ProvableCountSumTree` are routed), or replace the layered-arm block with a
single-line pointer referencing the full rationale in the `Item` arm; ensure
references to the specific variants (`CountSumTree`, `ProvableCountTree`,
`ProvableCountSumTree`) and the reason they must go through the plain-value
`Item`/`Op::Put` path remain adjacent to the `Item` arm handling insertion.
- Around line 597-648: The test currently only asserts the golden root for
empty_provable_count_sum_tree; extend the regression guard by inserting and
asserting golden roots for the other two consensus-critical trees: call
db.insert with Element::empty_count_sum_tree() (ADDRESS_BALANCES) and
Element::empty_provable_count_tree() (CLEAR_ADDRESS_POOL variant) similar to the
existing provable case, capture their root hashes (e.g., root_count_sum,
root_provable_count), and add assert_eq! checks against new GOLDEN_* constants;
update the test function
provable_count_sum_tree_insert_preserves_v11_consensus_root to include these
additional insertions and assertions so CountSumTree and ProvableCountTree
routing cannot regress to layered path behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f11de2f-66e5-4062-b3f5-8b2633043f14
📒 Files selected for processing (1)
grovedb/src/operations/insert/mod.rs
shumkov
pushed a commit
that referenced
this pull request
Jun 4, 2026
…#759) * fix(insert): version-gate add_element_on_transaction (v0=Op::Put, v1=layered) PR #752 broadened the non-batch insert tree arm so CountSumTree / ProvableCountTree / ProvableCountSumTree are written as layered subtrees (Op::PutLayeredReference). grovedb <= v4.1.0 wrote them as plain values (Op::Put), and that is the behaviour frozen into the live protocol-v11 activation chain (testnet block 245,344, transition_to_version_11). The layered op computes a different parent value_hash — combine_hash(value_hash(serialized), NULL_HASH) instead of value_hash(serialized) — and therefore a different grovedb root, a consensus divergence on replay. Rather than revert unconditionally (cf. #757), split add_element_on_transaction into a versioned dispatch, mirroring the proof v0/v1 pattern: - v0: grovedb v4.1.0 behaviour — those three types take the Op::Put arm. Selected by GROVE_V1 / GROVE_V2, preserving the protocol-v11 root. - v1: current behaviour — those three types are layered, consistent with the batch insert path (both root hash and fee). Selected by GROVE_V3. v0/v1 live in their own files as frozen snapshots; they differ only in which match arm those three element types fall into. GROVE_V3's add_element_on_transaction version slot is bumped 0 -> 1; v1/v2 stay 0. Adds a consensus-guard test that replays the transition_to_version_11 shape and pins both roots: the v0 root is byte-identical to PR #757's protocol-v11 golden, and the v1 (layered) root differs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(insert): cover all add_element_on_transaction arms under v0 and v1 Adds exhaustive non-batch-insert coverage for both frozen snapshots (v0.rs / v1.rs): the layered-tree arm (every tree type), the commitment-tree arm, the append-tree arm (MMR / bulk-append / dense), the item arm, the reference arm, both override guards, and the empty-tree-only (value.is_some) guard — driven under GROVE_V1 (v0 / Op::Put) and GROVE_V3 (v1 / layered). Also asserts the dispatcher rejects an unknown version slot. Closes the codecov/patch gap on the new add_element_on_transaction module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closed in favor of #759 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Restore the grovedb v4.1.0 non-batch insert dispatch for
CountSumTree,ProvableCountTreeandProvableCountSumTreeso they are written via the plain-value path (Op::Put) instead of the layered-subtree path (Op::PutLayeredReference).Why — consensus break at protocol v11
GroveDb::add_element_on_transaction(the non-batch insert path used bygrove_insert_if_not_exists) decides whether an element is inserted as a layered subtree (Op::PutLayeredReference) or a plain value (Op::Put).86562f65),CountSumTree/ProvableCountTree/ProvableCountSumTreefall through to_ => element.insert()→Op::Put.Op::PutLayeredReference.The two ops compute a different parent-node
value_hash:That changes the parent node hash → the grovedb root. These tree types are created on the protocol-v11 activation (Dash Platform
transition_to_version_11inserts anempty_provable_count_sum_treeand anempty_count_sum_tree), so the broadened dispatch makes a v11 node compute a different app-hash than the v4.1.0-era binaries that produced the canonical chain — a consensus divergence on replay.Evidence (testnet block 245,344, the v10→v11 activation block)
Replaying the block from the identical committed state, logging the grovedb root after each transition insert:
76d92e…76d92e…empty_sum_tree@[56]4d08e1c3…4d08e1c3…(same — SumTree is in the tree arm in both)empty_provable_count_sum_tree@[56,'c']e0f9eded…9b2b19a9…(diverges)Step 01 (
SumTree, unchanged) is identical; step 02 (ProvableCountSumTree, which changed arms) diverges. All later steps diverge downstream.The fix
In
add_element_on_transaction, moveCountSumTree/ProvableCountTree/ProvableCountSumTreeout of the layered-subtree arm back to theOp::Putarm, exactly matching v4.1.0.Tree/SumTree/BigSumTree/CountTreekeep the layered behavior (unchanged in both revs). The v12-onlyProvableSumTree/ProvableCountProvableSumTree(andCommitmentTree/MmrTree/BulkAppendTree/DenseAppendOnlyFixedSizeTree) were never on consensus and keep their current behavior.This is an accidental regression (PR #752 was "subtree dump/restore primitives"); the broadened behavior never reached consensus, so restoring v4.1.0 unconditionally is correct for all live protocol versions.
Test
provable_count_sum_tree_insert_preserves_v11_consensus_rootpins the post-insert root so the dispatch cannot silently regress again (empty_sum_treeas the unchanged control).Summary by CodeRabbit
Bug Fixes
Tests