perf(commitment-tree): batched append_many_raw — replace _without_frontier API#751
Merged
Merged
Conversation
Add `test-seeding`-gated `append_raw_without_frontier` and `append_many_without_frontier` on `CommitmentTree`. These populate the underlying BulkAppendTree at blake3 speed WITHOUT updating the Sinsemilla frontier, so a devnet shielded pool can be pre-loaded with a large N of filler notes without paying the per-note Pallas/Sinsemilla hashing (or the full Drive insert path). A frontier-less seeded tree has no valid Orchard anchor (so seeded notes aren't spendable), but BulkAppendTree chunk proofs — authenticated by the blake3 bulk state root, not the frontier — still verify, which is exactly what client wallet sync exercises. Under `test-seeding`, `CommitmentTree::open` tolerates the empty-frontier/populated-bulk shape so seeded state round-trips; production behavior is unchanged when the feature is off. The grovedb crate forwards this via `commitment_tree_test_seeding`. For devnet/benchmark seeding only — never enable in production. Co-Authored-By: Claude Opus 4.7 (1M context) <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:
📝 WalkthroughWalkthroughImplements deferred-root batch appends: adds frontier APIs to defer per-leaf root work, caches BulkAppendTree MMR roots for cheaper batched inserts, exposes CommitmentTree::append_many_raw to bulk-append validated entries, adds a RocksDB-backed seeding benchmark, and includes tests verifying correctness and spend-usability. ChangesBatched Bulk Append Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 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.
Actionable comments posted: 2
🤖 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-commitment-tree/src/commitment_tree/mod.rs`:
- Around line 209-217: Replace the manual early-return branch that checks
frontier_less_seed and the other similar branches with the repo-standard macros
and error-wrapping: instead of returning
Err(CommitmentTreeError::InvalidData(...)).wrap_with_cost(cost), use the
cost_return_on_error! macro to perform the early return with cost accumulation,
and convert underlying errors to the standardized contextual form using
.map_err(|e| CommitmentTreeError::CorruptedData(format!("...: {}", e))) (or
InvalidData where appropriate) before returning; specifically update the
frontier_less_seed check in commitment_tree::mod.rs (the branch that compares
frontier_size and total_count and constructs CommitmentTreeError::InvalidData)
and the other reported blocks (around the code near the markers 527-531,
542-547, 590-591, 603-604, 611-616) to use cost_return_on_error! and the
.map_err(|e| ...) wrapper pattern instead of manual early returns and
.wrap_with_cost(cost).
- Around line 514-519: The append_raw_without_frontier path allows advancing
bulk state while the in-memory frontier is non-empty, which can produce a
frontier_size mismatch on open; update the guard in append_raw_without_frontier
(and the duplicate logic around the other occurrence at the 584–598 block) to
check that the current frontier is empty before proceeding and return an
appropriate CommitmentTreeError (or CostResult error) if the frontier is
non-empty; specifically, locate the method append_raw_without_frontier and the
similar block at 584–598, add a precondition like “if frontier_size != 0 {
return Err(...) }” (using the crate’s existing error variant) so frontier-less
appends are only allowed when frontier is empty.
🪄 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
Run ID: 3c8da1d3-260e-47ba-a96e-98768e1c9114
📒 Files selected for processing (5)
grovedb-commitment-tree/Cargo.tomlgrovedb-commitment-tree/src/commitment_tree/mod.rsgrovedb-commitment-tree/src/commitment_tree/tests.rsgrovedb-commitment-tree/src/lib.rsgrovedb/Cargo.toml
Comment on lines
+209
to
+217
| let frontier_less_seed = | ||
| cfg!(feature = "test-seeding") && frontier_size == 0 && total_count > 0; | ||
| if !frontier_less_seed { | ||
| return Err(CommitmentTreeError::InvalidData(format!( | ||
| "frontier tree_size ({}) != bulk tree total_count ({})", | ||
| frontier_size, total_count | ||
| ))) | ||
| .wrap_with_cost(cost); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use repo-standard cost/error patterns in the new branches.
These added branches use manual early returns and manual wrapping. Please switch to cost_return_on_error! for cost-accumulating early returns and the standard contextual map_err wrapper pattern for error conversion.
As per coding guidelines **/*.rs: Use cost_return_on_error! macro for early returns with cost accumulation in Rust source files, and wrap errors with context using .map_err(|e| Error::CorruptedData(format!("context: {}", e))) pattern in Rust source files.
Also applies to: 527-531, 542-547, 590-591, 603-604, 611-616
🤖 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-commitment-tree/src/commitment_tree/mod.rs` around lines 209 - 217,
Replace the manual early-return branch that checks frontier_less_seed and the
other similar branches with the repo-standard macros and error-wrapping: instead
of returning Err(CommitmentTreeError::InvalidData(...)).wrap_with_cost(cost),
use the cost_return_on_error! macro to perform the early return with cost
accumulation, and convert underlying errors to the standardized contextual form
using .map_err(|e| CommitmentTreeError::CorruptedData(format!("...: {}", e)))
(or InvalidData where appropriate) before returning; specifically update the
frontier_less_seed check in commitment_tree::mod.rs (the branch that compares
frontier_size and total_count and constructs CommitmentTreeError::InvalidData)
and the other reported blocks (around the code near the markers 527-531,
542-547, 590-591, 603-604, 611-616) to use cost_return_on_error! and the
.map_err(|e| ...) wrapper pattern instead of manual early returns and
.wrap_with_cost(cost).
Member
Author
There was a problem hiding this comment.
This is Claude. Respectfully declining this one, for three reasons:
- Local consistency.
cost_return_on_error!is not used anywhere in thegrovedb-commitment-treecrate. The new branches deliberately mirror the existingappend,append_raw, andopenmethods in this same file, which all use manualmatch/return Err(..).wrap_with_cost(cost). Switching only the new code to the macro would make the file internally inconsistent. - The macro doesn't apply to most of these branches.
cost_return_on_error!unwraps aCostResult/CostContext. The flagged branches are either a plain boolean condition (thefrontier_sizemismatch inopen) or handleBulkAppendTree::append, which returns a plainResult, not aCostResult. There's no cost-bearing result to propagate. CorruptedDatadoesn't exist inCommitmentTreeError(variants areTreeFull,InvalidData,InvalidFieldElement,InvalidPayloadSize). The new code already usesInvalidData, matching the rest of the crate.
Happy to revisit if the crate adopts the macro convention more broadly.
Contributor
There was a problem hiding this comment.
@QuantumExplorer, all three points are valid — acknowledged and withdrawn.
- The macro is absent from the crate, so applying it only to the new code would create an inconsistency rather than improve it.
- The flagged branches wrap plain
Resultvalues or boolean conditions, notCostResult/CostContext, socost_return_on_error!genuinely doesn't apply. CorruptedDatadoesn't exist inCommitmentTreeError;InvalidDatais the correct variant and the new code already uses it consistently.
The suggestion was incorrect for this crate. No changes needed.
🐇 (｡•́︿•̀｡)
✏️ Learnings added
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 751
File: grovedb-commitment-tree/src/commitment_tree/mod.rs:209-217
Timestamp: 2026-05-25T07:27:06.183Z
Learning: In the `grovedb-commitment-tree` crate (Rust), `cost_return_on_error!` is NOT used anywhere. Error handling in this crate uses manual `match` / `return Err(..).wrap_with_cost(cost)` patterns throughout (including in `CommitmentTree::append`, `append_raw`, and `open`). Do not suggest switching to `cost_return_on_error!` for this crate unless the crate itself adopts that convention more broadly.
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 751
File: grovedb-commitment-tree/src/commitment_tree/mod.rs:209-217
Timestamp: 2026-05-25T07:27:06.183Z
Learning: `CommitmentTreeError` in `grovedb-commitment-tree` has the following variants: `TreeFull`, `InvalidData`, `InvalidFieldElement`, `InvalidPayloadSize`. There is NO `CorruptedData` variant. Always use `InvalidData` for wrapping underlying errors with context in this crate.
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 751
File: grovedb-commitment-tree/src/commitment_tree/mod.rs:209-217
Timestamp: 2026-05-25T07:27:06.183Z
Learning: In the `grovedb-commitment-tree` crate, `cost_return_on_error!` does not apply to branches handling plain `Result` (not `CostResult`/`CostContext`) values, such as BulkAppendTree::append results or boolean condition checks. The macro is only appropriate for unwrapping `CostResult`/`CostContext` while propagating accumulated costs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 669
File: merk/src/proofs/query/count_offset/tests.rs:54-64
Timestamp: 2026-05-17T11:22:44.675Z
Learning: In Rust code reviews, do not flag `impl Trait` used in *argument-position* as an error when it appears inside a concrete generic type in a function parameter type, e.g. `fn foo(x: &Merk<impl StorageContext<'static>>)` / `Foo<impl Bar>`. This is valid argument-position `impl Trait` (APIT) that desugars to an anonymous generic parameter and compiles on stable Rust.
Only treat `impl Trait` as unsupported when it’s nested/used in an invalid `impl Trait`-within-`impl Trait` position (e.g. forms like `impl Fn(impl Trait)`).
Learnt from: QuantumExplorer
Repo: dashpay/grovedb PR: 670
File: grovedb/src/tests/provable_count_provable_sum_tree_tests.rs:80-86
Timestamp: 2026-05-17T14:06:21.953Z
Learning: When reviewing Rust `matches!(value, pattern)` usages, do not warn that `value` is being moved if the pattern uses only non-binding `_` discards (e.g., `SomeEnum::Variant(_, _, _, _)` or other `_`-only wildcards). In this case, `_` is a non-binding discard and will not move or borrow `value`, so `value` remains usable after the `matches!` call. A move can occur only if the pattern binds fields (e.g., `Some(x)`), especially for non-`Copy` fields. Therefore, `matches!(non_copy_value, AllWildcardPattern)` is safe and should not be flagged as a potential move.
…ntier Guard both `append_raw_without_frontier` and `append_many_without_frontier`: advancing the bulk tree while the frontier already has leaves would leave `frontier_size < total_count`, a mismatch `open` cannot tolerate (it only accepts an empty frontier), producing unreopenable state. Reject it instead. Addresses CodeRabbit review on #751. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…en() check under it Rename the seeding feature `test-seeding` -> `test-seeding-ct` (and the grovedb forwarding feature to match). Under the feature, drop the frontier/bulk consistency check in `CommitmentTree::open` entirely rather than tolerating only the empty-frontier case. This lets a frontier-less-seeded tree have real, frontier-tracked notes added on top via the normal `append` path and still reopen at any `(frontier_size, total_count)` pair. Production behavior is unchanged when the feature is off. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #751 +/- ##
=========================================
Coverage 91.43% 91.44%
=========================================
Files 236 236
Lines 67111 67218 +107
=========================================
+ Hits 61364 61465 +101
- Misses 5747 5753 +6
🚀 New features to boost your workflow:
|
…tch coverage Add fault-injection to the test storage mock (toggleable get/put failures) and three tests covering the previously-uncovered error branches in the frontier-less seeding methods: the wrapped bulk-append storage error, per-entry error propagation out of the bulk loop, and the empty-input state-root recomputation failure. The remaining commit_mmr flush error is annotated codecov:ignore (unreachable via the seeding API, which always flushes in-call). Raises patch coverage above the 90% gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r call `BulkAppendTree::append` called `get_mmr_root()` on every non-compaction append, and `get_mmr_root()` clones the `mmr_overlay` — which accumulates the chunk leaf nodes (each carrying a ~573 KB blob) across all compaction cycles until the session-end flush. Cloning that growing, blob-bearing overlay on every append made bulk seeding O(N^2): a 1M frontier-less seed degraded from ~2400 notes/s to sub-1100 and never finished in reasonable time. The MMR is only mutated on compaction (every `epoch_size` appends), so its root is unchanged in between. Cache it (`last_mmr_root`), refreshing only on compaction. `from_state` keeps it lazy (`None`) because a restored MMR may not be readable until the first append; the first append computes it once, exactly as before. Bulk seeding is now linear at a constant ~2435 notes/s (1M in ~6.85 min vs. 30+ min / non-terminating before). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mark Standalone (harness=false) bench that seeds N random filler notes into a real RocksDB-backed CommitmentTree via append_many_without_frontier and reports wall-clock time, splitting compute from the disk commit. Defaults to 1M; override with SEED_N. Gated on test-seeding-ct. cargo bench -p grovedb-commitment-tree --bench seeding --features test-seeding-ct Measured 1M random notes in ~6.85 min (linear ~2435 notes/s) after the MMR-root caching fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shumkov
pushed a commit
that referenced
this pull request
May 26, 2026
…ntier Guard both `append_raw_without_frontier` and `append_many_without_frontier`: advancing the bulk tree while the frontier already has leaves would leave `frontier_size < total_count`, a mismatch `open` cannot tolerate (it only accepts an empty frontier), producing unreopenable state. Reject it instead. Addresses CodeRabbit review on #751. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
changed the title
…ntier API
Per-leaf `CommitmentTree::append_raw` was slow on bulk seeds not because of
Sinsemilla-per-leaf (amortized ~1 hash via the carry chain) but because
`CommitmentFrontier::append` called `self.root_hash()` after every insert,
walking the depth-32 Sinsemilla path for ~32 hashes per leaf. For 1M leaves
that's ~32M extra hashes. The upstream `incrementalmerkletree::Frontier`
already separates append (cheap) from root (expensive); our wrapper conflated
them.
The earlier `_without_frontier` shape side-stepped this by skipping the
frontier entirely, producing a Sinsemilla anchor that reflected only a handful
of cmx — wallets reconstructing the tree locally got a root the chain had
never recorded, and spend proofs failed. This commit fixes the actual problem.
Added:
* `CommitmentFrontier::append_no_root(cmx)` — upstream `Frontier::append`,
no depth-32 walk; carry-chain cost only.
* `CommitmentFrontier::root_hash_with_cost()` — pure accessor that
attributes the deferred 32-hash depth walk for batched callers.
* `BulkAppendTree::append_no_state_root(value)` — `append` minus the
per-leaf `compute_state_root` blake3. `append` now delegates to it +
one final `compute_current_state_root`.
* `BulkAppendTree::append_many<I: IntoIterator<Item = Vec<u8>>>` — batched
bulk-tree appends, one state-root computation at the end.
* `CommitmentTree::append_many_raw<I: IntoIterator<Item = ([u8;32],[u8;32],
Vec<u8>)>>` — byte-for-byte equivalent to N × `append_raw` (same
dense-buffer, MMR, `CommitmentFrontier::serialize()`, final state and
Sinsemilla roots) but the Sinsemilla anchor and bulk state root are each
computed exactly once at the end. Flushes the MMR overlay before return.
* `compute_current_state_root` now uses the `last_mmr_root` cache when
populated (O(1) on the hot path).
Removed:
* `test-seeding-ct` Cargo feature in `grovedb-commitment-tree` and the
forwarding feature in `grovedb`.
* `append_raw_without_frontier`, `append_many_without_frontier`,
`FrontierLessAppendResult`, `BulkSeedSummary`, and their lib.rs re-exports.
* The `#[cfg(not(feature = "test-seeding-ct"))]` gating around the
frontier/bulk consistency check in `CommitmentTree::open` — the check is
now unconditional.
* Fault-injection mock state and storage-fault tests that only existed to
cover the deleted branches.
Tests:
* `append_many_raw_byte_for_byte_matches_per_leaf` — for N ∈ {0,1,2,3,100,
2048,10_000}, the per-leaf and batched paths produce identical
`frontier.root_hash()`, `CommitmentFrontier::serialize()`, bulk state root,
and `total_count`.
* `append_many_raw_anchor_is_spend_usable` — independent Sinsemilla
auth-path recomputation; verifies `orchard::MerklePath::from_parts(pos,
path).root(cmx) == ct.anchor()`.
* `append_no_root_cost_omits_per_leaf_depth_walk` — confirms the savings
are exactly the per-leaf depth walks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-commitment-tree/benches/seeding.rs`:
- Around line 12-13: Update the example cargo bench commands in the seeding
benchmark header so they enable the feature-gated benchmark by adding --features
server to the invocations; specifically modify the two commented example
commands in the seeding.rs file header (the cargo bench examples) so both
instances include --features server and thus run the real benchmark path rather
than the fallback.
🪄 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
Run ID: 9d5d6fdb-2689-4c5e-98b2-5d29ac40f9ac
📒 Files selected for processing (7)
grovedb-bulk-append-tree/src/tree/append.rsgrovedb-bulk-append-tree/src/tree/mod.rsgrovedb-commitment-tree/Cargo.tomlgrovedb-commitment-tree/benches/seeding.rsgrovedb-commitment-tree/src/commitment_frontier/mod.rsgrovedb-commitment-tree/src/commitment_tree/mod.rsgrovedb-commitment-tree/src/commitment_tree/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- grovedb-bulk-append-tree/src/tree/append.rs
… commands The bench is gated by required-features = ["server"], so the example invocations in the file-level docs need --features server or they run the fallback `main` and skip the actual benchmark. Addresses CodeRabbit review on #751. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shumkov
pushed a commit
that referenced
this pull request
May 28, 2026
… commands The bench is gated by required-features = ["server"], so the example invocations in the file-level docs need --features server or they run the fallback `main` and skip the actual benchmark. Addresses CodeRabbit review on #751. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GroveDB's `StorageContext::get` reads straight from the transaction and never consults its `StorageBatch` (see `storage/src/rocksdb_storage/storage_context/ context_tx.rs:296-310`). So as soon as `commit_mmr` drains the MMR overlay into the batch, the freshly-flushed peaks become invisible to reads until `commit_multi_context_batch` lands the batch in the tx — which only happens at the very end of a session. For chained `append_many_raw` calls on the same `CommitmentTree` (the 500k-note Drive seeder pattern in dashpay/platform#3732), the internal `commit_mmr` at the end of batch N drained the overlay; batch N+1's first compaction then read a peak via `MmrStore::element_at_position` → `ctx.get` → tx (peak not there) → `MMR::push` raised `InconsistentStore`. Make `commit_mmr` the caller's responsibility — same as `append_raw`, which also never flushes on its own. The overlay now stays alive across chained batches; the caller flushes it once, right before committing the surrounding batch. The docs spell this out and explicitly warn against mid-session `commit_mmr`. Updated the seeding bench to call `commit_mmr` explicitly before dropping `ct`. The byte-for-byte equivalence, spend-usable anchor, and cost-accounting tests are unaffected (they compare in-memory frontier/state-root values that don't depend on commit ordering). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Actionable comments posted: 0 |
Address patch-coverage failures on PR #751: * Drop unused `BulkAppendTree::append_many` + `AppendManyResult`. The user spec said "or the equivalent" and `CommitmentTree::append_many_raw` goes through `append_no_state_root` in a loop — `append_many` had no real caller and was inflating the patch as dead code. * Re-add fault-injection toggles to the test storage mock and exercise: - `append_many_raw_rejects_invalid_cmx_mid_batch` - `append_many_raw_rejects_wrong_payload_size_mid_batch` - `append_many_raw_surfaces_bulk_storage_error` - `frontier_append_no_root_rejects_invalid_cmx` - `commit_mmr_success` (drives the bulk-tree `commit_mmr` flush path from the commitment-tree side, where `append_many_raw` purposely leaves the overlay in place per #751's chained-batch fix). * Annotate genuinely unreachable error branches with `codecov:ignore`: TreeFull on both `append` and `append_no_root` (requires 2^32 leaves), the frontier-append-error branch in `append_many_raw` (cmx already pre-validated upstream), and the end-of-batch state-root error branch (the dense-tree write-through cache + in-session MMR cache mean a fault-injecting mock can't trip it from inside `append_many_raw`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shumkov
approved these changes
May 28, 2026
shumkov
added a commit
that referenced
this pull request
Jun 1, 2026
PR #751 ('perf(commitment-tree): batched append_many_raw — replace _without_frontier API') chose to inline the bulk-append loop inside CommitmentTree::append_many_raw rather than extract a BulkAppendTree-level batched helper. This branch carried our pre-#751 helper version (defining BulkAppendTree::append_many<I> and AppendManyResult), which has no callers on develop or in platform — only append_many_raw is used. Reset both files to develop's versions so the branch's net delta vs develop is only the snapshot bootstrap surface (ingest_subtree_sst, replace_commitment_tree_subtree_root, raw_storage). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Jun 1, 2026
feat(drive-abci): gate shielded-pool seeding behind
shielded_test_data feature
dashpay/platform#3774
Merged
QuantumExplorer
added a commit
that referenced
this pull request
Jun 2, 2026
* feat: expose ingest_subtree_sst + replace_commitment_tree_subtree_root Two new public methods to enable snapshot-based bootstrap of CommitmentTree subtrees without paying the per-write WAL + fsync cost of the runtime seeder path: 1. `RocksDbStorage::ingest_subtree_sst` (also re-exposed on `GroveDb`) — bulk-ingest a single SST file (produced by `SstFileWriter`) into a named column family via `IngestExternalFile`. Pinned `allow_global_seqno=false` and `snapshot_consistency=false` for safe defaults. 2. `GroveDb::replace_commitment_tree_subtree_root` — extracts the parent-Merk tail of `commitment_tree_insert` as a public method, accepting a caller- provided `new_combined_root` instead of computing it from an append. Used in conjunction with (1) to apply a precomputed shielded-pool snapshot at devnet genesis. Bootstrap pattern (for the caller, e.g. drive-abci's shielded_snapshot module): - Use (1) to ingest the subtree's underlying RocksDB state from a SST file. - Open StorageContext at the subtree path, reload CommitmentTree, compute combined_root via `compute_commitment_tree_state_root`, verify it matches the snapshot header's recorded value. - Use (2) to write the parent Merk leaf with the verified combined_root. Neither method performs cross-validation on its own; misuse will produce an inconsistent Merk tree. Intended for snapshot-based bootstrap only; normal append flow must continue to go through `commitment_tree_insert`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat: expose raw_storage() on GroveDb Escape hatch for snapshot/replication tooling that needs to use the public StorageContext API directly (raw_iter etc.) without paying the typed element-layer overhead. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(commitment-tree): frontier-less bulk seeding for sync/scale testing Add `test-seeding`-gated `append_raw_without_frontier` and `append_many_without_frontier` on `CommitmentTree`. These populate the underlying BulkAppendTree at blake3 speed WITHOUT updating the Sinsemilla frontier, so a devnet shielded pool can be pre-loaded with a large N of filler notes without paying the per-note Pallas/Sinsemilla hashing (or the full Drive insert path). A frontier-less seeded tree has no valid Orchard anchor (so seeded notes aren't spendable), but BulkAppendTree chunk proofs — authenticated by the blake3 bulk state root, not the frontier — still verify, which is exactly what client wallet sync exercises. Under `test-seeding`, `CommitmentTree::open` tolerates the empty-frontier/populated-bulk shape so seeded state round-trips; production behavior is unchanged when the feature is off. The grovedb crate forwards this via `commitment_tree_test_seeding`. For devnet/benchmark seeding only — never enable in production. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(commitment-tree): reject frontier-less seeding on a non-empty frontier Guard both `append_raw_without_frontier` and `append_many_without_frontier`: advancing the bulk tree while the frontier already has leaves would leave `frontier_size < total_count`, a mismatch `open` cannot tolerate (it only accepts an empty frontier), producing unreopenable state. Reject it instead. Addresses CodeRabbit review on #751. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(commitment-tree): rename feature to test-seeding-ct; drop open() check under it Rename the seeding feature `test-seeding` -> `test-seeding-ct` (and the grovedb forwarding feature to match). Under the feature, drop the frontier/bulk consistency check in `CommitmentTree::open` entirely rather than tolerating only the empty-frontier case. This lets a frontier-less-seeded tree have real, frontier-tracked notes added on top via the normal `append` path and still reopen at any `(frontier_size, total_count)` pair. Production behavior is unchanged when the feature is off. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(commitment-tree): cover frontier-less seeding error paths for patch coverage Add fault-injection to the test storage mock (toggleable get/put failures) and three tests covering the previously-uncovered error branches in the frontier-less seeding methods: the wrapped bulk-append storage error, per-entry error propagation out of the bulk loop, and the empty-input state-root recomputation failure. The remaining commit_mmr flush error is annotated codecov:ignore (unreachable via the seeding API, which always flushes in-call). Raises patch coverage above the 90% gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(bulk-append-tree): cache MMR root so append is O(1), not O(N) per call `BulkAppendTree::append` called `get_mmr_root()` on every non-compaction append, and `get_mmr_root()` clones the `mmr_overlay` — which accumulates the chunk leaf nodes (each carrying a ~573 KB blob) across all compaction cycles until the session-end flush. Cloning that growing, blob-bearing overlay on every append made bulk seeding O(N^2): a 1M frontier-less seed degraded from ~2400 notes/s to sub-1100 and never finished in reasonable time. The MMR is only mutated on compaction (every `epoch_size` appends), so its root is unchanged in between. Cache it (`last_mmr_root`), refreshing only on compaction. `from_state` keeps it lazy (`None`) because a restored MMR may not be readable until the first append; the first append computes it once, exactly as before. Bulk seeding is now linear at a constant ~2435 notes/s (1M in ~6.85 min vs. 30+ min / non-terminating before). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * bench(commitment-tree): add 1M frontier-less seeding throughput benchmark Standalone (harness=false) bench that seeds N random filler notes into a real RocksDB-backed CommitmentTree via append_many_without_frontier and reports wall-clock time, splitting compute from the disk commit. Defaults to 1M; override with SEED_N. Gated on test-seeding-ct. cargo bench -p grovedb-commitment-tree --bench seeding --features test-seeding-ct Measured 1M random notes in ~6.85 min (linear ~2435 notes/s) after the MMR-root caching fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(commitment-tree): batched append_many_raw — replace _without_frontier API Per-leaf `CommitmentTree::append_raw` was slow on bulk seeds not because of Sinsemilla-per-leaf (amortized ~1 hash via the carry chain) but because `CommitmentFrontier::append` called `self.root_hash()` after every insert, walking the depth-32 Sinsemilla path for ~32 hashes per leaf. For 1M leaves that's ~32M extra hashes. The upstream `incrementalmerkletree::Frontier` already separates append (cheap) from root (expensive); our wrapper conflated them. The earlier `_without_frontier` shape side-stepped this by skipping the frontier entirely, producing a Sinsemilla anchor that reflected only a handful of cmx — wallets reconstructing the tree locally got a root the chain had never recorded, and spend proofs failed. This commit fixes the actual problem. Added: * `CommitmentFrontier::append_no_root(cmx)` — upstream `Frontier::append`, no depth-32 walk; carry-chain cost only. * `CommitmentFrontier::root_hash_with_cost()` — pure accessor that attributes the deferred 32-hash depth walk for batched callers. * `BulkAppendTree::append_no_state_root(value)` — `append` minus the per-leaf `compute_state_root` blake3. `append` now delegates to it + one final `compute_current_state_root`. * `BulkAppendTree::append_many<I: IntoIterator<Item = Vec<u8>>>` — batched bulk-tree appends, one state-root computation at the end. * `CommitmentTree::append_many_raw<I: IntoIterator<Item = ([u8;32],[u8;32], Vec<u8>)>>` — byte-for-byte equivalent to N × `append_raw` (same dense-buffer, MMR, `CommitmentFrontier::serialize()`, final state and Sinsemilla roots) but the Sinsemilla anchor and bulk state root are each computed exactly once at the end. Flushes the MMR overlay before return. * `compute_current_state_root` now uses the `last_mmr_root` cache when populated (O(1) on the hot path). Removed: * `test-seeding-ct` Cargo feature in `grovedb-commitment-tree` and the forwarding feature in `grovedb`. * `append_raw_without_frontier`, `append_many_without_frontier`, `FrontierLessAppendResult`, `BulkSeedSummary`, and their lib.rs re-exports. * The `#[cfg(not(feature = "test-seeding-ct"))]` gating around the frontier/bulk consistency check in `CommitmentTree::open` — the check is now unconditional. * Fault-injection mock state and storage-fault tests that only existed to cover the deleted branches. Tests: * `append_many_raw_byte_for_byte_matches_per_leaf` — for N ∈ {0,1,2,3,100, 2048,10_000}, the per-leaf and batched paths produce identical `frontier.root_hash()`, `CommitmentFrontier::serialize()`, bulk state root, and `total_count`. * `append_many_raw_anchor_is_spend_usable` — independent Sinsemilla auth-path recomputation; verifies `orchard::MerklePath::from_parts(pos, path).root(cmx) == ct.anchor()`. * `append_no_root_cost_omits_per_leaf_depth_walk` — confirms the savings are exactly the per-leaf depth walks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(commitment-tree): add --features server to seeding bench example commands The bench is gated by required-features = ["server"], so the example invocations in the file-level docs need --features server or they run the fallback `main` and skip the actual benchmark. Addresses CodeRabbit review on #751. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(commitment-tree): don't commit_mmr inside append_many_raw GroveDB's `StorageContext::get` reads straight from the transaction and never consults its `StorageBatch` (see `storage/src/rocksdb_storage/storage_context/ context_tx.rs:296-310`). So as soon as `commit_mmr` drains the MMR overlay into the batch, the freshly-flushed peaks become invisible to reads until `commit_multi_context_batch` lands the batch in the tx — which only happens at the very end of a session. For chained `append_many_raw` calls on the same `CommitmentTree` (the 500k-note Drive seeder pattern in dashpay/platform#3732), the internal `commit_mmr` at the end of batch N drained the overlay; batch N+1's first compaction then read a peak via `MmrStore::element_at_position` → `ctx.get` → tx (peak not there) → `MMR::push` raised `InconsistentStore`. Make `commit_mmr` the caller's responsibility — same as `append_raw`, which also never flushes on its own. The overlay now stays alive across chained batches; the caller flushes it once, right before committing the surrounding batch. The docs spell this out and explicitly warn against mid-session `commit_mmr`. Updated the seeding bench to call `commit_mmr` explicitly before dropping `ct`. The byte-for-byte equivalence, spend-usable anchor, and cost-accounting tests are unaffected (they compare in-memory frontier/state-root values that don't depend on commit ordering). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: cargo fmt --all Three minor wrap/line-length fixes flagged by CI after the develop merge: - grovedb-bulk-append-tree/src/tree/append.rs: rejoin one-line import - grovedb/src/operations/commitment_tree.rs: collapse one-line let - storage/src/rocksdb_storage/storage.rs: collapse multi-line fn signature Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: drop vestigial BulkAppendTree::append_many helper PR #751 ('perf(commitment-tree): batched append_many_raw — replace _without_frontier API') chose to inline the bulk-append loop inside CommitmentTree::append_many_raw rather than extract a BulkAppendTree-level batched helper. This branch carried our pre-#751 helper version (defining BulkAppendTree::append_many<I> and AppendManyResult), which has no callers on develop or in platform — only append_many_raw is used. Reset both files to develop's versions so the branch's net delta vs develop is only the snapshot bootstrap surface (ingest_subtree_sst, replace_commitment_tree_subtree_root, raw_storage). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat: gate snapshot-bootstrap surface behind `unsafe-dump-load` feature Three escape-hatch APIs added for caller-driven subtree dump/restore are now Cargo-feature-gated so production builds don't carry any of them: * `GroveDb::raw_storage()` — borrow underlying RocksDbStorage * `RocksDbStorage::ingest_subtree_sst()` — bulk-load SST into a CF * `GroveDb::replace_subtree_root()` — caller-provided child hash Also generalizes the third API. The previous `replace_commitment_tree_subtree_root` took CommitmentTree-specific arguments (total_count, chunk_power, flags) and constructed the Element internally. The same Merk-tail plumbing is identical across all non-Merk tree types (commitment / mmr / bulk-append / dense), so the new shape just takes an arbitrary `Element` plus the caller-computed combined root: pub fn replace_subtree_root<'b, B, P>( &self, path: P, key: &[u8], new_element: Element, new_combined_root: [u8; 32], transaction: TransactionArg, grove_version: &GroveVersion, ) -> CostResult<(), Error> The body cheap-checks that the Element is a tree variant (rejecting Item/Reference, which have no child-hash slot) but leaves hash-vs-state correctness to the caller — same contract as before, just no longer commitment-tree-specific. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style: cargo fmt (cfg-gated import order) `#[cfg(feature = "unsafe-dump-load")] use rocksdb::IngestExternalFileOptions` should precede the unconditional `use rocksdb::{...}` block per rustfmt's sort rules. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(grovedb): unsafe-dump-load subtree roundtrip + non-tree rejection Two new tests covering the `unsafe-dump-load`-gated surface in-tree (codecov patch coverage was 0% on the 80 new lines; CodeRabbit + codecov flagged it on PR #752). `unsafe_dump_load_subtree_roundtrip_preserves_root_hash`: Builds a CommitmentTree subtree on GroveDb A (insert 5 leaves), dumps the subtree's storage to an SST file (via `raw_storage` + `SstFileWriter` — the same path consumers will use), then on a fresh GroveDb B with only an empty CommitmentTree skeleton: ingests the SST via `ingest_subtree_sst` and patches the parent Merk leaf via `replace_subtree_root` with the combined_root recomputed from A. The two GroveDb root_hashes must match byte-for-byte. This pins the snapshot-bootstrap contract documented in `operations/replace_subtree_root.rs`: SST-ingest + caller-provided child hash is equivalent to a normal insert path, *provided* the caller's hash matches the underlying state. `replace_subtree_root_rejects_non_tree_element`: Pins the cheap guard in `replace_subtree_root`: passing an Item / Reference element (with no child-hash slot) must surface as `Error::InvalidInput` rather than silently corrupting the parent Merk. Adds `rocksdb` as a dev-dependency of `grovedb` so the SST-write side of the roundtrip can use the same QuantumExplorer rocksdb revision that `grovedb-storage` optionally depends on. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Quantum Explorer <quantum@dash.org>
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
2 participants
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
Replaces the test-only
_without_frontierAPI (the original shape of this PR) with a real batched append onCommitmentTreethat produces a correct, spend-usable Sinsemilla anchor. Motivated by dashpay/platform#3714 — seeding ~1M notes for sync/scale tests — but the runtime path also benefits: any block with multiple shielded outputs goes from32 × NSinsemilla hashes to32 + N.What was actually wrong
Per-leaf
CommitmentTree::append_rawwas slow on bulk seeds not because of Sinsemilla-per-leaf cost —Frontier::appendis amortized ~1 hash via the carry chain — but becauseCommitmentFrontier::appendcalledself.root_hash()after every insert, which walks the depth-32 Sinsemilla path for ~32 hashes per leaf. For 1M leaves that's ~32M extra hashes. The upstreamincrementalmerkletree::Frontiercleanly separates append (cheap) from root (expensive); our wrapper conflated them.The earlier
_without_frontiershape side-stepped this by skipping the frontier entirely. That made seeded chains produce a Sinsemilla anchor that reflected only the handful of cmx that did go through the frontier — wallets reconstructing the tree locally would get a root the chain had never recorded, and spend proofs would fail. This PR fixes the actual problem instead.What's added
CommitmentFrontier::append_no_root(cmx)— same upstreamFrontier::append, but no depth-32 walk. Cost accounting tracks only the carry chain.CommitmentFrontier::root_hash_with_cost()— pure accessor that attributes the 32 sinsemilla_hash_calls of the depth walk, so batched callers can recover the deferred cost at the single end-of-batch root computation.BulkAppendTree::append_no_state_root(value)— likeappendminus the per-leafcompute_state_rootblake3 hash.BulkAppendTree::append_many<I: IntoIterator<Item = Vec<u8>>>(values)— batched bulk-tree appends with one state-root computation at the end. (appendnow delegates toappend_no_state_root+ a single final root, so the two paths share their hot logic.)CommitmentTree::append_many_raw<I: IntoIterator<Item = ([u8;32], [u8;32], Vec<u8>)>>(entries)— the user-facing batched API. Byte-for-byte equivalent to N ×append_raw(same dense-buffer state, same chunk MMR, sameCommitmentFrontier::serialize(), same finalbulk_state_rootandsinsemilla_root), but the Sinsemilla anchor and bulk state root are each computed exactly once at the end.commit_mmris flushed before return; callers don't need a separate call.BulkAppendTree::last_mmr_root) is kept — it now benefits the batched path and the production single-insert path.compute_current_state_rootuses it when populated, keeping that call O(1) on the hot path.What's removed
test-seeding-ctCargo feature ingrovedb-commitment-treeand the forwarding feature of the same name ingrovedb.CommitmentTree::append_raw_without_frontier/append_many_without_frontier,FrontierLessAppendResult,BulkSeedSummary, and their re-exports inlib.rs.#[cfg(not(feature = "test-seeding-ct"))]gating around the frontier/bulk consistency check inCommitmentTree::open— the check is now unconditional. With no way to produce an inconsistent (empty frontier / populated bulk) state, there is no exception to make._without_frontierstorage-error branches.Measured
SEED_N=1000000 cargo bench -p grovedb-commitment-tree --bench seeding --features server, on-disk RocksDB,chunk_power=11, payload 216 B, deterministic rejection-sampled valid-Pallas cmx:_without_frontier)append_many_raw)test-seeding-ct(footgun)open()consistency checkThe ~1.7 min overhead vs the unsound frontier-less path is exactly the cost of computing a correct Sinsemilla anchor over all 1M cmx (~1M carry-chain hashes + one depth-32 walk). Well under the spec's 17–25 min ceiling.
Test plan
append_many_raw_byte_for_byte_matches_per_leaf— for N ∈ {0, 1, 2, 3, 100, 2048, 10_000}, the per-leaf and batched paths produce identicalfrontier.root_hash(), identicalCommitmentFrontier::serialize(), identical bulk state root, and identicaltotal_count.append_many_raw_anchor_is_spend_usable— build a tree viaappend_many_raw, compute the orchard Merkle auth path for a known position by independent Sinsemilla recomputation, and verifyorchard::MerklePath::from_parts(pos, path).root(cmx) == ct.anchor().append_no_root_cost_omits_per_leaf_depth_walk— N × eagerappendcost == N ×append_no_root+ 1 ×root_hash_with_cost+ 32 × (N − 1), confirming the savings are exactly the per-leaf root walks the eager path was paying.cached_mmr_root_matches_recomputation_across_compactions— the cache invariant from this PR's earlier perf fix still holds across compaction cycles.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Benchmarks