feat: use transactional storage context for all specialized trees - #638
Merged
QuantumExplorer merged 9 commits intoMar 10, 2026
Merged
Conversation
Specialized trees (DenseFixedSizedMerkleTree, MMR, BulkAppendTree, CommitmentTree) previously used immediate storage context which bypassed the StorageBatch pipeline. This meant commit_batch cost tracking had no effect on these code paths. This change adds in-memory caches that provide read-after-write visibility, enabling transactional storage context where writes are deferred to a StorageBatch: - DenseFixedSizedMerkleTree: write-through cache serves reads from memory, falling back to storage for pre-session values - MMRBatch: overlay persistence across compaction cycles so cross-epoch MMR reads find nodes pushed in previous compactions without storage - BulkAppendTree/CommitmentTree: MMR overlay held in-memory during session, flushed via commit_mmr() at session end All GroveDB operations (dense_tree, mmr_tree, bulk_append_tree, commitment_tree) now use get_transactional_storage_context + commit_multi_context_batch, matching the pattern used by Merk trees. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an in-memory MMR overlay and overlay-aware MMR constructors, threads transactional StorageBatch contexts through many tree/MMR operations for read-after-write visibility, exposes commit_mmr() to flush overlays, and adds a write-through cache to DenseFixedSizedMerkleTree. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BulkAppendTree
participant MMR as MMR (with overlay)
participant DataBatch
participant Storage
Client->>BulkAppendTree: append/push
BulkAppendTree->>MMR: MMR::new_with_overlay(mmr_size, store, overlay)
MMR->>MMR: batch preloads overlay in-memory
BulkAppendTree->>MMR: batch.element_at_position(pos)
MMR-->>BulkAppendTree: node (overlay or store)
BulkAppendTree->>MMR: batch.push/update (buffers node in overlay)
BulkAppendTree->>BulkAppendTree: take_overlay (store locally)
Client->>BulkAppendTree: commit_mmr()
BulkAppendTree->>DataBatch: create StorageBatch
BulkAppendTree->>MMR: MMR::new_with_overlay(..., taken_overlay)
BulkAppendTree->>MMR: batch.commit() -> write buffered nodes into DataBatch
MMR->>DataBatch: write nodes into batch
DataBatch->>Storage: commit_multi_context_batch
Storage-->>DataBatch: ack
BulkAppendTree->>BulkAppendTree: clear overlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Aligns with Merk which always uses transactional storage context for both reads and writes. The remaining get_immediate_storage_context calls in grovedb/src/operations/ are now zero — all specialized tree operations (dense tree, MMR, bulk append, commitment tree, and proof generation) consistently use get_transactional_storage_context. Read-only operations pass None as the batch parameter since they don't write. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grovedb-dense-fixed-sized-merkle-tree/src/tree.rs (1)
36-52:⚠️ Potential issue | 🟡 MinorAddress pipeline warning: field
cacheis never read.The pipeline reports that
cacheis never read. This occurs becauseget_value()and other cache-reading methods are behind#[cfg(feature = "storage")], but the field itself is unconditional. When compiled without thestoragefeature, the field exists but is unused.Consider one of:
- Gate the field with
#[cfg(feature = "storage")]and use a unit type placeholder otherwise- Add
#[allow(dead_code)]if non-storage builds are uncommonOption 1: Feature-gate the field
pub struct DenseFixedSizedMerkleTree<S> { height: u8, count: u16, /// The underlying storage context. pub storage: S, + #[cfg(feature = "storage")] /// Write-through cache: holds values written in this session. /// Indexed by position. `None` means the value has not been written /// in this session (fall back to storage). cache: Vec<Option<Vec<u8>>>, }Note: This would require adjusting constructors and accessors with matching
#[cfg]blocks.Option 2: Suppress warning
+ #[cfg_attr(not(feature = "storage"), allow(dead_code))] /// Write-through cache: holds values written in this session. /// Indexed by position. `None` means the value has not been written /// in this session (fall back to storage). cache: Vec<Option<Vec<u8>>>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-dense-fixed-sized-merkle-tree/src/tree.rs` around lines 36 - 52, Field `cache` on DenseFixedSizedMerkleTree is unused in non-storage builds causing a dead-code warning; gate the field with #[cfg(feature = "storage")] (and provide a #[cfg(not(feature = "storage"))] placeholder like () if needed) and update all places that construct or access it (e.g., the struct DenseFixedSizedMerkleTree definition, its constructors/initializers and methods that touch `cache` such as get_value and insert) so the code compiles in both feature sets; alternatively, if you prefer minimal change, add #[allow(dead_code)] to the `cache` field but prefer the feature-gate approach and mirror cfg annotations on constructors/accessors that read/write `cache`.
🧹 Nitpick comments (2)
grovedb-commitment-tree/src/commitment_tree/mod.rs (1)
369-377: Consider returningCostResultfor cost tracking consistency.Most mutating methods in
CommitmentTree(e.g.,save,open,append_raw) returnCostResultfor fee estimation. This method returns a plainResult, which means storage costs from the underlying MMR commit won't be tracked.If the underlying
BulkAppendTree::commit_mmrperforms storage operations, consider returningCostResult<(), CommitmentTreeError>to maintain consistent cost accumulation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-commitment-tree/src/commitment_tree/mod.rs` around lines 369 - 377, commit_mmr currently returns Result<(), CommitmentTreeError> causing MMR storage costs to be omitted; change it to return CostResult<(), CommitmentTreeError> and propagate cost from the underlying BulkAppendTree::commit_mmr. Update the signature of CommitmentTree::commit_mmr to CostResult<(), CommitmentTreeError>, call self.bulk_tree.commit_mmr() and map its output into the CostResult (propagating any cost accumulation and mapping errors into CommitmentTreeError as before), and ensure any callers of CommitmentTree::commit_mmr are adjusted to handle a CostResult return type.grovedb/src/operations/bulk_append_tree.rs (1)
156-158: Verify return value is intentional: returning oldtotal_countas position.The function returns
total_count(captured at line 60, before the append) as the global position. This is semantically correct since the 0-based position of a newly appended item equals the previous count. However, theAppendResultfrom line 86 already containsglobal_positionwhich would be clearer to use.Consider using
result.global_positionfor clarity:♻️ Optional: use result.global_position for clarity
+ let global_position = result.global_position; + let new_state_root = result.state_root; let new_total_count = tree.total_count; // ... rest of function ... tx.commit_local() - .map(|()| (new_state_root, total_count)) + .map(|()| (new_state_root, global_position)) .wrap_with_cost(cost)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/bulk_append_tree.rs` around lines 156 - 158, The code currently returns the captured pre-append total_count as the new item's global position when calling tx.commit_local(). Replace that with the AppendResult's global_position to make the return value explicit and clearer: use the result.global_position from the AppendResult produced earlier (instead of total_count) when mapping the tx.commit_local() result, keeping the returned tuple shape (global_position, total_count) semantics intact and referencing the AppendResult variable and tx.commit_local() call in bulk_append_tree.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb-bulk-append-tree/src/tree/append.rs`:
- Around line 191-205: The commit_mmr function currently calls
mmr.commit().unwrap().map_err(...) which discards the CostResult cost component;
update commit_mmr to either return CostResult<(), BulkAppendError> and replace
the unwrap/map_err with the cost_return_on_error! macro when handling
mmr.commit() (use symbols: commit_mmr, mmr.commit(), CostResult,
cost_return_on_error!, BulkAppendError, MMR) so costs are propagated per
guidelines, or if discarding cost is intentional add a concise comment above
commit_mmr explaining why cost tracking is intentionally omitted at this
boundary.
---
Outside diff comments:
In `@grovedb-dense-fixed-sized-merkle-tree/src/tree.rs`:
- Around line 36-52: Field `cache` on DenseFixedSizedMerkleTree is unused in
non-storage builds causing a dead-code warning; gate the field with
#[cfg(feature = "storage")] (and provide a #[cfg(not(feature = "storage"))]
placeholder like () if needed) and update all places that construct or access it
(e.g., the struct DenseFixedSizedMerkleTree definition, its
constructors/initializers and methods that touch `cache` such as get_value and
insert) so the code compiles in both feature sets; alternatively, if you prefer
minimal change, add #[allow(dead_code)] to the `cache` field but prefer the
feature-gate approach and mirror cfg annotations on constructors/accessors that
read/write `cache`.
---
Nitpick comments:
In `@grovedb-commitment-tree/src/commitment_tree/mod.rs`:
- Around line 369-377: commit_mmr currently returns Result<(),
CommitmentTreeError> causing MMR storage costs to be omitted; change it to
return CostResult<(), CommitmentTreeError> and propagate cost from the
underlying BulkAppendTree::commit_mmr. Update the signature of
CommitmentTree::commit_mmr to CostResult<(), CommitmentTreeError>, call
self.bulk_tree.commit_mmr() and map its output into the CostResult (propagating
any cost accumulation and mapping errors into CommitmentTreeError as before),
and ensure any callers of CommitmentTree::commit_mmr are adjusted to handle a
CostResult return type.
In `@grovedb/src/operations/bulk_append_tree.rs`:
- Around line 156-158: The code currently returns the captured pre-append
total_count as the new item's global position when calling tx.commit_local().
Replace that with the AppendResult's global_position to make the return value
explicit and clearer: use the result.global_position from the AppendResult
produced earlier (instead of total_count) when mapping the tx.commit_local()
result, keeping the returned tuple shape (global_position, total_count)
semantics intact and referencing the AppendResult variable and tx.commit_local()
call in bulk_append_tree.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31da469d-095d-409f-9e39-109a3cb3033d
📒 Files selected for processing (13)
grovedb-bulk-append-tree/src/proof/mod.rsgrovedb-bulk-append-tree/src/tree/append.rsgrovedb-bulk-append-tree/src/tree/fetch.rsgrovedb-bulk-append-tree/src/tree/mod.rsgrovedb-commitment-tree/src/commitment_tree/mod.rsgrovedb-dense-fixed-sized-merkle-tree/src/tests.rsgrovedb-dense-fixed-sized-merkle-tree/src/tree.rsgrovedb-merkle-mountain-range/src/mmr.rsgrovedb-merkle-mountain-range/src/mmr_store.rsgrovedb/src/operations/bulk_append_tree.rsgrovedb/src/operations/commitment_tree.rsgrovedb/src/operations/dense_tree.rsgrovedb/src/operations/mmr_tree.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #638 +/- ##
===========================================
- Coverage 90.84% 90.83% -0.02%
===========================================
Files 182 182
Lines 51781 51898 +117
===========================================
+ Hits 47043 47142 +99
- Misses 4738 4756 +18
🚀 New features to boost your workflow:
|
- compact_with_value: if mmr.push() or mmr.get_root() fails after std::mem::take drains the overlay, restore it before returning the error. Previously the overlay data from all prior compaction cycles would be permanently lost. - DenseFixedSizedMerkleTree insert/try_insert: clear the cache slot when rolling back count after a compute_root_hash failure. Prevents a stale cache entry from diverging from the logical tree state. - put_value: add debug_assert! that position < cache capacity. - commit_mmr: add doc comment explaining why cost tracking is intentionally omitted at this boundary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grovedb/src/operations/commitment_tree.rs (1)
516-547:⚠️ Potential issue | 🔴 CriticalUse the combined commitment-tree root in the replacement op.
Line 542 stores
bulk_state_rootdirectly, but the non-batch path computescompute_commitment_tree_state_root(sinsemilla_root, bulk_state_root)before updating Merk. BatchedCommitmentTreeInsertops will therefore propagate a different child hash thancommitment_tree_insert_raw, which breaks the frontier/anchor binding for preprocessed inserts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/commitment_tree.rs` around lines 516 - 547, The replacement op currently uses bulk_state_root directly; instead compute and use the combined commitment-tree root via compute_commitment_tree_state_root(sinsemilla_root, bulk_state_root) so the batched path produces the same child hash as commitment_tree_insert_raw and non-batch updates; update the value assigned to the GroveOp::ReplaceNonMerkTreeRoot hash (the replacement variable) to use that combined root (ensuring sinsemilla_root is the same upstream variable used in the non-batched flow).
🧹 Nitpick comments (3)
grovedb/src/operations/dense_tree.rs (1)
96-102: Add context to the new data-batch commits.These new
commit_multi_context_batchcalls return bare converted errors, so we lose whether the failure came from the insert path or the preprocessing path. Wrap each commit with phase-specific context before converting it.As per coding guidelines, "Wrap errors with context using
.map_err(|e| Error::CorruptedData(format!("context: {}", e)))pattern in Rust source files"Also applies to: 437-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/dense_tree.rs` around lines 96 - 102, The commit_multi_context_batch calls (e.g., the call using &mut cost, self.db.commit_multi_context_batch(data_batch, Some(tx.as_ref())) ) are mapping errors directly into a bare conversion and losing phase context; change each to wrap the error with phase-specific context before conversion using the pattern .map_err(|e| Error::CorruptedData(format!("commit after <phase>: {}", e))).map_err(Into::into) so the log indicates whether the failure occurred during the insert path or the preprocessing path; apply this same wrapping to the other commit_multi_context_batch invocation around lines 437-444 as well.grovedb/src/operations/mmr_tree.rs (1)
113-119: Add context to the new data-batch commits.Both new
commit_multi_context_batchcalls useInto::into, so a storage failure no longer tells us whether the append path or the preprocess path failed. Please wrap each commit with phase-specific context before converting it.As per coding guidelines, "Wrap errors with context using
.map_err(|e| Error::CorruptedData(format!("context: {}", e)))pattern in Rust source files"Also applies to: 465-471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/mmr_tree.rs` around lines 113 - 119, The commit_multi_context_batch calls currently use .map_err(Into::into) so storage errors lose phase info; update each call wrapped in cost_return_on_error to map the error into a CorruptedData with phase context (e.g., replace .map_err(Into::into) with .map_err(|e| Error::CorruptedData(format!("commit append batch failed: {}", e))) for the append path, and similarly use "commit preprocess batch failed" for the preprocess path), keeping the call on self.db.commit_multi_context_batch and preserving cost_return_on_error usage.grovedb/src/operations/bulk_append_tree.rs (1)
93-105: Keep the new flush/commit failures phase-specific.Line 94 and Lines 100-104, plus the preprocessing twin at Lines 540-550, now collapse distinct failure points into generic converted errors. Since bulk append has separate MMR-flush and data-batch-commit phases now, wrap each failure with that phase name before converting it.
As per coding guidelines, "Wrap errors with context using
.map_err(|e| Error::CorruptedData(format!("context: {}", e)))pattern in Rust source files"Also applies to: 539-551
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/bulk_append_tree.rs` around lines 93 - 105, The two failure points collapse distinct phases into generic errors; update the error mapping so each phase is wrapped with phase-specific context before converting: for the MMR flush (tree.commit_mmr()) map the error with .map_err(|e| Error::CorruptedData(format!("MMR flush failed: {}", e))).then pass that into map_bulk_err (or incorporate map_bulk_err if it expects the same type), and for the data batch commit (self.db.commit_multi_context_batch(...)) map the error with .map_err(|e| Error::CorruptedData(format!("Data batch commit failed: {}", e))).then convert to the target error type (e.g., .map_err(Into::into)); similarly apply the same wrapping at the preprocessing twin around the same calls (lines ~539-551) so each phase reports its own context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@grovedb/src/operations/commitment_tree.rs`:
- Around line 516-547: The replacement op currently uses bulk_state_root
directly; instead compute and use the combined commitment-tree root via
compute_commitment_tree_state_root(sinsemilla_root, bulk_state_root) so the
batched path produces the same child hash as commitment_tree_insert_raw and
non-batch updates; update the value assigned to the
GroveOp::ReplaceNonMerkTreeRoot hash (the replacement variable) to use that
combined root (ensuring sinsemilla_root is the same upstream variable used in
the non-batched flow).
---
Nitpick comments:
In `@grovedb/src/operations/bulk_append_tree.rs`:
- Around line 93-105: The two failure points collapse distinct phases into
generic errors; update the error mapping so each phase is wrapped with
phase-specific context before converting: for the MMR flush (tree.commit_mmr())
map the error with .map_err(|e| Error::CorruptedData(format!("MMR flush failed:
{}", e))).then pass that into map_bulk_err (or incorporate map_bulk_err if it
expects the same type), and for the data batch commit
(self.db.commit_multi_context_batch(...)) map the error with .map_err(|e|
Error::CorruptedData(format!("Data batch commit failed: {}", e))).then convert
to the target error type (e.g., .map_err(Into::into)); similarly apply the same
wrapping at the preprocessing twin around the same calls (lines ~539-551) so
each phase reports its own context.
In `@grovedb/src/operations/dense_tree.rs`:
- Around line 96-102: The commit_multi_context_batch calls (e.g., the call using
&mut cost, self.db.commit_multi_context_batch(data_batch, Some(tx.as_ref())) )
are mapping errors directly into a bare conversion and losing phase context;
change each to wrap the error with phase-specific context before conversion
using the pattern .map_err(|e| Error::CorruptedData(format!("commit after
<phase>: {}", e))).map_err(Into::into) so the log indicates whether the failure
occurred during the insert path or the preprocessing path; apply this same
wrapping to the other commit_multi_context_batch invocation around lines 437-444
as well.
In `@grovedb/src/operations/mmr_tree.rs`:
- Around line 113-119: The commit_multi_context_batch calls currently use
.map_err(Into::into) so storage errors lose phase info; update each call wrapped
in cost_return_on_error to map the error into a CorruptedData with phase context
(e.g., replace .map_err(Into::into) with .map_err(|e|
Error::CorruptedData(format!("commit append batch failed: {}", e))) for the
append path, and similarly use "commit preprocess batch failed" for the
preprocess path), keeping the call on self.db.commit_multi_context_batch and
preserving cost_return_on_error usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62e7b28a-3bec-4fa6-8b38-5b991061d521
📒 Files selected for processing (5)
grovedb/src/operations/bulk_append_tree.rsgrovedb/src/operations/commitment_tree.rsgrovedb/src/operations/dense_tree.rsgrovedb/src/operations/mmr_tree.rsgrovedb/src/operations/proof/generate.rs
Preprocessing functions (dense, MMR, bulk append, commitment) previously created their own StorageBatch and committed it directly to the transaction. This meant failed batches with borrowed (external) transactions would leak orphaned preprocessing data into the transaction. Now all preprocessing functions receive the shared StorageBatch from apply_batch, so writes are only committed atomically at the end. Batch discard tests switched from internal (owned) transactions to external (borrowed) transactions to verify no preprocessing data leaks into the transaction on batch failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
grovedb/src/tests/mmr_tree_tests.rs (2)
1948-1967: Assert the surviving leaf isreal_leaf.The final
leaf_count == 1assertion does not prove the failed batch left no stale overlay state behind; it only proves one leaf exists. Verifying index0or the root againstreal_leafwould actually pin the behavior this test is documenting.♻️ Suggested assertion tightening
let count = db .mmr_tree_leaf_count([b"parent"].as_ref(), b"mmr", None, grove_version) .unwrap() .expect("leaf count"); assert_eq!(count, 1, "only the second batch's leaf should be present"); + let value = db + .mmr_tree_get_value([b"parent"].as_ref(), b"mmr", 0, None, grove_version) + .unwrap() + .expect("leaf 0"); + assert_eq!(value, Some(b"real_leaf".to_vec())); + let root = db + .mmr_tree_root_hash([b"parent"].as_ref(), b"mmr", None, grove_version) + .unwrap() + .expect("root"); + assert_eq!(root, expected_mmr_root(&[b"real_leaf".to_vec()]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/mmr_tree_tests.rs` around lines 1948 - 1967, After asserting count == 1, also verify the actual surviving leaf equals b"real_leaf": call mmr_tree_leaf_by_index([b"parent"].as_ref(), b"mmr", 0, None, grove_version).unwrap().expect("leaf by index") and assert its value equals b"real_leaf". Alternatively you can compute mmr_tree_root([b"parent"].as_ref(), b"mmr", None, grove_version) and assert the root matches the expected root when the single leaf is b"real_leaf"; use mmr_tree_leaf_by_index or mmr_tree_root to pin the exact surviving leaf content.
1787-1824: Strengthen rollback verification with root/value checks.This only proves the count went back to
1. It can still miss regressions where rollback leavesleaf_0or the root hash mutated. Capture the pre-tx root and assert both the root and leaf0are unchanged after rollback.♻️ Suggested assertion tightening
let count_before = db .mmr_tree_leaf_count(EMPTY_PATH, b"mmr", None, grove_version) .unwrap() .expect("count before"); assert_eq!(count_before, 1); + let root_before = db + .mmr_tree_root_hash(EMPTY_PATH, b"mmr", None, grove_version) + .unwrap() + .expect("root before"); // Start transaction and append more let tx = db.start_transaction(); @@ let count_after = db .mmr_tree_leaf_count(EMPTY_PATH, b"mmr", None, grove_version) .unwrap() .expect("count after rollback"); assert_eq!( count_after, 1, "mmr leaf count should revert after rollback" ); + let root_after = db + .mmr_tree_root_hash(EMPTY_PATH, b"mmr", None, grove_version) + .unwrap() + .expect("root after rollback"); + assert_eq!(root_after, root_before, "root should revert after rollback"); + assert_eq!( + db.mmr_tree_get_value(EMPTY_PATH, b"mmr", 0, None, grove_version) + .unwrap() + .expect("leaf 0 after rollback"), + Some(b"leaf_0".to_vec()) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/mmr_tree_tests.rs` around lines 1787 - 1824, Before starting the transaction, call and store the MMR root and the value of leaf 0 (in addition to the existing count) and after rollback re-fetch the root and leaf 0 and assert they equal the stored pre-tx values; keep the existing use of start_transaction, mmr_tree_append, mmr_tree_leaf_count and rollback_transaction, but add calls to the MMR getters (e.g., mmr_tree_root and the leaf-getter used in this test suite) before the loop and assert equality after db.rollback_transaction(&tx) alongside the existing count assertion so both root hash and leaf_0 value are verified unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb-bulk-append-tree/src/tree/append.rs`:
- Around line 209-217: The code currently takes ownership of self.mmr_overlay
with std::mem::take before calling MMR::new_with_overlay and mmr.commit(), so if
commit fails the instance loses its staged MMR nodes; fix by not permanently
removing the overlay until commit succeeds: move the overlay into a local
variable (e.g., let overlay = std::mem::take(&mut self.mmr_overlay)), pass that
local overlay to MMR::new_with_overlay (MMR::new_with_overlay(..., overlay)),
then call mmr.commit(); on success leave self.mmr_overlay cleared, but on Err
restore the overlay into self.mmr_overlay before mapping the error to
BulkAppendError::MmrError so get_mmr_root() and retries see the original staged
nodes.
In `@grovedb-dense-fixed-sized-merkle-tree/src/tree.rs`:
- Around line 49-53: The struct's cache field is always compiled but only read
under #[cfg(feature = "storage")], causing an unused-field warning; add
#[cfg(feature = "storage")] to the cache field declaration and also gate its
initializations in the Tree::new() and Tree::from_state() constructors (or
refactor those methods) so the cache is only created when the "storage" feature
is enabled, and ensure get_value remains inside the same #[cfg(feature =
"storage")] impl block.
In `@grovedb/src/operations/commitment_tree.rs`:
- Around line 167-173: The current code calls
self.db.commit_multi_context_batch(..., Some(tx.as_ref())) via
cost_return_on_error which commits frontier/MMR/bulk writes into tx early,
risking partial commits if later steps fail; modify the direct insert path to
use the shared storage_batch used by preprocess_commitment_tree_ops so all
commitment-tree data and metadata (frontier/MMR/bulk writes) are staged together
and committed atomically with the parent Merk update and propagation.
Concretely, replace the early call to self.db.commit_multi_context_batch(...)
(and its cost_return_on_error wrapper) with logic that appends the same writes
into the existing storage_batch (the same batch object used by
preprocess_commitment_tree_ops) and defer calling commit_multi_context_batch
until the unified batch is ready to be committed with tx, ensuring
CommitmentTree updates, storage_batch, and tx are committed together.
In `@grovedb/src/operations/mmr_tree.rs`:
- Around line 113-119: The commit currently writes MMR node data into tx before
insert_subtree and propagation complete, risking orphaned data if later steps
fail; modify mmr_tree_append to gather subtree node writes and metadata into the
same storage_batch used for insert_subtree (as preprocess_mmr_tree_ops does) and
defer calling self.db.commit_multi_context_batch(tx...) until after
insert_subtree and propagation succeed so both subtree data and Element::MmrTree
size/root updates are committed atomically; locate and replace the early
commit_multi_context_batch call in mmr_tree_append and instead append those
writes into the shared storage_batch passed through insert_subtree/propagate,
then commit that single batch at the end.
---
Nitpick comments:
In `@grovedb/src/tests/mmr_tree_tests.rs`:
- Around line 1948-1967: After asserting count == 1, also verify the actual
surviving leaf equals b"real_leaf": call
mmr_tree_leaf_by_index([b"parent"].as_ref(), b"mmr", 0, None,
grove_version).unwrap().expect("leaf by index") and assert its value equals
b"real_leaf". Alternatively you can compute mmr_tree_root([b"parent"].as_ref(),
b"mmr", None, grove_version) and assert the root matches the expected root when
the single leaf is b"real_leaf"; use mmr_tree_leaf_by_index or mmr_tree_root to
pin the exact surviving leaf content.
- Around line 1787-1824: Before starting the transaction, call and store the MMR
root and the value of leaf 0 (in addition to the existing count) and after
rollback re-fetch the root and leaf 0 and assert they equal the stored pre-tx
values; keep the existing use of start_transaction, mmr_tree_append,
mmr_tree_leaf_count and rollback_transaction, but add calls to the MMR getters
(e.g., mmr_tree_root and the leaf-getter used in this test suite) before the
loop and assert equality after db.rollback_transaction(&tx) alongside the
existing count assertion so both root hash and leaf_0 value are verified
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25313a4f-33bd-4d52-b1be-cb9bf81ba6a6
📒 Files selected for processing (11)
grovedb-bulk-append-tree/src/tree/append.rsgrovedb-dense-fixed-sized-merkle-tree/src/tree.rsgrovedb/src/batch/mod.rsgrovedb/src/operations/bulk_append_tree.rsgrovedb/src/operations/commitment_tree.rsgrovedb/src/operations/dense_tree.rsgrovedb/src/operations/mmr_tree.rsgrovedb/src/tests/bulk_append_tree_tests.rsgrovedb/src/tests/commitment_tree_tests.rsgrovedb/src/tests/dense_tree_tests.rsgrovedb/src/tests/mmr_tree_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- grovedb/src/operations/dense_tree.rs
The count-based checks (dense_tree_count, mmr_tree_leaf_count, bulk_count) only read the parent element which is properly discarded by the batch. They don't detect leaked preprocessing data in the actual subtree storage. Add raw_subtree_get() test helper that reads directly from subtree storage, bypassing the parent element. Use it in dense, MMR, and bulk append batch discard tests to verify no data leaks into the transaction at position 0. Verified: all 7 dense/MMR/bulk tests now fail without the preprocessing atomicity fix (previously they passed despite data leakage). Combined with the 2 commitment tree tests that already caught the bug via anchor checks, all 9 tests properly detect the atomicity issue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- commit_mmr(): restore mmr_overlay via take_overlay() if mmr.commit() fails, matching the pattern already used in compact_with_value(). Without this, a failed commit permanently loses staged MMR nodes. - DenseFixedSizedMerkleTree: gate cache field with #[cfg(feature = "storage")] to suppress "field is never read" warning when compiled without storage feature. All cache usage is already inside the storage-gated impl block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
grovedb/src/lib.rs (1)
1217-1220: Add context before converting raw storage errors.This helper is meant to diagnose transactional leakage, so a bare
Err(e.into())makes failures much harder to interpret. Please wrap the storage error with at least the probed key (and ideally the subtree path) before returning it.♻️ Suggested change
- let result = storage_ctx.get(key).value; - match result { - Ok(opt) => Ok(opt.map(|v| v.to_vec())), - Err(e) => Err(e.into()), - } + storage_ctx + .get(key) + .value + .map(|opt| opt.map(|v| v.to_vec())) + .map_err(|e| { + Error::CorruptedData(format!( + "raw subtree get failed for key {}: {}", + hex::encode(key), + e + )) + })As per coding guidelines, "Wrap errors with context using
.map_err(|e| Error::CorruptedData(format!("context: {}", e)))pattern in Rust source files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/lib.rs` around lines 1217 - 1220, Wrap the raw storage error returned from storage_ctx.get(key).value with contextual information before converting: instead of returning Err(e.into()), map the error to include the probed key (and the current subtree/path variable if available) using the project's error-wrapping pattern (e.g. .map_err(|e| Error::CorruptedData(format!("failed to read key {:?}{}: {}", key, subtree_path_if_present, e)))) so callers can see which key/subtree caused the failure; apply this change at the site where storage_ctx.get(key).value is matched (refer to the storage_ctx.get and key symbols in this block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb/src/tests/bulk_append_tree_tests.rs`:
- Around line 1900-1906: The test is probing for leftover MMR data using an
8-byte U64 key but compaction writes 4-byte tagged keys via
MmrStore::with_key_size(..., MmrKeySize::U32) (used by BulkAppendTree
compaction), so change the probe to build the MMR lookup key using the same U32
key-size encoding (MmrKeySize::U32) and lookup path (raw_subtree_get) so the
test queries the actual key format produced by the compaction path (use the same
key-construction helper or logic that MmrStore/MmrKeySize::U32 uses) to
correctly detect leaks.
---
Nitpick comments:
In `@grovedb/src/lib.rs`:
- Around line 1217-1220: Wrap the raw storage error returned from
storage_ctx.get(key).value with contextual information before converting:
instead of returning Err(e.into()), map the error to include the probed key (and
the current subtree/path variable if available) using the project's
error-wrapping pattern (e.g. .map_err(|e| Error::CorruptedData(format!("failed
to read key {:?}{}: {}", key, subtree_path_if_present, e)))) so callers can see
which key/subtree caused the failure; apply this change at the site where
storage_ctx.get(key).value is matched (refer to the storage_ctx.get and key
symbols in this block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53818205-d34c-46b8-a8fa-85f246fc455a
📒 Files selected for processing (4)
grovedb/src/lib.rsgrovedb/src/tests/bulk_append_tree_tests.rsgrovedb/src/tests/dense_tree_tests.rsgrovedb/src/tests/mmr_tree_tests.rs
Each direct API function (dense_tree_insert, mmr_tree_append, bulk_append, commitment_tree_insert) commits subtree data to the transaction before the parent Merk element update. If the Merk update fails, subtree data is orphaned in the transaction. This is the same pattern as other direct GroveDB operations — callers are expected to rollback the transaction on error. The batch path (preprocess_*_ops) avoids this via a shared StorageBatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The doc comment incorrectly stated this function was "used by batch preprocessing". In reality, batch preprocessing (preprocess_commitment_tree_ops) performs its own inline processing with a shared StorageBatch and never calls this function. This function is only used by the direct API and apply_operations_without_batching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BulkAppendTree compaction writes MMR nodes via MmrKeySize::U32 (4-byte tagged keys), not U64 (8-byte). The test was probing with an 8-byte key that would never match, causing a false pass. Use the correct 4-byte key format: 0x8000_0000u32.to_be_bytes(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grovedb-bulk-append-tree/src/tree/append.rs (1)
188-188: Consider documenting the clone cost for large overlays.The
self.mmr_overlay.clone()is necessary becauseget_mmr_rootis a&selfmethod andMMR::new_with_overlaytakes ownership. For sessions with many compaction cycles, this clone could allocate significant memory on each call.If
get_mmr_rootis called frequently, consider caching the root hash after compaction or documenting that callers should minimize repeated calls during large sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-bulk-append-tree/src/tree/append.rs` at line 188, get_mmr_root currently calls MMR::new_with_overlay(mmr_size, &mmr_store, self.mmr_overlay.clone()), which forces cloning potentially-large overlays on every &self call; to fix, either change get_mmr_root to take self by value or &mut self so you can move or reuse the overlay, add a cached root field updated after compactions and return the cached value when valid, or document the clone cost prominently in the get_mmr_root API docs and call sites; locate uses of get_mmr_root, MMR::new_with_overlay, and self.mmr_overlay.clone to implement the chosen approach and update callers/tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@grovedb-bulk-append-tree/src/tree/append.rs`:
- Line 188: get_mmr_root currently calls MMR::new_with_overlay(mmr_size,
&mmr_store, self.mmr_overlay.clone()), which forces cloning potentially-large
overlays on every &self call; to fix, either change get_mmr_root to take self by
value or &mut self so you can move or reuse the overlay, add a cached root field
updated after compactions and return the cached value when valid, or document
the clone cost prominently in the get_mmr_root API docs and call sites; locate
uses of get_mmr_root, MMR::new_with_overlay, and self.mmr_overlay.clone to
implement the chosen approach and update callers/tests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 766ec36d-85e0-44de-8774-31869c5fb267
📒 Files selected for processing (6)
grovedb-bulk-append-tree/src/tree/append.rsgrovedb-dense-fixed-sized-merkle-tree/src/tree.rsgrovedb/src/operations/bulk_append_tree.rsgrovedb/src/operations/commitment_tree.rsgrovedb/src/operations/dense_tree.rsgrovedb/src/operations/mmr_tree.rs
QuantumExplorer
deleted the
fix/specialized-trees-use-transactional-context
branch
March 10, 2026 07:26
4 tasks
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.
Summary
StorageBatchpipeline. This meantcommit_batchcost tracking (fix: version-gate commit_batch accumulated costs #637) had no effect on these code paths.StorageBatch:commit_mmr()at session enddense_tree,mmr_tree,bulk_append_tree,commitment_tree) now useget_transactional_storage_context+commit_multi_context_batch, matching the pattern used by Merk treesPreprocessing atomicity fix
Preprocessing functions (dense, MMR, bulk append, commitment) previously created their own local
StorageBatchand committed it directly to the transaction viacommit_multi_context_batch. This meant that if a later operation in the batch failed, the preprocessing data was already written to a borrowed (external) transaction and could not be discarded — leaving orphaned subtree data.Fix: All preprocessing functions now receive the shared
StorageBatchfromapply_batch, so writes are only committed atomically at the end alongside all other batch operations. Dropping the batch discards everything, including preprocessing data.Audit fixes
mmr.push()ormmr.get_root()fails afterstd::mem::takedrains the overlay, restore it before returning the errorcompute_root_hashfailureTest plan
cargo test -p grovedb-dense-fixed-sized-merkle-tree— 168 tests passcargo test -p grovedb-merkle-mountain-range— 115 tests passcargo test -p grovedb-bulk-append-tree— 68 tests passcargo test -p grovedb-commitment-tree— 1 test passescargo test -p grovedb --lib— 1422 tests passcargo clippy -- -D warnings— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests