feat: add grovedb-commitment-tree crate - #413
Merged
Merged
Conversation
Orchard-style commitment tree integration for GroveDB, combining a Sinsemilla frontier (for ZK-friendly anchor computation) with a BulkAppendTree (for efficient append-only storage with epoch compaction). Key components: - CommitmentFrontier: depth-32 incremental Merkle tree using MerkleHashOrchard (Sinsemilla) hashing, ~1KB constant size - CommitmentTree<S, M>: server-side tree generic over MemoSize, with typed ciphertext append and payload size validation - ClientMemoryCommitmentTree: in-memory client for witness generation - ClientPersistentCommitmentTree: SQLite-backed persistent client - Ciphertext serialization helpers (serialize/deserialize/size) Also adds sinsemilla_hash_calls field to OperationCost for tracking elliptic curve hash operations separately from Blake3 node hashes. 72 tests covering frontier, storage, client, and SQLite operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CMT as CommitmentTree
participant CF as CommitmentFrontier
participant BAT as BulkAppendTree
participant Storage as Storage
Client->>CMT: append(cmx, ciphertext)
activate CMT
CMT->>CF: merkle_hash_from_bytes(cmx) / append(hash)
activate CF
CF->>CF: update frontier, increment sinsemilla_hash_calls
CF-->>CMT: sinsemilla_root, hash_count
deactivate CF
CMT->>BAT: append(ciphertext_payload)
activate BAT
BAT->>Storage: write chunk/buffer
BAT-->>CMT: position, bulk_state_root, costs
deactivate BAT
CMT->>CMT: aggregate costs -> CommitmentAppendResult
CMT-->>Client: CommitmentAppendResult
deactivate CMT
sequenceDiagram
participant App as Application
participant CPCT as ClientPersistentCommitmentTree
participant SQLiteStore as SqliteShardStore
participant DB as SQLiteDB
App->>CPCT: open_path(path, max_checkpoints)
activate CPCT
CPCT->>SQLiteStore: new(conn) / ensure_tables()
activate SQLiteStore
SQLiteStore->>DB: CREATE TABLE IF NOT EXISTS ...
SQLiteStore-->>CPCT: configured store
deactivate SQLiteStore
CPCT->>DB: load serialized frontier (deserialize)
CPCT-->>App: ready
deactivate CPCT
App->>CPCT: append(cmx, retention)
activate CPCT
CPCT->>CPCT: merkle_hash_from_bytes
CPCT->>SQLiteStore: shard put/update via ShardTree
SQLiteStore->>DB: INSERT/UPDATE shard rows
SQLiteStore-->>CPCT: ack
CPCT-->>App: Result
deactivate CPCT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![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: 10
🧹 Nitpick comments (3)
costs/src/lib.rs (1)
167-174: Consider adding a helper constructor for consistency.The existing
with_*helper functions (lines 127-174) provide convenient constructors for common cost scenarios. A similar helper forsinsemilla_hash_callscould be useful for future callers.📦 Proposed addition
pub fn with_hash_node_calls(hash_node_calls: u32) -> Self { OperationCost { hash_node_calls, ..Default::default() } } + + /// Helper function to build default `OperationCost` with different + /// `sinsemilla_hash_calls`. + pub fn with_sinsemilla_hash_calls(sinsemilla_hash_calls: u32) -> Self { + OperationCost { + sinsemilla_hash_calls, + ..Default::default() + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@costs/src/lib.rs` around lines 167 - 174, Add a symmetric helper constructor for sinsemilla_hash_calls on OperationCost similar to the existing with_hash_node_calls; implement a pub fn with_sinsemilla_hash_calls(sinsemilla_hash_calls: u32) -> Self that returns OperationCost { sinsemilla_hash_calls, ..Default::default() } so callers can create default OperationCost values with only the sinsemilla_hash_calls overridden (refer to the existing with_hash_node_calls helper and the sinsemilla_hash_calls field).grovedb-commitment-tree/src/client/tests.rs (1)
185-194: Minor: Avoid allocation in expect() on success path.Using
format!insideexpect()allocates a string on every iteration, even when the assertion passes.♻️ Proposed fix
for i in (0..50u64).step_by(2) { let path = tree .witness(Position::from(i), 0) - .expect(&format!("witness note at position {}", i)); + .unwrap_or_else(|e| panic!("witness note at position {}: {:?}", i, e)); assert!( path.is_some(), "should produce witness for marked note at position {}", i ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-commitment-tree/src/client/tests.rs` around lines 185 - 194, The expect(&format!("witness note at position {}", i)) always allocates a String on the success path; replace it with a lazy panic so formatting only happens on error—call unwrap_or_else on the Result returned by tree.witness(Position::from(i), 0) and panic!("witness note at position {}", i) inside the closure (or otherwise provide a non-allocating static message), so formatting is deferred until an actual error; update the call site in the loop iterating (0..50u64).step_by(2) that currently uses expect/format!.grovedb-commitment-tree/src/commitment_frontier/mod.rs (1)
219-221:empty_sinsemilla_root()can return the precomputed constant directly.Line 217-Line 218 says cached/precomputed, but the function recomputes each call. Returning
EMPTY_SINSEMILLA_ROOTkeeps docs and behavior aligned.Suggested simplification
pub fn empty_sinsemilla_root() -> [u8; 32] { - MerkleHashOrchard::empty_root(Level::from(NOTE_COMMITMENT_TREE_DEPTH as u8)).to_bytes() + EMPTY_SINSEMILLA_ROOT }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-commitment-tree/src/commitment_frontier/mod.rs` around lines 219 - 221, The function empty_sinsemilla_root currently recomputes the root each call; change it to return the precomputed constant EMPTY_SINSEMILLA_ROOT directly (i.e., have empty_sinsemilla_root() return EMPTY_SINSEMILLA_ROOT) so docs and behavior match the cached/precomputed comment and avoid unnecessary recomputation; update any references if needed to keep the return type [u8; 32] consistent with EMPTY_SINSEMILLA_ROOT.
🤖 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-commitment-tree/src/client/sqlite_client_tests.rs`:
- Around line 117-137: The test currently assumes coexistence because
ClientPersistentCommitmentTree::open(conn, 100) succeeds but never proves the
original my_app_data table is still readable; change the test to use two
connections to the same on-disk DB so one can be handed to the tree while the
other verifies the app table post-writes: create a temp file DB (or use a URI
with shared cache), open conn1 to create and populate my_app_data, open conn2 to
the same DB and pass conn2 into ClientPersistentCommitmentTree::open, perform
tree.append(...), then query my_app_data using conn1 to assert the row is still
present. Ensure you reference the test_bring_your_own_connection function, the
ClientPersistentCommitmentTree::open call, and the my_app_data table in your
assertions.
In `@grovedb-commitment-tree/src/client/sqlite_store_tests.rs`:
- Around line 49-52: The test test_schema_idempotent currently only creates a
SqliteShardStore once; change it to open an in-memory Connection, call
SqliteShardStore::new(conn) twice (re-using the same Connection) to ensure the
second initialization succeeds and is idempotent—i.e., construct _store1 =
SqliteShardStore::new(conn.clone() or the same conn) and then _store2 =
SqliteShardStore::new(same conn) and assert both creations return Ok (no panic)
to validate idempotency of SqliteShardStore::new.
In `@grovedb-commitment-tree/src/client/sqlite_store.rs`:
- Around line 452-469: The three DELETEs in sql_truncate_checkpoints_retaining
must be executed inside a single DB transaction to avoid partial state on error;
change the function to open a transaction (e.g., let tx = conn.transaction()? ),
replace the three conn.execute(...) calls with tx.execute(...) using the same
SQL and params![checkpoint_id], then call tx.commit()? to commit and return
Ok(()); ensure any error from begin/execute/commit is propagated as
SqliteShardStoreError.
- Around line 400-435: The sequence of delete/update/inserts in
sql_update_checkpoint_with must be atomic; wrap the read-modify-write steps in a
database transaction: start a transaction (e.g., let tx = conn.transaction()?),
call sql_get_checkpoint against the transaction (or otherwise ensure the SELECT
runs in the same tx), run the DELETE, UPDATE and all INSERTs using the
transaction handle (tx.execute(...)), and then commit the transaction
(tx.commit()) so failures roll back; if tx.commit() fails, return the
appropriate SqliteShardStoreError. Ensure references to
sql_update_checkpoint_with, sql_get_checkpoint, and the SQL statements are
updated to use the transaction API rather than executing directly on conn.
- Around line 304-326: The function sql_add_checkpoint performs multiple related
inserts and must be wrapped in a database transaction to avoid partial commits;
change it to start a transaction (via Connection::transaction or equivalent) at
the top of sql_add_checkpoint, perform the initial INSERT into
commitment_tree_checkpoints and the subsequent INSERTs into
commitment_tree_checkpoint_marks_removed using the transaction object (e.g.,
txn.execute), then commit the transaction at the end so any error during the
loop will roll back; ensure you replace conn.execute calls with txn.execute and
call txn.commit() before returning Ok(()).
In `@grovedb-commitment-tree/src/commitment_frontier/mod.rs`:
- Around line 56-61: The sinsemilla hash cost uses trailing_ones on the current
frontier position but must use the next leaf index (the position after
f.position()) because ommer merges for an append depend on the next index;
update the mapping that computes ommer_hashes (the closure on
self.frontier.value().map(...)) to compute trailing_ones from the next position
(e.g., f.position() + 1) instead of the current position, and keep the
subsequent update to cost.sinsemilla_hash_calls the same (adding 32 +
ommer_hashes).
In `@grovedb-commitment-tree/src/commitment_tree/mod.rs`:
- Around line 231-259: The append_raw flow mutates BulkAppendTree before
updating the Sinsemilla frontier, which can leave bulk advanced if
frontier.append fails; change the order so you call self.frontier.append(cmx)
first (handling its CostContext and error wrapping into cost) and only if that
succeeds call self.bulk_tree.append(&item_value), then add
bulk_result.hash_count to cost.hash_node_calls — ensure cost is accumulated in
the same variables and error returns still call .wrap_with_cost(cost) so no
partial commit occurs when frontier.append fails.
- Around line 102-106: The struct CommitmentTree exposes bulk_tree publicly
which allows callers to append directly and bypass CommitmentTree invariants;
make bulk_tree private (remove pub) and provide/ensure all external
modifications go through the tree's validated APIs such as append_raw (or add a
controlled wrapper method) so frontier remains synchronized with BulkAppendTree;
update any callers that accessed CommitmentTree::bulk_tree to use the new
private API or wrapper methods and ensure append_raw continues to perform the
necessary validation/synchronization with frontier.
- Around line 163-176: After loading the stored frontier in the open path,
validate that the restored CommitmentFrontier size matches the persisted bulk
tree count by comparing frontier.tree_size() to bulk_tree.total_count; if they
differ, return an appropriate CommitmentTreeError (or wrap_with_cost(cost)) to
prevent using a stale/missing frontier. Locate the match that produces
`frontier` (the block using CommitmentFrontier::deserialize and
CommitmentFrontier::new) and add a check immediately after that assignment
comparing `frontier.tree_size()` and `bulk_tree.total_count`, returning an error
on mismatch so anchors/roots can't be computed from an inconsistent frontier.
In `@grovedb-commitment-tree/src/lib.rs`:
- Around line 17-29: The re-exports ClientPersistentCommitmentTree,
SqliteShardStore, and SqliteShardStoreError are gated by #[cfg(feature =
"sqlite")] but the client module (mod client) is only compiled under
#[cfg(feature = "client")], causing a missing-module compile when sqlite is
enabled without client; either add "client" to the sqlite feature in Cargo.toml
(so sqlite includes client) or change the re-export guards to #[cfg(all(feature
= "client", feature = "sqlite"))] so the symbols
(ClientPersistentCommitmentTree, SqliteShardStore, SqliteShardStoreError) are
only re-exported when the client module exists.
---
Nitpick comments:
In `@costs/src/lib.rs`:
- Around line 167-174: Add a symmetric helper constructor for
sinsemilla_hash_calls on OperationCost similar to the existing
with_hash_node_calls; implement a pub fn
with_sinsemilla_hash_calls(sinsemilla_hash_calls: u32) -> Self that returns
OperationCost { sinsemilla_hash_calls, ..Default::default() } so callers can
create default OperationCost values with only the sinsemilla_hash_calls
overridden (refer to the existing with_hash_node_calls helper and the
sinsemilla_hash_calls field).
In `@grovedb-commitment-tree/src/client/tests.rs`:
- Around line 185-194: The expect(&format!("witness note at position {}", i))
always allocates a String on the success path; replace it with a lazy panic so
formatting only happens on error—call unwrap_or_else on the Result returned by
tree.witness(Position::from(i), 0) and panic!("witness note at position {}", i)
inside the closure (or otherwise provide a non-allocating static message), so
formatting is deferred until an actual error; update the call site in the loop
iterating (0..50u64).step_by(2) that currently uses expect/format!.
In `@grovedb-commitment-tree/src/commitment_frontier/mod.rs`:
- Around line 219-221: The function empty_sinsemilla_root currently recomputes
the root each call; change it to return the precomputed constant
EMPTY_SINSEMILLA_ROOT directly (i.e., have empty_sinsemilla_root() return
EMPTY_SINSEMILLA_ROOT) so docs and behavior match the cached/precomputed comment
and avoid unnecessary recomputation; update any references if needed to keep the
return type [u8; 32] consistent with EMPTY_SINSEMILLA_ROOT.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Cargo.tomlcosts/src/lib.rsgrovedb-commitment-tree/Cargo.tomlgrovedb-commitment-tree/benches/verification.rsgrovedb-commitment-tree/src/client/client_memory_commitment_tree.rsgrovedb-commitment-tree/src/client/client_persistent_commitment_tree.rsgrovedb-commitment-tree/src/client/mod.rsgrovedb-commitment-tree/src/client/sqlite_client_tests.rsgrovedb-commitment-tree/src/client/sqlite_store.rsgrovedb-commitment-tree/src/client/sqlite_store_tests.rsgrovedb-commitment-tree/src/client/tests.rsgrovedb-commitment-tree/src/commitment_frontier/mod.rsgrovedb-commitment-tree/src/commitment_frontier/tests.rsgrovedb-commitment-tree/src/commitment_tree/mod.rsgrovedb-commitment-tree/src/commitment_tree/tests.rsgrovedb-commitment-tree/src/error.rsgrovedb-commitment-tree/src/lib.rs
…nCost literals - Replace bare .unwrap() with .expect() in commitment-tree tests - Split sqlite_store.rs into mod.rs, sql_helpers.rs, tree_serialization.rs - Deduplicate SHARD_HEIGHT and test_leaf into shared locations - Add 12 new coverage tests for deserialize, buffer/chunk, state root - Validate cmx field element before BulkAppendTree mutation (F6) - Add recursion depth limit to tree deserialization (V1) - Wrap SQLite checkpoint ops in unchecked_transaction (V9) - Add frontier vs bulk count validation in CommitmentTree::open() - Fix test_schema_idempotent to actually test idempotency - Fix test_bring_your_own_connection to use shared connection - Add sinsemilla_hash_calls: 0 to all OperationCost struct literals - Fix unused variable/import warnings in grovedb tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
grovedb-commitment-tree/Cargo.toml (1)
11-11: Consider updating documentation URL to point to this crate's docs.The
documentationfield points tohttps://docs.rs/grovedbrather thanhttps://docs.rs/grovedb-commitment-tree. Users looking for this crate's API documentation will be directed to the parent crate's docs instead.Suggested fix
-documentation = "https://docs.rs/grovedb" +documentation = "https://docs.rs/grovedb-commitment-tree"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-commitment-tree/Cargo.toml` at line 11, Update the Cargo.toml documentation field value so it points to this crate's docs instead of the parent crate: change the documentation entry named "documentation" from "https://docs.rs/grovedb" to "https://docs.rs/grovedb-commitment-tree" in the grovedb-commitment-tree Cargo.toml.grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs (1)
349-368: Clarify the truncation semantics in documentation.The function retains the checkpoint at
checkpoint_idbut clears itsmarks_removedset (lines 362-365). This is correct for "rewind to checkpoint" semantics, but the behavior is subtle. Consider adding a doc comment explaining that retained checkpoint has its marks cleared.📝 Suggested documentation improvement
+/// Truncate checkpoints, keeping only those with `checkpoint_id <= checkpoint_id`. +/// +/// The retained checkpoint at `checkpoint_id` has its `marks_removed` set cleared, +/// as those marks represent removals that occurred *after* this checkpoint state. pub(crate) fn sql_truncate_checkpoints_retaining( conn: &Connection, checkpoint_id: u32, ) -> Result<(), SqliteShardStoreError> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs` around lines 349 - 368, Add a doc comment to sql_truncate_checkpoints_retaining explaining its truncation semantics: it deletes all checkpoints with id > checkpoint_id and also clears the marks_removed set for the retained checkpoint (checkpoint_id) so the checkpoint itself is kept but its marks are reset — useful to clarify "rewind to checkpoint" behavior; update the function-level comment above sql_truncate_checkpoints_retaining to state this explicitly and mention that marks_removed entries for the retained checkpoint are removed by the third DELETE.grovedb-commitment-tree/src/lib.rs (1)
32-32: Consider explicit re-exports instead of glob forcommitment_frontier.Using
pub use commitment_frontier::*re-exports all public items from that module. If new items are added tocommitment_frontier, they automatically become part of this crate's public API, which could lead to unintentional API changes. Consider listing exports explicitly for better API control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb-commitment-tree/src/lib.rs` at line 32, Replace the glob re-export to explicit exports to avoid accidental public API expansion: instead of `pub use commitment_frontier::*;`, list each public item you intend to re-export from the `commitment_frontier` module (e.g., structs, enums, functions, traits) by name (for example `pub use commitment_frontier::{Commitment, CommitmentTree, FrontierError};`), updating the list whenever you intentionally add new public items; locate the `pub use commitment_frontier::*` line in lib.rs and replace it with the explicit `pub use commitment_frontier::{ ... }` statement containing the exact symbols to export.
🤖 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-commitment-tree/Cargo.toml`:
- Around line 20-27: Update the incrementalmerkletree dependency to ensure
compatibility with shardtree: change the version constraint for
incrementalmerkletree in Cargo.toml from "0.8" to "0.8.1" so that
incrementalmerkletree satisfies shardtree (shardtree = { version = "0.6",
optional = true }) which requires >= 0.8.1; modify the line referencing
incrementalmerkletree to "0.8.1".
In `@grovedb-commitment-tree/src/client/sqlite_store/mod.rs`:
- Around line 109-113: Replace panic-on-poisoned-mutex logic with proper error
propagation: add a ConnectionPoisoned variant to the SqliteShardStoreError enum
and update its Display impl, change new_shared to map conn.lock() failures into
Err(SqliteShardStoreError::ConnectionPoisoned) instead of expect, change
with_conn to take a closure that returns Result<T, SqliteShardStoreError> and
map conn.lock() failures into the same ConnectionPoisoned error, and update all
ShardStore trait method calls that call with_conn (and new_shared) to propagate
the returned SqliteShardStoreError (they already use ? in closures so adjust
signatures accordingly).
In `@grovedb-commitment-tree/src/client/sqlite_store/tree_serialization.rs`:
- Around line 123-144: The deserializer currently treats any flag byte other
than 0x01 as "no annotation"; change the logic around has_ann to explicitly
accept only 0x00 (meaning None) or 0x01 (meaning parse 32 bytes and produce
Some(Arc<MerkleHashOrchard>)), and return
Err(SqliteShardStoreError::Serialization("invalid parent annotation
flag".to_string())) for any other value; update the branch that computes ann
(which uses variables has_ann, *pos, data, merkle_hash_from_bytes, and
constructs SqliteShardStoreError::Serialization) so malformed flag bytes are
rejected rather than treated as None.
In `@grovedb-commitment-tree/src/test_utils.rs`:
- Around line 8-13: test_leaf currently uses index % 31 which creates collisions
(e.g., 0 and 31); change the leaf derivation to incorporate the full index
entropy instead of truncating it. Specifically, modify test_leaf so the call
that produces varied (currently using
MerkleHashOrchard::combine(Level::from((index % 31) as u8 + 1), &empty, &empty))
is replaced by a derivation that mixes the full index (for example, hash
index.to_le_bytes() or otherwise include index.to_le_bytes() when building the
input to MerkleHashOrchard::combine) so each distinct index yields a distinct
varied value; keep using MerkleHashOrchard::combine and Level::from but derive
the level or the combined inputs from the full index rather than index % 31.
---
Nitpick comments:
In `@grovedb-commitment-tree/Cargo.toml`:
- Line 11: Update the Cargo.toml documentation field value so it points to this
crate's docs instead of the parent crate: change the documentation entry named
"documentation" from "https://docs.rs/grovedb" to
"https://docs.rs/grovedb-commitment-tree" in the grovedb-commitment-tree
Cargo.toml.
In `@grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs`:
- Around line 349-368: Add a doc comment to sql_truncate_checkpoints_retaining
explaining its truncation semantics: it deletes all checkpoints with id >
checkpoint_id and also clears the marks_removed set for the retained checkpoint
(checkpoint_id) so the checkpoint itself is kept but its marks are reset —
useful to clarify "rewind to checkpoint" behavior; update the function-level
comment above sql_truncate_checkpoints_retaining to state this explicitly and
mention that marks_removed entries for the retained checkpoint are removed by
the third DELETE.
In `@grovedb-commitment-tree/src/lib.rs`:
- Line 32: Replace the glob re-export to explicit exports to avoid accidental
public API expansion: instead of `pub use commitment_frontier::*;`, list each
public item you intend to re-export from the `commitment_frontier` module (e.g.,
structs, enums, functions, traits) by name (for example `pub use
commitment_frontier::{Commitment, CommitmentTree, FrontierError};`), updating
the list whenever you intentionally add new public items; locate the `pub use
commitment_frontier::*` line in lib.rs and replace it with the explicit `pub use
commitment_frontier::{ ... }` statement containing the exact symbols to export.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
grovedb-commitment-tree/Cargo.tomlgrovedb-commitment-tree/src/client/client_memory_commitment_tree.rsgrovedb-commitment-tree/src/client/client_persistent_commitment_tree.rsgrovedb-commitment-tree/src/client/mod.rsgrovedb-commitment-tree/src/client/sqlite_client_tests.rsgrovedb-commitment-tree/src/client/sqlite_store/mod.rsgrovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rsgrovedb-commitment-tree/src/client/sqlite_store/tree_serialization.rsgrovedb-commitment-tree/src/client/sqlite_store_tests.rsgrovedb-commitment-tree/src/client/tests.rsgrovedb-commitment-tree/src/commitment_frontier/tests.rsgrovedb-commitment-tree/src/commitment_tree/mod.rsgrovedb-commitment-tree/src/commitment_tree/tests.rsgrovedb-commitment-tree/src/lib.rsgrovedb-commitment-tree/src/test_utils.rsgrovedb/src/batch/estimated_costs/average_case_costs.rsgrovedb/src/batch/estimated_costs/worst_case_costs.rsgrovedb/src/batch/multi_insert_cost_tests.rsgrovedb/src/batch/single_insert_cost_tests.rsgrovedb/src/batch/single_sum_item_insert_cost_tests.rsgrovedb/src/operations/delete/mod.rsgrovedb/src/operations/insert/mod.rsgrovedb/src/tests/provable_count_tree_test.rsgrovedb/src/tests/test_provable_count_fresh.rsmerk/src/element/get.rs
✅ Files skipped from review due to trivial changes (1)
- grovedb/src/batch/single_sum_item_insert_cost_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- grovedb-commitment-tree/src/client/sqlite_store_tests.rs
- grovedb-commitment-tree/src/client/client_memory_commitment_tree.rs
- grovedb-commitment-tree/src/client/mod.rs
Comment on lines
+20
to
+27
| [dependencies] | ||
| orchard = { git = "https://github.com/dashpay/orchard.git", rev = "41c8f7169f2683c99cf0e0c63e8d25ec12c47a79", features = ["circuit"] } | ||
| incrementalmerkletree = "0.8" | ||
| shardtree = { version = "0.6", optional = true } | ||
| rusqlite = { version = "0.38", features = ["bundled"], optional = true } | ||
| grovedb-costs = { version = "4.0.0", path = "../costs" } | ||
| grovedb-storage = { version = "4.0.0", path = "../storage", optional = true } | ||
| grovedb-bulk-append-tree = { version = "4.0.0", path = "../grovedb-bulk-append-tree", optional = true } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dependency version compatibility
echo "=== Checking shardtree 0.6 dependencies on crates.io ==="
curl -s "https://crates.io/api/v1/crates/shardtree/0.6.1" | jq -r '.version.deps[]' 2>/dev/null | head -20
echo -e "\n=== Checking incrementalmerkletree 0.8.x versions ==="
curl -s "https://crates.io/api/v1/crates/incrementalmerkletree" 2>/dev/null | jq -r '.versions[] | select(.num | startswith("0.8")) | .num' | sort -V
echo -e "\n=== Checking latest shardtree versions ==="
curl -s "https://crates.io/api/v1/crates/shardtree" 2>/dev/null | jq -r '.versions[0:5] | .[] | .num'
echo -e "\n=== Checking thiserror 2.x versions ==="
curl -s "https://crates.io/api/v1/crates/thiserror" 2>/dev/null | jq -r '.versions[] | select(.num | startswith("2.")) | .num' | head -5Repository: dashpay/grovedb
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Check shardtree's actual dependency requirements
echo "=== Checking shardtree 0.6.1 full crate data ==="
curl -s "https://crates.io/api/v1/crates/shardtree/0.6.1" | jq '.version' 2>/dev/null
echo -e "\n=== Checking shardtree 0.6.1 dependencies directly ==="
curl -s "https://crates.io/api/v1/crates/shardtree/0.6.1/dependencies" 2>/dev/null | jq '.'
echo -e "\n=== Alternative: checking raw Cargo.toml from GitHub ==="
curl -s "https://raw.githubusercontent.com/zcash/incrementalmerkletree/main/shardtree/Cargo.toml" 2>/dev/null | grep -A 5 "\[dependencies\]"Repository: dashpay/grovedb
Length of output: 5780
🏁 Script executed:
cat grovedb-commitment-tree/Cargo.toml | sed -n '20,30p'Repository: dashpay/grovedb
Length of output: 637
Pin incrementalmerkletree to at least 0.8.1 for shardtree 0.6 compatibility.
The current constraint incrementalmerkletree = "0.8" allows version 0.8.0, but shardtree = { version = "0.6", optional = true } requires incrementalmerkletree >= 0.8.1. Change this to incrementalmerkletree = "0.8.1" to ensure the correct version is selected and avoid potential dependency resolution issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@grovedb-commitment-tree/Cargo.toml` around lines 20 - 27, Update the
incrementalmerkletree dependency to ensure compatibility with shardtree: change
the version constraint for incrementalmerkletree in Cargo.toml from "0.8" to
"0.8.1" so that incrementalmerkletree satisfies shardtree (shardtree = { version
= "0.6", optional = true }) which requires >= 0.8.1; modify the line referencing
incrementalmerkletree to "0.8.1".
- test_leaf now chains combine over all 8 bytes of the index instead of using index % 31, which caused collisions (e.g. 0 and 31) - Tree deserializer now explicitly matches 0x00/0x01 for the parent annotation flag and returns an error for any other value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Self Reviewed |
- Use += for OperationCost addition (assign_op_pattern) - Remove module_inception: unwrap inner mod tests from tests.rs files - Replace expect(&format!()) with unwrap_or_else (expect_fun_call) - Add #![warn(missing_docs)] to crate root - Add doc comments to all CommitmentTreeError variants and fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
grovedb-commitment-tree/src/commitment_tree/mod.rs (1)
233-239:⚠️ Potential issue | 🟠 MajorAvoid partial state when bulk append succeeds but frontier append fails.
At Line 270, storage is mutated before frontier update. If Line 283 returns
TreeFull, bulk state advances while frontier does not, breaking invariants.🔧 Suggested fix (frontier-first with rollback on bulk failure)
- /// 1. Appends `cmx || payload` to the `BulkAppendTree` (data storage) - /// 2. Appends `cmx` to the Sinsemilla frontier (in-memory) + /// 1. Appends `cmx` to the Sinsemilla frontier (in-memory) + /// 2. Appends `cmx || payload` to the `BulkAppendTree` (data storage) @@ - // 1. Build cmx||payload and append to BulkAppendTree + // 1. Build cmx||payload let mut item_value = Vec::with_capacity(32 + payload.len()); item_value.extend_from_slice(&cmx); item_value.extend_from_slice(payload); - let bulk_result = match self.bulk_tree.append(&item_value) { - Ok(r) => r, - Err(e) => { - return Err(CommitmentTreeError::InvalidData(format!( - "bulk append: {}", - e - ))) - .wrap_with_cost(cost); - } - }; - cost.hash_node_calls += bulk_result.hash_count; - - // 2. Append cmx to Sinsemilla frontier (tracks sinsemilla_hash_calls) + // 2. Append cmx to Sinsemilla frontier first, with rollback support. + let frontier_before = self.frontier.clone(); let sinsemilla_root = match self.frontier.append(cmx) { grovedb_costs::CostContext { value: Ok(root), cost: frontier_cost, } => { cost += frontier_cost; root } grovedb_costs::CostContext { value: Err(e), cost: frontier_cost, } => { cost += frontier_cost; return Err(e).wrap_with_cost(cost); } }; + + // 3. Append cmx||payload to BulkAppendTree. + let bulk_result = match self.bulk_tree.append(&item_value) { + Ok(r) => r, + Err(e) => { + self.frontier = frontier_before; + return Err(CommitmentTreeError::InvalidData(format!( + "bulk append: {}", + e + ))) + .wrap_with_cost(cost); + } + }; + cost.hash_node_calls += bulk_result.hash_count;Also applies to: 270-298
🤖 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 233 - 239, The bulk append mutates storage (BulkAppendTree) before updating the Sinsemilla frontier, so if frontier.append (the in-memory update) fails (e.g., returns TreeFull) the on-disk bulk state advances and invariants break; fix by performing the frontier update first (call frontier.append or the Sinsemilla frontier method that adds cmx) and only then perform the storage bulk append, or if you must append storage first, implement a rollback path that removes the last appended entry from BulkAppendTree when frontier.append fails; update the function that performs "Append a note commitment and raw payload" to use frontier-first with rollback-on-storage-failure (or storage-then-rollback-on-frontier-failure) and ensure error handling cleans up both components to preserve invariants.
🤖 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-commitment-tree/src/client/sqlite_store/tree_serialization.rs`:
- Around line 114-115: The deserialization currently uses
RetentionFlags::from_bits_truncate which silently masks unknown bits; change the
logic in tree deserialization where flags_byte is converted (currently before
calling Tree::leaf((hash, flags))) to perform strict validation by using
RetentionFlags::from_bits(flags_byte) and returning an Err (propagate the
existing deserialization error type) when from_bits returns None, mirroring the
strict validation used for parent annotation flags in the same module; ensure
the error path produces a clear deserialization failure rather than constructing
Tree::leaf with masked flags.
---
Duplicate comments:
In `@grovedb-commitment-tree/src/commitment_tree/mod.rs`:
- Around line 233-239: The bulk append mutates storage (BulkAppendTree) before
updating the Sinsemilla frontier, so if frontier.append (the in-memory update)
fails (e.g., returns TreeFull) the on-disk bulk state advances and invariants
break; fix by performing the frontier update first (call frontier.append or the
Sinsemilla frontier method that adds cmx) and only then perform the storage bulk
append, or if you must append storage first, implement a rollback path that
removes the last appended entry from BulkAppendTree when frontier.append fails;
update the function that performs "Append a note commitment and raw payload" to
use frontier-first with rollback-on-storage-failure (or
storage-then-rollback-on-frontier-failure) and ensure error handling cleans up
both components to preserve invariants.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
grovedb-commitment-tree/src/client/sqlite_store/tree_serialization.rsgrovedb-commitment-tree/src/client/tests.rsgrovedb-commitment-tree/src/commitment_frontier/mod.rsgrovedb-commitment-tree/src/commitment_frontier/tests.rsgrovedb-commitment-tree/src/commitment_tree/mod.rsgrovedb-commitment-tree/src/error.rsgrovedb-commitment-tree/src/lib.rsgrovedb-commitment-tree/src/test_utils.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- grovedb-commitment-tree/src/error.rs
5 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
grovedb-commitment-treecrate: Orchard-style commitment tree combining a Sinsemilla frontier with a BulkAppendTree for ZK-friendly anchor computation and efficient append-only storageCommitmentFrontier: depth-32 incremental Merkle tree using Sinsemilla (MerkleHashOrchard) hashing, ~1KB constant size regardless of tree depthCommitmentTree<S, M>: server-side tree generic overMemoSize, with typedTransmittedNoteCiphertext<M>append and payload size validation (216 bytes for DashMemo)ClientMemoryCommitmentTree(in-memory) andClientPersistentCommitmentTree(SQLite-backed) for wallet note tracking and spend proof constructionsinsemilla_hash_callsfield toOperationCostfor tracking elliptic curve hash operations separately from Blake3Crate structure
commitment_frontiercommitment_treeCommitmentTree<S, M>combining frontier + BulkAppendTreeclienterrorCommitmentTreeErrorwithInvalidPayloadSizevariantFeatures
serverstorageCommitmentTree<S, M>with GroveDB storage integrationclientClientMemoryCommitmentTree(shardtree dep)sqliteClientPersistentCommitmentTree+SqliteShardStore(rusqlite dep)Test plan
cargo test -p grovedb-commitment-tree --all-features)cargo build --workspace)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests