fix(commitment-tree): reject negative sqlite positions - #746
Conversation
📝 WalkthroughWalkthroughThis PR adds checked SQLite integer conversions and non-negative schema constraints for commitment-tree shard indices and checkpoint positions, then applies them to shard and checkpoint read/write paths. It also adds tests for constraint enforcement, legacy corrupted rows, and overflow rejection. ChangesNegative Value Validation and Overflow Protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/client/sqlite_store/sql_helpers.rs`:
- Around line 128-134: Wrap rusqlite errors from both stmt.query_map calls and
their row iterations with contextual mapping using .map_err(|e|
Error::CorruptedData(format!("...: {}", e))) so callers get operation-specific
context; specifically, update the first loop around stmt.query_map(...) that
constructs Address::from_parts(Level::from(SHARD_HEIGHT),
non_negative_i64_to_u64("shard_index", index?)?) to map any query_map/row error
into Error::CorruptedData with a message like "querying shard indices" and do
the same for the second query_map loop at the other location (lines ~393-399)
with a descriptive message for that operation.
🪄 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: d0757e2e-a0d6-4211-9377-70c0dca97249
📒 Files selected for processing (2)
grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rsgrovedb-commitment-tree/src/client/sqlite_store_tests.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #746 +/- ##
========================================
Coverage 91.49% 91.50%
========================================
Files 240 240
Lines 67763 67790 +27
========================================
+ Hits 62003 62030 +27
Misses 5760 5760
🚀 New features to boost your workflow:
|
cd1aafd to
5ca122d
Compare
|
✅ Review complete (commit 851f1da) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped defensive PR for the SQLite commitment-tree store: adds CHECK constraints to freshly created tables and validates legacy i64 rows on read before converting to u64. Read coverage spans the natural enumeration paths (sql_last_shard, sql_get_shard_roots, checkpoint loads, mark loads) and the test suite exercises both schema-level rejection and legacy-row rejection. No in-scope blockers or suggestions found.
Note: GitHub does not allow PastaClaw to approve their own PR, so this clean review is posted as a comment review rather than an approval.
Out-of-scope follow-up noted
- Write-side
u64 as i64casts and directsql_get_shardindex cast remain unchecked — Multiple write paths (sql_put_shard:114,sql_truncate_shards:146,sql_add_checkpoint:213,224,sql_update_checkpoint_with:323,333) and the targeted read pathsql_get_shard:57still use uncheckedas i64casts onu64values. On the new schema the added CHECK constraints reject these defensively at write time; on legacy schemas a wrapped negative could be written. Pre-existing pattern, not introduced by this PR. The PR's read-side guarantee is enforced where shards/checkpoints are enumerated, which is the realistic legacy-data exposure path. A directget_shard(addr)whose index wraps to a negative would require a contrived caller. Worth tracking separately to applyi64::try_from(u64)symmetrically at the write boundary and the index-bind insql_get_shard.- Follow-up: Open a follow-up issue to use
i64::try_from(u64)for shard_index and position values at the write boundary and thesql_get_shardquery bind ingrovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rs.
- Follow-up: Open a follow-up issue to use
|
Acknowledged the out-of-scope follow-up from the clean review: opened #766 to track replacing the remaining unchecked SQLite |
Several write paths in the sqlite shard store bound u64 shard indexes and checkpoint/mark positions to sqlite via unchecked \`as i64\` casts. For values above i64::MAX this silently wraps to a negative i64 and is rejected by the (post dashpay#746) CHECK constraints at write time, but the error surfaces as a generic sqlite constraint failure rather than as a clear out-of-range error from our code. Add a \`u64_to_i64\` helper that returns \`SqliteShardStoreError::Serialization\` on overflow and apply it to: - sql_get_shard / sql_put_shard shard_index binds - sql_truncate_shards shard_index bind - sql_add_checkpoint position and mark position binds - sql_update_checkpoint_with position and mark position binds Overflow is now detected before any sqlite work happens, so add/update fail without partially writing a checkpoint row. The negative-row read validation added in dashpay#746 (the matching \`non_negative_i64_to_u64\` helper used by read paths) is unchanged. Adds targeted tests for the failure paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grovedb-commitment-tree/src/client/sqlite_store_tests.rs (1)
715-737: ⚡ Quick winAdd the symmetric update test for overflowing removed marks.
sql_update_checkpoint_withnow validatesmarks_removed()positions before starting the rewrite transaction, but the tests only cover overflowing the updated tree position. Add the mark-position case to lock in the no-partial-rewrite guarantee for Lines 335-340 insql_helpers.rs.Test coverage patch
fn test_update_checkpoint_with_rejects_position_overflow() { let mut store = test_store(); store .add_checkpoint(1, Checkpoint::at_position(Position::from(10))) .expect("add"); @@ TreeState::AtPosition(Position::from(10)) ); } + + #[test] + fn test_update_checkpoint_with_rejects_mark_position_overflow() { + let mut store = test_store(); + let mut original_marks = BTreeSet::new(); + original_marks.insert(Position::from(7)); + store + .add_checkpoint( + 1, + Checkpoint::from_parts( + TreeState::AtPosition(Position::from(10)), + original_marks.clone(), + ), + ) + .expect("add"); + + let err = store + .update_checkpoint_with(&1, |cp| { + let mut marks = BTreeSet::new(); + marks.insert(Position::from((i64::MAX as u64) + 1)); + *cp = Checkpoint::from_parts(TreeState::AtPosition(Position::from(10)), marks); + Ok(()) + }) + .expect_err("mark position above i64::MAX should overflow"); + assert_overflow_error(&err, "mark position"); + + let loaded = store.get_checkpoint(&1).expect("get").expect("exists"); + assert_eq!( + loaded.tree_state(), + TreeState::AtPosition(Position::from(10)) + ); + assert_eq!(loaded.marks_removed(), &original_marks); + }🤖 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/client/sqlite_store_tests.rs` around lines 715 - 737, Add a new test function that mirrors the existing test_update_checkpoint_with_rejects_position_overflow test but covers the marks_removed position overflow case instead of the tree position overflow case. The new test should create a checkpoint with marks_removed, attempt to update it with an overflow position for marks_removed (using a value exceeding i64::MAX), verify that the appropriate overflow error is returned, and confirm that the original checkpoint remains unchanged because the validation in sql_helpers.rs lines 335-340 happens before the rewrite transaction starts. This ensures symmetric test coverage for both position validation paths in the update_checkpoint_with operation.
🤖 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.
Nitpick comments:
In `@grovedb-commitment-tree/src/client/sqlite_store_tests.rs`:
- Around line 715-737: Add a new test function that mirrors the existing
test_update_checkpoint_with_rejects_position_overflow test but covers the
marks_removed position overflow case instead of the tree position overflow case.
The new test should create a checkpoint with marks_removed, attempt to update it
with an overflow position for marks_removed (using a value exceeding i64::MAX),
verify that the appropriate overflow error is returned, and confirm that the
original checkpoint remains unchanged because the validation in sql_helpers.rs
lines 335-340 happens before the rewrite transaction starts. This ensures
symmetric test coverage for both position validation paths in the
update_checkpoint_with operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2a81d5b-4397-483d-a351-066bba9be282
📒 Files selected for processing (2)
grovedb-commitment-tree/src/client/sqlite_store/sql_helpers.rsgrovedb-commitment-tree/src/client/sqlite_store_tests.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Leaving a quick note on the current The actionable patch gate is green:
The failing |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Several write paths in the sqlite shard store bound u64 shard indexes and checkpoint/mark positions to sqlite via unchecked \`as i64\` casts. For values above i64::MAX this silently wraps to a negative i64 and is rejected by the (post dashpay#746) CHECK constraints at write time, but the error surfaces as a generic sqlite constraint failure rather than as a clear out-of-range error from our code. Add a \`u64_to_i64\` helper that returns \`SqliteShardStoreError::Serialization\` on overflow and apply it to: - sql_get_shard / sql_put_shard shard_index binds - sql_truncate_shards shard_index bind - sql_add_checkpoint position and mark position binds - sql_update_checkpoint_with position and mark position binds Overflow is now detected before any sqlite work happens, so add/update fail without partially writing a checkpoint row. The negative-row read validation added in dashpay#746 (the matching \`non_negative_i64_to_u64\` helper used by read paths) is unchanged. Adds targeted tests for the failure paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up cleanup on the prior commit: - In `sql_add_checkpoint` and `sql_update_checkpoint_with`, collect mark positions through `Iterator::collect::<Result<_, _>>()?` instead of an explicit `Vec::with_capacity` + `for ... push` loop. Behavior is identical: `collect` short-circuits on the first overflow and returns before `conn.unchecked_transaction()?` is called, preserving the "no partial write on overflow" property. - In sqlite_store_tests.rs, factor the repeated overflow-message assertion in the six new tests into a small `assert_overflow_error` helper, so each test ends with one named assertion line. No public API change; all 38 sqlite store tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a symmetric test that verifies sql_update_checkpoint_with rejects overflowing marks_removed positions and preserves the original checkpoint, mirroring the existing position-overflow case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
54a896b to
6644cce
Compare
|
This is Claude. Rebased this branch onto current develop (maintainer edit) so it sits on top of the recently-merged changes to the same module — notably #767, which renamed |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
2 participants
PR Body
Summary
shard_indexandpositionvalues when readinglegacy commitment tree rows.
CHECKconstraints for newly-created SQLite commitmenttree tables.
u64→i64conversions before binding shard indexes andcheckpoint positions on SQLite write/query helper paths.
overflow failures for shards, checkpoints, and marks-removed positions.
Closes #766.
Validation
cargo build --offline -p grovedb-commitment-treecargo test --offline -p grovedb-commitment-tree --features sqlite— 66 passedcargo fmt -p grovedb-commitment-tree --checkcargo clippy --offline -p grovedb-commitment-tree --features sqlite --tests --no-deps— no new warnings (3 pre-existing unused imports in unrelatedcommitment_frontier/mod.rs)cargo clippy --offline -- -D warnings— fails on pre-existing unrelated dead-code error ingrovedb-bulk-append-tree/src/tree/mod.rs:77shipSummary by CodeRabbit
Bug Fixes
NULLcheckpoint positions.Tests