Skip to content

feat: subtree dump/restore primitives - #752

Merged
QuantumExplorer merged 17 commits into
developfrom
feat/snapshot-apply-public-api
Jun 2, 2026
Merged

feat: subtree dump/restore primitives#752
QuantumExplorer merged 17 commits into
developfrom
feat/snapshot-apply-public-api

Conversation

@shumkov

@shumkov shumkov commented May 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Three escape-hatch APIs for caller-driven subtree dump/restore, all gated behind a new opt-in Cargo feature unsafe-dump-load. Off by default so production builds carry none of this code.

API Where What it does
GroveDb::raw_storage() grovedb/src/lib.rs Borrow the underlying RocksDbStorage for direct use of the Storage trait (raw iter, low-level reads).
RocksDbStorage::ingest_subtree_sst(cf, sst_path) storage/src/rocksdb_storage/storage.rs Bulk-load a precomputed SST into a column family via RocksDB IngestExternalFile. Pinned allow_global_seqno=false + snapshot_consistency=false.
GroveDb::replace_subtree_root(path, key, element, combined_root, …) grovedb/src/operations/replace_subtree_root.rs Replace a subtree leaf's child hash + element data in its parent Merk with caller-provided values, without re-reading the subtree to recompute the hash.

What changed since the earlier "[DON'T MERGE]" shape

  • Merged with develop (PR perf(commitment-tree): batched append_many_raw — replace _without_frontier API #751's append_many_raw now flows in via develop; this branch's net delta is just the bootstrap surface).
  • Renamed replace_commitment_tree_subtree_rootreplace_subtree_root and made it generic — takes an Element instead of (total_count, chunk_power, flags). The parent-Merk plumbing is identical across all non-Merk tree types (CommitmentTree / Mmr / BulkAppend / Dense), so a single helper covers all of them. Callers build whichever Element variant they're restoring.
  • Vestigial BulkAppendTree::append_many helper dropped (PR perf(commitment-tree): batched append_many_raw — replace _without_frontier API #751 inlined the loop into append_many_raw directly; no callers).
  • All three APIs #[cfg(feature = "unsafe-dump-load")]-gated. storage crate exposes unsafe-dump-load = ["rocksdb_storage"]; grovedb crate exposes unsafe-dump-load = ["grovedb-storage/unsafe-dump-load"].

Safety contract (also in module-level doc)

replace_subtree_root is documented as caller-responsible:

  • Caller must ensure new_combined_root matches the underlying subtree state (typical pattern: ingest SST, reopen typed tree, recompute the post-ingest root, then call this).
  • Mismatches produce a silently-inconsistent Merk tree — the parent's recorded child hash diverges from actual subtree state.
  • Runtime check is a cheap "is this a tree variant at all" guard; reject Item/Reference. Hash-vs-state correctness is the caller's problem.

ingest_subtree_sst bypasses any open transaction. Caller must arrange transaction semantics at a higher layer (in dashpay/platform we only call it when the destination subtree is known empty at InitChain time, and rely on InitChain abort = wipe-and-restart for failure recovery).

raw_storage is documented as an escape hatch with no stability guarantee.

Use case

Devnet shielded-pool genesis snapshot bake/apply in dashpay/platform (the consumer):

  1. Bake stage runs drive-abci snapshot-bake → generates 30k–1M-note commitment-tree subtree → dumps to SST via raw_storage().raw_iter() + SstFileWriter.
  2. Runtime InitChain reads the embedded SST, ingests via ingest_subtree_sst, recomputes the combined root via compute_commitment_tree_state_root, then replace_subtree_root to patch the parent Merk leaf.

Roundtrip is verified on the consumer side: 30 000 notes → 8.6 MB SST → re-applied to a fresh platform yields a byte-equivalent Sinsemilla anchor (df37726e…4c2300).

Verification

  • cargo check -p grovedb --no-default-features --features minimal — default (feature off), clean.
  • cargo check -p grovedb --no-default-features --features "minimal,unsafe-dump-load" — feature on, clean.
  • All 1852 grovedb library tests pass.
  • CI on this branch is fully green except codecov/patch (0% coverage on the 109 new bootstrap-API lines — real gap; coverage is intentionally not raised inside grovedb because the realistic exercise of these APIs is dump+restore at the consumer level, not in-grovedb).

Stability note

These three APIs are explicitly opt-in escape hatches, not stable public surface. Misuse is silent corruption. The unsafe-dump-load feature name is the signal; the docstrings reinforce it.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Opt-in "unsafe-dump-load" feature to enable out-of-band dump/restore tooling.
    • Read-only access to underlying storage for low-level snapshot/replication workflows.
    • Bulk SST ingest to efficiently load subtree data into the storage engine.
    • Caller-driven subtree-root replacement to support snapshot-based bootstrap scenarios.
  • Tests

    • Roundtrip test verifying dump→ingest restores identical subtree root hash.
    • Validation test ensuring non-tree replacements are rejected.

shumkov and others added 2 commits May 25, 2026 17:05
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>
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>
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 62bc12da-9e3d-413f-b0a9-a5a57d8415c3

📥 Commits

Reviewing files that changed from the base of the PR and between c7f6dc3 and 62e1296.

📒 Files selected for processing (2)
  • grovedb/Cargo.toml
  • grovedb/src/tests/commitment_tree_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • grovedb/Cargo.toml

📝 Walkthrough

Walkthrough

Adds an unsafe-dump-load feature that gates storage-level SST ingestion, GroveDb escape-hatch accessors/delegation, a caller-driven subtree-root replacement operation, and feature-gated integration tests exercising a dump/ingest/replace roundtrip.

Changes

Unsafe dump-load feature implementation

Layer / File(s) Summary
Storage-layer SST ingestion foundation
storage/Cargo.toml, storage/src/rocksdb_storage/storage.rs
Adds unsafe-dump-load Cargo feature, conditionally imports IngestExternalFileOptions, and implements RocksDbStorage::ingest_subtree_sst to bulk-ingest external SST files into a named column family with allow_global_seqno=false, snapshot_consistency=false, and mapped errors.
GroveDb high-level escape-hatch APIs
grovedb/Cargo.toml, grovedb/src/lib.rs
Adds unsafe-dump-load Cargo feature and two public methods on GroveDb: raw_storage() to expose the underlying RocksDbStorage reference and ingest_subtree_sst() to delegate SST ingestion to storage.
Subtree-root replacement operation
grovedb/src/operations/mod.rs, grovedb/src/operations/replace_subtree_root.rs
Conditionally exposes replace_subtree_root module and implements GroveDb::replace_subtree_root, which validates new_element as a tree, inserts the provided child hash and element into the parent Merkle without re-reading the subtree, caches and propagates the updated Merk, commits the storage batch, and finalizes the transaction with cost tracking.
Unsafe dump-load integration tests
grovedb/src/tests/commitment_tree_tests.rs
Adds unsafe-dump-load-gated tests that dump a subtree to an SST using RocksDB APIs, ingest the SST into a fresh DB, call replace_subtree_root, and assert the restored root matches the source; includes a test rejecting non-tree elements.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibble at roots where snapshots sleep,
I carry hashes in a pocket deep,
I hop and plant a subtree's song anew,
Unsafe and swift — a bootstrap's quiet cue,
A rabbit's wink — restore true.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: subtree dump/restore primitives' directly and clearly describes the main changes introduced in the PR: new APIs enabling caller-driven subtree dump and restore operations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/snapshot-apply-public-api

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.44%. Comparing base (69d02c6) to head (62e1296).

Files with missing lines Patch % Lines
storage/src/rocksdb_storage/storage.rs 73.68% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #752   +/-   ##
========================================
  Coverage    91.44%   91.44%           
========================================
  Files          236      237    +1     
  Lines        67218    67298   +80     
========================================
+ Hits         61465    61540   +75     
- Misses        5753     5758    +5     
Components Coverage Δ
grovedb-core 88.97% <100.00%> (+0.02%) ⬆️
merk 92.26% <ø> (ø)
storage 86.20% <73.68%> (-0.17%) ⬇️
commitment-tree 96.03% <ø> (ø)
mmr 96.79% <ø> (ø)
bulk-append-tree 89.82% <ø> (ø)
element 97.38% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

QuantumExplorer and others added 12 commits May 26, 2026 08:49
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>
…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>
…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>
…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>
… 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>
…y-public-api

# Conflicts:
#	grovedb-commitment-tree/src/commitment_frontier/mod.rs
#	grovedb-commitment-tree/src/commitment_tree/mod.rs
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>
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>
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>
shumkov added a commit to dashpay/platform that referenced this pull request Jun 1, 2026
…-dump-load`

Picks up the post-refactor tip of dashpay/grovedb#752 (`2e9dff50`), which:
  1. Renames `replace_commitment_tree_subtree_root` → `replace_subtree_root`
     and makes it generic over Element variant (caller builds whichever
     non-Merk tree element they're snapshot-restoring).
  2. Gates all three snapshot-bootstrap entry points
     (`raw_storage`, `ingest_subtree_sst`, `replace_subtree_root`) behind a
     new `unsafe-dump-load` Cargo feature, so production grovedb has no
     compiled access to them.

Platform changes:
  * `drive` exposes a pass-through `unsafe-dump-load` feature that forwards
    to `grovedb/unsafe-dump-load`.
  * `drive-abci` exposes a pass-through `unsafe-dump-load` feature that
    forwards to `drive/unsafe-dump-load`.
  * `drive-abci`'s `[dev-dependencies]` always enables the feature so
    `cargo test` can run the snapshot roundtrip test.
  * Dockerfile passes `--features=unsafe-dump-load` to both
    `cargo chef cook` and `cargo build` when `SHIELDED_TEST_DATA=true`
    (composes correctly with any user-supplied `ADDITIONAL_FEATURES`).
  * Callers in `shielded_snapshot` and the snapshot-bake path now construct
    `Element::new_commitment_tree(total_count, chunk_power, flags)` locally
    and pass it to `replace_subtree_root`.

Verified locally:
  * Default build (no SDK_TEST_DATA, no SHIELDED_TEST_DATA): compiles, no
    snapshot-bootstrap APIs in the binary.
  * `SDK_TEST_DATA=true` only: compiles, no shielded code or APIs.
  * `SHIELDED_TEST_DATA=true`: compiles with feature on.
  * `snapshot_dump_apply_preserves_anchor` test (with feature on) passes —
    30k notes dumped + reapplied yields byte-equivalent anchor.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@shumkov shumkov changed the title [DON'T MERGE] feat: expose public API for shielded-pool snapshot bake/apply feat(unsafe-dump-load): subtree dump/restore primitives behind a Cargo feature Jun 2, 2026
@shumkov
shumkov marked this pull request as ready for review June 2, 2026 02:01
@shumkov
shumkov requested a review from QuantumExplorer as a code owner June 2, 2026 02:01
@shumkov shumkov changed the title feat(unsafe-dump-load): subtree dump/restore primitives behind a Cargo feature feat: subtree dump/restore primitives behind a Cargo feature Jun 2, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@storage/src/rocksdb_storage/storage.rs`:
- Around line 46-47: CI reports a rustfmt formatting violation around the
conditional import for IngestExternalFileOptions; run `cargo fmt` and adjust the
conditional import so it matches rustfmt expectations (ensure the `#[cfg(feature
= "unsafe-dump-load")]` attribute is immediately above the `use
rocksdb::IngestExternalFileOptions;` statement with correct spacing/indentation
in storage.rs, preserving the symbol name IngestExternalFileOptions and the cfg
attribute exactly as shown).
🪄 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: 7638cefe-1258-4ae5-b88e-e1fc253480ae

📥 Commits

Reviewing files that changed from the base of the PR and between 69d02c6 and 2e9dff5.

📒 Files selected for processing (6)
  • grovedb/Cargo.toml
  • grovedb/src/lib.rs
  • grovedb/src/operations/mod.rs
  • grovedb/src/operations/replace_subtree_root.rs
  • storage/Cargo.toml
  • storage/src/rocksdb_storage/storage.rs

Comment thread storage/src/rocksdb_storage/storage.rs Outdated
@shumkov shumkov changed the title feat: subtree dump/restore primitives behind a Cargo feature feat: subtree dump/restore primitives Jun 2, 2026
shumkov and others added 2 commits June 2, 2026 12:34
`#[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>
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>
@shumkov

shumkov commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Re: codecov 0% patch coverage —

Addressed in 62e1296 via two in-tree tests in grovedb/src/tests/commitment_tree_tests.rs:

  • unsafe_dump_load_subtree_roundtrip_preserves_root_hash — full round-trip exercising all three new APIs: builds a CommitmentTree on GroveDb A, dumps the subtree to an SST via raw_storage + SstFileWriter, ingests it into a fresh GroveDb B via ingest_subtree_sst, patches the parent leaf via replace_subtree_root, and asserts the post-restore root_hash matches A byte-for-byte.

  • replace_subtree_root_rejects_non_tree_element — pins the cheap guard rejecting Item / Reference elements (which have no child-hash slot in the parent Merk).

Both are gated on #[cfg(feature = "unsafe-dump-load")]; CI's cargo llvm-cov nextest --all-features picks them up automatically.

The consumer-level test on platform (snapshot_dump_apply_preserves_anchor in dashpay/platform#3774) exercises the same path under realistic scale (30k notes, header + checksum).

@QuantumExplorer
QuantumExplorer merged commit a18f792 into develop Jun 2, 2026
10 checks passed
@QuantumExplorer
QuantumExplorer deleted the feat/snapshot-apply-public-api branch June 2, 2026 08:31
shumkov added a commit to dashpay/platform that referenced this pull request Jun 2, 2026
…lop)

Repoints the pin from the pre-merge feat branch tip
(`2e9dff50455083dc9fbf15b954cea602431acf9d`) to the squash-merge commit on
grovedb develop (`a18f7929460ef9c5d814f61ff84d8805b2a1761b`). Same content,
canonical address.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
shumkov added a commit that referenced this pull request Jun 4, 2026
…rotocol-v11 consensus)

CountSumTree, ProvableCountTree and ProvableCountSumTree must be inserted via
the plain-value path (Op::Put), not the layered-subtree path
(Op::PutLayeredReference). PR #752 accidentally moved them into the layered arm
of add_element_on_transaction, which changes the parent node's value_hash from
value_hash(serialized) to combine_hash(value_hash(serialized), NULL_HASH) and
therefore the grovedb root -- breaking protocol-v11 consensus on replay
(testnet block 245,344: transition_to_version_11 inserts an
empty_provable_count_sum_tree and an empty_count_sum_tree).

Restores the grovedb v4.1.0 behavior frozen into the v11 activation chain.
The v12-only ProvableSumTree / ProvableCountProvableSumTree keep the layered
behavior (never on consensus). Adds a regression test pinning the root.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shumkov pushed a commit that referenced this pull request Jun 4, 2026
…#759)

* fix(insert): version-gate add_element_on_transaction (v0=Op::Put, v1=layered)

PR #752 broadened the non-batch insert tree arm so CountSumTree /
ProvableCountTree / ProvableCountSumTree are written as layered subtrees
(Op::PutLayeredReference). grovedb <= v4.1.0 wrote them as plain values
(Op::Put), and that is the behaviour frozen into the live protocol-v11
activation chain (testnet block 245,344, transition_to_version_11). The
layered op computes a different parent value_hash —
combine_hash(value_hash(serialized), NULL_HASH) instead of
value_hash(serialized) — and therefore a different grovedb root, a consensus
divergence on replay.

Rather than revert unconditionally (cf. #757), split add_element_on_transaction
into a versioned dispatch, mirroring the proof v0/v1 pattern:

- v0: grovedb v4.1.0 behaviour — those three types take the Op::Put arm.
  Selected by GROVE_V1 / GROVE_V2, preserving the protocol-v11 root.
- v1: current behaviour — those three types are layered, consistent with the
  batch insert path (both root hash and fee). Selected by GROVE_V3.

v0/v1 live in their own files as frozen snapshots; they differ only in which
match arm those three element types fall into. GROVE_V3's
add_element_on_transaction version slot is bumped 0 -> 1; v1/v2 stay 0.

Adds a consensus-guard test that replays the transition_to_version_11 shape and
pins both roots: the v0 root is byte-identical to PR #757's protocol-v11 golden,
and the v1 (layered) root differs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(insert): cover all add_element_on_transaction arms under v0 and v1

Adds exhaustive non-batch-insert coverage for both frozen snapshots
(v0.rs / v1.rs): the layered-tree arm (every tree type), the commitment-tree
arm, the append-tree arm (MMR / bulk-append / dense), the item arm, the
reference arm, both override guards, and the empty-tree-only (value.is_some)
guard — driven under GROVE_V1 (v0 / Op::Put) and GROVE_V3 (v1 / layered).
Also asserts the dispatcher rejects an unknown version slot.

Closes the codecov/patch gap on the new add_element_on_transaction module.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants