feat(replication): state sync support for the append-only tree family (fixes #785) - #788
feat(replication): state sync support for the append-only tree family (fixes #785)#788QuantumExplorer wants to merge 5 commits into
Conversation
Investigation tests for the state-sync gap in the append-only tree
family (CommitmentTree / MmrTree / BulkAppendTree /
DenseAppendOnlyFixedSizeTree):
- a populated CommitmentTree bricks source-side fetch_chunk with an
opaque CorruptedData ("cannot create chunk producer for empty
Merk"): is_empty_tree() raw-iterates the prefix namespace, sees the
non-Merk payload entries, and the chunk producer then fails on the
rootless Merk
- the same failure reproduces for populated MmrTree, BulkAppendTree,
and DenseAppendOnlyFixedSizeTree
- an EMPTY CommitmentTree syncs fine, demonstrating that a naive
skip/empty-chunk fix would silently commit a destination missing the
frontier and note payload (restore never recomputes non-Merk state
roots; the app-hash check passes on the byte-identical parent Merk)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e sync (Phase 0 of #785) A populated CommitmentTree / MmrTree / BulkAppendTree / DenseAppendOnlyFixedSizeTree previously made source-side fetch_chunk fail with an opaque CorruptedData ("cannot create chunk producer for empty Merk") when a syncing peer requested the subtree's chunk. Reject instead with a descriptive NotSupported on both sides: - target-side discovery (discover_new_subtrees_metadata) rejects when it encounters a populated non-Merk tree element, mirroring the indexed-tree guards from #778 - source-side fetch_chunk rejects when the requested prefix has a non-empty namespace under a non-Merk tree type, where the chunk producer would otherwise fail on the rootless Merk Empty append-only trees keep syncing as before (no payload exists; the element itself is restored via the parent Merk). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… replay (Phase 1 of #785) Adds state-sync transfer for the non-Merk append-only tree family: CommitmentTree, MmrTree, BulkAppendTree, and DenseAppendOnlyFixedSizeTree. Previously a single populated tree of any of these types made every snapshot from the holding node unusable. Design — target-driven entry replay: - The target holds the subtree's element (counts + parameters) from the hash-verified parent Merk, and encodes a (start, state, param) page cursor into every local chunk id it requests. - The source serves pages of leaf entries only (plus the serialized Sinsemilla frontier on a commitment tree's first page — it is an accumulator and cannot be replayed without redoing every Sinsemilla hash), read through the same accessors normal reads use. - The target replays each entry through the real append primitives (BulkAppendTree::append / MMR::push / DenseFixedSizedMerkleTree:: insert), so every internal node, chunk blob, and cached hash on the target is locally derived from the wire entries. - At subtree completion the target recomputes the type-specific state root from its own storage (new strict GroveDb:: compute_non_merk_state_root) and requires combine_hash( value_hash(element_bytes), state_root) to equal the parent binding. Any tampering with wire bytes — entries, frontier, counts — fails the sync instead of committing corrupt state. Protocol notes: - CURRENT_STATE_SYNC_VERSION stays 1: mixed old/new peers fail safe (a cursor-less request for an append-only subtree gets a descriptive NotSupported; an old source cannot serve pages), with no silent corruption in either direction. - Node-local wire behavior only — no committed hashes change, so no GroveVersion gating. Tests: round trips for all four types (multi-epoch commitment tree, multi-chunk bulk tree, multi-page MMR transfer), byzantine-source tamper rejection (flipped entry byte, stripped frontier, tampered frontier, dropped entry), subtree-batch-boundary interleaving, and the old-peer cursor-less rejection path. Closes #785 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe replication system now synchronizes non-Merk append-only subtrees through cursor-based pages. It replays commitment, bulk-append, MMR, and dense tree entries, validates reconstructed roots against parent Merk hashes, and covers round trips, tampering, malformed input, and empty trees. ChangesNon-Merk append-only replication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StateSyncSession
participant GroveDb
participant NonMerkRestorer
participant AppendOnlyTree
participant ParentMerk
StateSyncSession->>GroveDb: request cursor-based page
GroveDb-->>StateSyncSession: encoded append-only page
StateSyncSession->>NonMerkRestorer: apply page
NonMerkRestorer->>AppendOnlyTree: replay entries
AppendOnlyTree-->>NonMerkRestorer: updated tree state
StateSyncSession->>NonMerkRestorer: finalize replay
NonMerkRestorer->>ParentMerk: validate reconstructed state binding
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (80.24%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #788 +/- ##
===========================================
- Coverage 92.11% 92.11% -0.01%
===========================================
Files 257 258 +1
Lines 77936 78920 +984
===========================================
+ Hits 71794 72696 +902
- Misses 6142 6224 +82
🚀 New features to boost your workflow:
|
…tree round trips Raises patch coverage on the #785 entry-replay code: - direct malformed-input coverage for NonMerkRestorer (bad cursor length, out-of-order cursor, undecodable page, more-without-entries, entry overflow, missing frontier, premature finalize, aux on a non-commitment-tree page, page after final) - empty MmrTree / BulkAppendTree / DenseAppendOnlyFixedSizeTree round trip, covering the empty-tree state-root conventions in compute_non_merk_state_root Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
grovedb/src/replication.rs (1)
88-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the
fetch_chunkdoc notes for the append-only path.The Notes section states that the function opens a
Merktree for each chunk and that empty trees return an empty byte vector. Append-only subtrees now bypass both behaviors: they serve cursor-based entry pages and reject requests without a page cursor. Add that case so callers of this public method know the new contract.📝 Suggested doc addition
/// - The function opens a `Merk` tree for each chunk and retrieves the /// associated data. /// - Empty trees return an empty byte vector. + /// - Non-Merk append-only subtrees (`CommitmentTree`, `MmrTree`, + /// `BulkAppendTree`, `DenseAppendOnlyFixedSizeTree`) are served as + /// cursor-based entry pages instead of Merk chunks. A request for one + /// of these subtrees without a page cursor returns + /// `Error::NotSupported`. + /// - Indexed-tree requests return `Error::NotSupported`.🤖 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/replication.rs` around lines 88 - 96, Update the Notes section of the public fetch_chunk documentation to describe append-only subtrees: they serve cursor-based entry pages and reject requests that lack a page cursor, rather than opening Merk trees or returning empty vectors for empty trees. Keep the existing notes for non-append-only chunks unchanged.grovedb/src/replication/state_sync_session.rs (1)
541-550: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the
set_new_transactionconstraint in the SAFETY comment.
apply_chunkdiffers from the two call sites this comment cites. Unlikeadd_subtree_sync_infoanddiscover_new_subtrees_metadata,apply_chunkcan replace and commitself.transactionitself, at Line 698 viaset_new_transaction. After that call,transaction_refpoints at a committed and dropped transaction. The current code is sound because the last use oftransaction_refis at Line 644, inside the loop that ends before Line 698. That ordering is not stated anywhere, so a future edit that moves a use below the loop would introduce a use-after-free without any compiler diagnostic.🛡️ Suggested comment extension
let db = self.db; // SAFETY: the transaction lives as long as the pinned session and is // dropped last; the reference is only used within this call while // the session is alive. This mirrors the pattern used by // `add_subtree_sync_info` and `discover_new_subtrees_metadata`. + // + // ADDITIONAL INVARIANT for this call site: `set_new_transaction()` + // below replaces and commits `self.transaction`, which invalidates + // `transaction_ref`. Every use of `transaction_ref` MUST stay inside + // the per-chunk loop, above the `set_new_transaction()` call. Do not + // use `transaction_ref` after that point. let transaction_ref: &'db Transaction<'db> = unsafe {🤖 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/replication/state_sync_session.rs` around lines 541 - 550, Update the SAFETY comment above transaction_ref in apply_chunk to document that set_new_transaction may replace and commit/drop self.transaction, and that transaction_ref must not be used after the loop’s final use before that call. Preserve the existing lifetime rationale while explicitly requiring all transaction_ref accesses to remain before set_new_transaction.grovedb/src/tests/replication_session_tests.rs (2)
1354-1367: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting that the transfer actually used more than one page.
This test depends on
MAX_PAGE_BYTESstaying at 1 MiB: 4 leaves of 400 KiB total 1.6 MiB. If that constant is later raised above 1.6 MiB, the transfer completes in a single page. The test still passes, but it no longer covers the multi-page path it is named for. Count the applied chunks, or size the payload from the constant, so the coverage cannot silently disappear.// Sizing the payload from the constant keeps the split guaranteed: use crate::replication::non_merk_sync::MAX_PAGE_BYTES; // needs pub(crate) let leaf_size = MAX_PAGE_BYTES / 2 + 1; // any two leaves exceed one page🤖 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/tests/replication_session_tests.rs` around lines 1354 - 1367, Update the replication test around the mmr_tree_append loop so it cannot silently stop exercising multi-page transfers when MAX_PAGE_BYTES changes. Size the appended payloads from MAX_PAGE_BYTES to guarantee the total exceeds one page, or count applied chunks and assert that more than one page was used; expose MAX_PAGE_BYTES as pub(crate) if needed.
1423-1468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider consolidating the four sync-driver loops.
This file now contains four near-identical fetch/apply loops:
sync_source_to_destination(Lines 48-68),try_sync_source_to_destination(Lines 742-756), this helper (Lines 1423-1468), and the inline loop instate_sync_non_merk_trees_with_batch_size_one(Lines 1670-1688). They differ only in batch size, error handling, and the optional page mutation. One driver taking a batch size and an optional mutation hook would cover all four call sites and keep future protocol changes to a single place.🤖 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/tests/replication_session_tests.rs` around lines 1423 - 1468, Consolidate the duplicated chunk fetch/apply logic from sync_source_to_destination, try_sync_source_to_destination, the current helper, and state_sync_non_merk_trees_with_batch_size_one into one shared sync driver. Parameterize it for batch size, error-handling behavior, and an optional page-mutation hook, preserving each caller’s existing semantics; route all four call sites through the driver so fetch, apply, and queue-extension behavior has one implementation.grovedb/src/lib.rs (1)
2489-2590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving
compute_non_merk_child_hashfrom this strict variant.
compute_non_merk_state_rootandcompute_non_merk_child_hash(Lines 2387-2464) now contain the same four per-type reconstruction branches. They differ only in failure policy: the strict variant returnsCorruptedData, the lenient one falls back tomerk_root_hash. The empty-tree branches also already express the same value two ways (NULL_HASHhere,merk_root_hashthere), which is the exact kind of drift a shared implementation prevents. Both feed hashes that are compared against consensus-bound parent bindings, so the two must never diverge.♻️ Suggested direction
fn compute_non_merk_child_hash<'b, B: AsRef<[u8]>>( &self, element: &Element, subtree_path: SubtreePath<'b, B>, transaction: &Transaction, merk_root_hash: [u8; 32], ) -> [u8; 32] { - match element { - // ... duplicated per-type reconstruction ... - } + self.compute_non_merk_state_root(element, subtree_path, transaction) + .unwrap_or(merk_root_hash) }Note that this makes the empty
BulkAppendTree/MmrTree/DenseAppendOnlyFixedSizeTreecases returnNULL_HASHexplicitly instead of the passed-inmerk_root_hash; confirm those are identical for an empty inner Merk before applying.🤖 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/lib.rs` around lines 2489 - 2590, Refactor compute_non_merk_child_hash to reuse the per-type reconstruction logic from compute_non_merk_state_root so both functions produce identical hashes. Preserve the lenient function’s fallback to merk_root_hash on reconstruction errors while retaining CorruptedData propagation in the strict function, and confirm empty inner Merk cases use the same NULL_HASH value before sharing the implementation.
🤖 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.
Inline comments:
In `@grovedb/src/tests/replication_session_tests.rs`:
- Around line 1544-1559: In the frontier-tamper case within
try_sync_with_ct_page_mutation, replace the broad "cannot" alternative with
exact, frontier-specific error substrings that represent the valid
tampered-frontier rejection paths. Keep the existing "state root mismatch after
replay" match if applicable, and align the assertion with the exact-substring
style used by the other three cases.
---
Nitpick comments:
In `@grovedb/src/lib.rs`:
- Around line 2489-2590: Refactor compute_non_merk_child_hash to reuse the
per-type reconstruction logic from compute_non_merk_state_root so both functions
produce identical hashes. Preserve the lenient function’s fallback to
merk_root_hash on reconstruction errors while retaining CorruptedData
propagation in the strict function, and confirm empty inner Merk cases use the
same NULL_HASH value before sharing the implementation.
In `@grovedb/src/replication.rs`:
- Around line 88-96: Update the Notes section of the public fetch_chunk
documentation to describe append-only subtrees: they serve cursor-based entry
pages and reject requests that lack a page cursor, rather than opening Merk
trees or returning empty vectors for empty trees. Keep the existing notes for
non-append-only chunks unchanged.
In `@grovedb/src/replication/state_sync_session.rs`:
- Around line 541-550: Update the SAFETY comment above transaction_ref in
apply_chunk to document that set_new_transaction may replace and commit/drop
self.transaction, and that transaction_ref must not be used after the loop’s
final use before that call. Preserve the existing lifetime rationale while
explicitly requiring all transaction_ref accesses to remain before
set_new_transaction.
In `@grovedb/src/tests/replication_session_tests.rs`:
- Around line 1354-1367: Update the replication test around the mmr_tree_append
loop so it cannot silently stop exercising multi-page transfers when
MAX_PAGE_BYTES changes. Size the appended payloads from MAX_PAGE_BYTES to
guarantee the total exceeds one page, or count applied chunks and assert that
more than one page was used; expose MAX_PAGE_BYTES as pub(crate) if needed.
- Around line 1423-1468: Consolidate the duplicated chunk fetch/apply logic from
sync_source_to_destination, try_sync_source_to_destination, the current helper,
and state_sync_non_merk_trees_with_batch_size_one into one shared sync driver.
Parameterize it for batch size, error-handling behavior, and an optional
page-mutation hook, preserving each caller’s existing semantics; route all four
call sites through the driver so fetch, apply, and queue-extension behavior has
one implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c2f0c5f-89dd-418e-bc11-aa3ee8202739
📒 Files selected for processing (5)
grovedb/src/lib.rsgrovedb/src/replication.rsgrovedb/src/replication/non_merk_sync.rsgrovedb/src/replication/state_sync_session.rsgrovedb/src/tests/replication_session_tests.rs
- tighten the frontier-tamper assertion to the specific frontier rejection errors instead of a broad "cannot" substring - document the append-only page-serving contract in fetch_chunk's notes - extend the apply_chunk SAFETY comment with the set_new_transaction invariant: transaction_ref must not be used after the per-chunk loop - size the multi-page MMR test payload from MAX_PAGE_BYTES so the multi-page path cannot silently stop being covered if the budget is raised - consolidate the four near-identical sync-driver loops in the tests into one run_sync(source, version, batch_size, mutator) driver Deliberately NOT applied: deriving compute_non_merk_child_hash from the strict compute_non_merk_state_root. The two differ on empty trees for a reason — the lenient variant returns the actual (possibly non-null) inner Merk root so verify_grovedb still flags a corrupt DB where a count-0 append-only tree has stray Merk nodes; the strict variant's NULL_HASH would mask exactly that corruption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This is Claude. Addressed the CodeRabbit review in df05f98:
|
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Found two resource-safety issues in the new non-Merk state-sync path. The functional round-trip and tamper tests pass, but both peer-facing boundaries need additional validation before this is safe against malformed requests/responses.
| ))); | ||
| } | ||
| }; | ||
| let mut sections = unpack_nested_bytes(packed)?; |
There was a problem hiding this comment.
[P1] Enforce the page-entry cap on receipt
MAX_PAGE_ENTRIES is enforced only by the honest sender loops. This decoder trusts the packed section count and allocates before applying any protocol limit; apply_page then replays every decoded entry. A Byzantine snapshot source can send a few megabytes containing roughly a million zero-length entries, causing large metadata allocations, hashes, and transactional writes before the final root mismatch rejects the subtree. Reject counts above MAX_PAGE_ENTRIES + 1 before unpack_nested_bytes, and enforce a receiver-side byte/work budget.
| && bytes < MAX_PAGE_BYTES | ||
| { | ||
| let node = store_ref | ||
| .element_at_position(leaf_to_pos(leaf)) |
There was a problem hiding this comment.
[P2] Validate peer-controlled MMR cursor arithmetic
Both id.state and id.start come from the requesting peer. With state = u64::MAX, mmr_size_to_leaf_count returns 2^63; start = 2^63 - 1 enters this loop and leaf_to_pos panics in debug builds with integer overflow. Release builds wrap to an unrelated position. Validate that the MMR size is canonical and that the leaf index is within the helper's arithmetic range—or derive the metadata from the source's authenticated element—before converting it.
1 participant
Implements both phases proposed in #785: state sync previously could not transfer the non-Merk append-only tree family at all — a single populated
CommitmentTree(the live shielded-pool notes tree),MmrTree,BulkAppendTree, orDenseAppendOnlyFixedSizeTreemade every snapshot from the holding node unusable, with the source'sfetch_chunkdying on an opaqueCorruptedData("... cannot create chunk producer for empty Merk").Commits
NotSupportedon both the target-side discovery and source-sidefetch_chunkpaths, mirroring the indexed-tree guards from State sync does not support indexed trees (PCIT/PSIT/PCPSIT) #778.Phase 1 design: target-driven entry replay
(start, state, param)page cursor (17 bytes) into every local chunk id it requests. The source never reconstructs tree geometry from its raw namespace, and the global chunk id format is unchanged.get_chunk_value/get_buffer_value, MMRelement_at_position, denseget), with a 1 MiB / 8192-entry page budget.BulkAppendTree::append,MMR::push,DenseFixedSizedMerkleTree::insert), so every internal node, chunk blob, and cached hash on the target is locally derived from the wire entries — there is no raw copy of internal state that verification could miss.GroveDb::compute_non_merk_state_root(same dispatchverify_grovedbuses, but error-propagating instead of falling back), and requirescombine_hash(value_hash(element_bytes), state_root)to equal the element value hash bound into the restored parent Merk (the fix(grovedb): bind terminal non-Merk tree element bytes to the parent value_hash #782 terminal binding). Any tampering with wire bytes — a flipped entry byte, a stripped or altered frontier, a dropped entry — fails the sync before commit.Protocol compatibility
CURRENT_STATE_SYNC_VERSIONstays 1. Mixed old/new peers fail safe with no silent corruption in either direction: an old target requesting an append-only subtree cursor-lessly gets a descriptiveNotSupportedfrom a new source; a new target syncing from an old source fails when the old source cannot serve pages. Replication is node-local wire behavior — no committed hashes change — so noGroveVersiongating (consistent with the #778 guards).Tests
subtrees_batch_size = 1across CT/MMR/dense)grovedbsuite: 2553 passed, clippy cleanCloses #785. Cross-refs: #778 (indexed trees — same choke point, still guarded), #783 / #784 (future types 16/15 should plug into this same path).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests