fix(drive): authenticate boundary in compacted absence proofs#3792
fix(drive): authenticate boundary in compacted absence proofs#3792QuantumExplorer wants to merge 11 commits into
Conversation
The compacted nullifier-changes and address-balance-changes proof verifiers derived the forward-scan lower bound (`start_key`) from raw, un-authenticated proof bytes. Because compacted keys are `(start_block_be, end_block_be)`, a range like `(100, 200)` that contains a requested height `150` sorts lexicographically before `(150, 150)`. A malicious node could therefore prove `range_from((150, 150)..)` directly — a valid GroveDB proof in which the containing range appears only as a hash-only boundary node — and the verifier would accept the incomplete proof and silently return zero results, concealing a spent nullifier (double-spend) or a balance change from trust-minimized light clients. Fix: never trust the proof bytes to self-report the boundary. Use GroveDB's `verify_query_with_chained_path_queries`: 1. A boundary query (descending `range_to_inclusive(..=(start, MAX))`, limit 1) authenticates the single greatest compacted key `<= (start, MAX)` against the real root hash. A malicious prover cannot substitute or omit it without breaking the root. 2. A generator derives the forward `start_key` from that authenticated boundary (the containing range iff its `end_block >= start`). 3. The chained forward query is verified against the same root. The provers now emit a single merged proof (`prove_query_many`) covering both the boundary key and the forward range. Both parallel instances are fixed (shielded nullifiers and address-fund balances). Adds PoC tests proving the malicious skip-descending proof is now rejected and that honest containing-range/roundtrip proofs still verify. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughRefactors provers to discover an authenticated compacted boundary key then prove a forward range (merged when boundary exists). Verifiers switch from merk-proof KV extraction to GroveDB chained path-query verification that derives forward-start from an authenticated boundary. Adds ChangesCompacted proofs refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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 |
|
✅ Review complete (commit 3e20289) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs (1)
301-307: 💤 Low valueConsider extracting
compacted_keyto a shared module.This helper is duplicated in the verifier (
verify_compacted_address_balance_changes/v0/mod.rs). Both modules construct the same 16-byte big-endian key format. Extracting to a shared utility (e.g., acompacted_utilsmodule orDriveassociated function) would centralize the key format definition.🤖 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 `@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs` around lines 301 - 307, The compacted_key helper is duplicated; extract its implementation to a shared place (e.g., a new module like compacted_utils::compacted_key or an associated Drive::compacted_key) and replace both local definitions (the compacted_key function in this file and the one in verify_compacted_address_balance_changes/v0/mod.rs) with calls to that shared function; ensure the shared function signature takes (start_block: u64, end_block: u64) -> Vec<u8> and retains the 16-byte big-endian construction so both callers keep the exact same key format.packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs (1)
10-16: 💤 Low valueConsider extracting
compacted_keyhelper to a shared module.This helper function is duplicated in at least three places:
- This prover file
verify_compacted_nullifier_changes/v0/mod.rsverify_compacted_address_balance_changes/v0/mod.rs(per context snippet 1)Consider extracting to a shared utility module (e.g.,
crate::drive::shielded::nullifiers::keysor a common compacted-key module) to reduce duplication.🤖 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 `@packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs` around lines 10 - 16, The compacted_key function is duplicated; extract it to a shared utility module (e.g., create a new module like crate::drive::shielded::keys or crate::drive::shielded::nullifiers::keys) and move the implementation there as a public function pub fn compacted_key(start_block: u64, end_block: u64) -> Vec<u8>. Replace local definitions in prove_compacted_nullifier_changes::v0::mod (the compacted_key in this file), verify_compacted_nullifier_changes::v0::mod, and verify_compacted_address_balance_changes::v0::mod with a use/import of the new shared compacted_key to remove duplication and ensure visibility and tests/compilation are updated accordingly.
🤖 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
`@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs`:
- Around line 301-307: The compacted_key helper is duplicated; extract its
implementation to a shared place (e.g., a new module like
compacted_utils::compacted_key or an associated Drive::compacted_key) and
replace both local definitions (the compacted_key function in this file and the
one in verify_compacted_address_balance_changes/v0/mod.rs) with calls to that
shared function; ensure the shared function signature takes (start_block: u64,
end_block: u64) -> Vec<u8> and retains the 16-byte big-endian construction so
both callers keep the exact same key format.
In
`@packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs`:
- Around line 10-16: The compacted_key function is duplicated; extract it to a
shared utility module (e.g., create a new module like
crate::drive::shielded::keys or crate::drive::shielded::nullifiers::keys) and
move the implementation there as a public function pub fn
compacted_key(start_block: u64, end_block: u64) -> Vec<u8>. Replace local
definitions in prove_compacted_nullifier_changes::v0::mod (the compacted_key in
this file), verify_compacted_nullifier_changes::v0::mod, and
verify_compacted_address_balance_changes::v0::mod with a use/import of the new
shared compacted_key to remove duplication and ensure visibility and
tests/compilation are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64eb07da-f4ef-4ad0-bc36-02f959467637
📒 Files selected for processing (4)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3792 +/- ##
============================================
- Coverage 87.18% 87.18% -0.01%
============================================
Files 2624 2625 +1
Lines 321014 321413 +399
============================================
+ Hits 279892 280220 +328
- Misses 41122 41193 +71
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "7ab69025545128e703b6955c6d94a0f0d868eeee60c53ea66893bffecc75b6a4"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR closes a real proof-soundness hole by replacing un-authenticated boundary derivation with GroveDB chained path queries, and is well-tested (including a malicious-prover regression test). However, the new Some(boundary_key) prover branch drops the caller-supplied limit for both compacted endpoints — the public gRPC handlers explicitly pass limit = Some(25) with a comment 'max 25 to stay within proof size limits', and this regression lets an unauthenticated client trigger proofs over the entire compacted tail. The same branch also silently ignores the TransactionArg argument, creating a latent root-hash inconsistency for any future transactional caller.
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs:108-126: Merged nullifier proof drops caller `limit`, enabling unbounded proof amplification
In the `Some(boundary_key)` branch the forward query is built with `SizedQuery::new(forward_query, None, None)` and fed to `prove_query_many`, so the caller's `limit` is no longer enforced once the compacted tree is non-empty (the common case). The public handler `query_recent_compacted_nullifier_changes_v0` (packages/rs-drive-abci/src/query/shielded/recent_compacted_nullifier_changes/v0/mod.rs:25) deliberately passes `limit = Some(25u16)` — annotated in the address-balance handler as 'max 25 to stay within proof size limits' — but here that cap is bypassed, while the `None` branch (line 136) still honors it. An unauthenticated client can call `prove=true` with `start_block_height = 0` and force the node to walk and serialize every compacted nullifier range, which becomes a remote CPU/memory/bandwidth amplification vector against proof-serving masternodes. Either (a) wrap the merged `PathQuery` with a top-level `SizedQuery` limit before proving, or (b) decline to merge when a boundary key is found and instead build a single `PathQuery` whose ranges union the boundary point and the limited forward scan. The verifier already enforces `limit` on its forward subset query, so soundness is unaffected by capping the prover.
- [SUGGESTION] packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs:108-126: `Some(boundary_key)` branch silently drops `transaction: TransactionArg`
`prove_compacted_nullifier_changes_v0` accepts `transaction: TransactionArg` and threads it through step 1's descending probe (line 56-62) and through the `None` branch's `grove_get_proved_path_query` (line 138-143). The new `Some(boundary_key)` branch instead calls `self.grove.prove_query_many(..., None, ...)`, where the `None` is a `ProveOptions`, not the transaction — so under an active write transaction step 1 sees the transactional snapshot while step 2 builds the merged proof against the committed snapshot, producing either a malformed proof or one bound to a stale root hash. All current production callers pass `transaction = None`, but the public signature advertises transactional support that this path silently ignores. Route the merged-proof path through a transaction-aware helper (e.g. compose a single `PathQuery` and call `grove_get_proved_path_query` with the transaction), or remove `TransactionArg` from the signature.
In `packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs:265-283: Merged address-balance proof drops caller `limit`, enabling unbounded proof amplification
Mirror of the nullifier-prover regression: once `boundary_key` exists, the merged proof is generated from `range_from(forward_start..)` with `SizedQuery::new(forward_query, None, None)`. The public handler `query_recent_compacted_address_balance_changes_v0` (packages/rs-drive-abci/src/query/address_funds/recent_compacted_address_balance_changes/v0/mod.rs:31-33) explicitly comments 'Limit the number of compacted entries we return (max 25 to stay within proof size limits)' and passes `limit = Some(25u16)`, but that cap is bypassed here for any non-empty compacted address-balance tree. An attacker can request `prove=true` from an early `start_block_height` and make the node generate and transmit a proof spanning the entire compacted history. Apply a coarse upper-bound at the merged-query level (or fall back to a single non-merged path query whose forward range carries `limit`).
- [SUGGESTION] packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs:265-283: `Some(boundary_key)` branch silently drops `transaction: TransactionArg` in address-balance prover
Same divergence as the nullifier prover: the `Some(boundary_key)` branch calls `self.grove.prove_query_many` (line 277-281) where the second argument is `Option<ProveOptions>`, not `TransactionArg` — the function's `transaction` parameter is dropped. Meanwhile the descending probe at line 228-234 and the `None` branch at line 290 both honor the transaction. Under an active transaction this creates a root-hash inconsistency between step 1's snapshot and the merged proof. Latent today (all production callers pass `None`), but worth fixing now before a future transactional caller appears: route through a transaction-aware proof helper or drop the unused parameter.
In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs:273-387: Untested case: caller queries past the last compacted range
The new tests cover (a) honest roundtrip with compaction, (b) honest containing-range `(100,200)` queried at 150, (c) empty tree, and (d) the malicious skip-descending PoC. The branch where the boundary query authenticates a key `(s, e)` with `e < start_block_height` — i.e. the SDK sync case where a client queries the tip past the last compaction — is not exercised on either prover or verifier side. This branch takes a distinct code path: the prover still emits the merged proof (boundary point + forward range starting at `(start, start)`) and the verifier's generator falls through `find_map` to the `(start, start)` fallback. Add a small unit test (e.g. store `(100, 200)` and query at 300, expecting `Ok` with zero results) to both the nullifier and address-balance verifier test suites.
Adversarial review found that the chained boundary(descending) + forward(ascending) verification cannot be satisfied by a single one-directional GroveDB proof. When >=2 compacted ranges sort at/below the query height (the normal paginated sync case — the query handler hardcodes limit=25 and the SDK loops), the honest prove->verify roundtrip fails with "Cannot verify upper bound of queried range". The original tests only covered single-range and empty cases, so they missed it. Adds two #[ignore]d regression tests reproducing the failure and a "# KNOWN LIVENESS BUG" note on the verifier. The planned fix (deferred) is to re-key compacted entries by (end_block, start_block) so retrieval becomes a single ascending range_from((H,0)..), which also closes the original absence-proof soundness hole structurally. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 16d1e43. The latest delta only adds docs and two #[ignore]d regression tests confirming a critical liveness bug: chaining a descending boundary query with an ascending forward query is unsatisfiable by one one-directional GroveDB proof when >=2 compacted ranges sort at/below the query height (the normal paginated-sync case). All 7 prior findings remain STILL VALID. The author already flagged this as DO NOT MERGE.
🔴 2 blocking | 🟡 1 suggestion(s)
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs:49-61: Acknowledged liveness regression: chained descending-boundary + ascending-forward proof is unsatisfiable in the normal multi-range case
The verifier issues a descending boundary subquery (`Query::new_with_direction(false)`, `range_to_inclusive(..=(start, MAX))`, limit 1) chained with an ascending forward subquery (`range_from(start_key..)`). The prover emits a single ascending merged proof via `prove_query_many`, and a single GroveDB proof is one-directional. Whenever >=2 compacted ranges sort at/below `start_block_height` — the normal paginated sync case since the public handler hardcodes `limit=25` and clients page — honest verification fails with `InvalidProof(... "Cannot verify upper bound of queried range")`. The two new `#[ignore]`d regression tests (`multiple_ranges_below_query_height_verify`, `containing_range_with_two_lower_ranges_verifies`) reproduce this, and the author has self-flagged the PR as DO NOT MERGE. The same direction mismatch is structurally present in the address-balance verifier and prover. Recommended fix per the PR description: re-key compacted entries by `(end_block, start_block)` so retrieval is a single ascending `range_from((H, 0)..)`, which also closes the original soundness hole structurally. Block merge until the rekey lands and the `#[ignore]`d tests pass un-ignored.
In `packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs:76-121: Address-balance verifier has the same one-directional-proof liveness defect, undocumented
The address-balance verifier mirrors the nullifier verifier's chained descending-boundary + ascending-forward construction, and the address-balance prover emits the same single merged `prove_query_many` proof. The PR description acknowledges the bug for nullifiers but the address-balance twin has no `KNOWN LIVENESS BUG` docstring and no analogous `#[ignore]`d regression test. The same realistic scenario — >=2 compacted address-balance ranges at/below `start_block_height` — breaks honest light-client sync of compacted address-balance changes. Fix in lockstep with the nullifier path; either by the same rekey or by an equivalent single-direction reformulation.
In `packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs:249: Address-balance prover uses `.unwrap()` on `try_into` while the nullifier twin uses `?`
`let end_block = u64::from_be_bytes(key[8..16].try_into().unwrap());` is functionally correct since the length is checked to be 16 immediately above. However the parallel nullifier prover (lines 77-81) wraps the same conversion in a `map_err` returning `ProtocolError::CorruptedSerialization`. The two implementations were kept intentionally symmetric (per the docstrings); divergent error-handling style invites copy-paste drift on the next change to this path. Use the nullifier prover's `try_into().map_err(..)?` style for consistency.
| /// # KNOWN LIVENESS BUG (tracked in PR #3792 — fix deferred) | ||
| /// | ||
| /// The boundary query is **descending** (greatest key `<= bound`) while the | ||
| /// forward query is **ascending** (paginated `range_from`). A single GroveDB | ||
| /// proof is one-directional, so when **two or more** compacted ranges sort | ||
| /// at/below `start_block_height` the honest proof fails verification with | ||
| /// "Cannot verify upper bound of queried range" (see the two `#[ignore]`d | ||
| /// `*_below_query_height*` regression tests). The single-range and empty | ||
| /// cases work, which is why this slipped the original tests. The planned fix | ||
| /// is to re-key compacted entries by `(end_block, start_block)` so retrieval | ||
| /// becomes a single ascending `range_from((H, 0)..)` — which also closes the | ||
| /// original absence-proof soundness hole structurally (the containing range | ||
| /// becomes an ordinary in-range result that cannot be omitted). |
There was a problem hiding this comment.
🔴 Blocking: Acknowledged liveness regression: chained descending-boundary + ascending-forward proof is unsatisfiable in the normal multi-range case
The verifier issues a descending boundary subquery (Query::new_with_direction(false), range_to_inclusive(..=(start, MAX)), limit 1) chained with an ascending forward subquery (range_from(start_key..)). The prover emits a single ascending merged proof via prove_query_many, and a single GroveDB proof is one-directional. Whenever >=2 compacted ranges sort at/below start_block_height — the normal paginated sync case since the public handler hardcodes limit=25 and clients page — honest verification fails with InvalidProof(... "Cannot verify upper bound of queried range"). The two new #[ignore]d regression tests (multiple_ranges_below_query_height_verify, containing_range_with_two_lower_ranges_verifies) reproduce this, and the author has self-flagged the PR as DO NOT MERGE. The same direction mismatch is structurally present in the address-balance verifier and prover. Recommended fix per the PR description: re-key compacted entries by (end_block, start_block) so retrieval is a single ascending range_from((H, 0)..), which also closes the original soundness hole structurally. Block merge until the rekey lands and the #[ignore]d tests pass un-ignored.
source: ['claude', 'codex']
There was a problem hiding this comment.
Acknowledged and confirmed — this is the known liveness bug the PR description flags as DO NOT MERGE. The fix (re-key compacted entries by (end_block, start_block) so retrieval is a single ascending range_from((H,0)..), which also closes the original soundness hole structurally) is deferred by the maintainer. The two #[ignore]d regression tests reproduce it. Leaving this thread open until the re-key lands and those tests pass un-ignored.
There was a problem hiding this comment.
Resolved in 2191680 — Acknowledged liveness regression: chained descending-boundary + ascending-forward proof is unsatisfiable in the normal multi-range case no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Step 1: boundary query — authenticate the single greatest compacted | ||
| // key <= (start_block_height, u64::MAX). Descending, limit 1. | ||
| let boundary_end_key = compacted_key(start_block_height, u64::MAX); | ||
| let mut boundary_inner = Query::new_with_direction(false); // descending | ||
| boundary_inner.insert_range_to_inclusive(..=boundary_end_key); | ||
| let boundary_query = | ||
| PathQuery::new(path.clone(), SizedQuery::new(boundary_inner, Some(1), None)); | ||
|
|
||
| // Step 2: generator — derive the forward query's lower bound from the | ||
| // AUTHENTICATED boundary result (not from raw proof bytes). | ||
| let forward_path = path.clone(); | ||
| let generator = | ||
| move |boundary_results: Vec<PathKeyOptionalElementTrio>| -> Option<PathQuery> { | ||
| let start_key = boundary_results | ||
| .iter() | ||
| .find_map(|(_path, key, _element)| { | ||
| if key.len() != 16 { | ||
| return None; | ||
| } | ||
| let end_block = u64::from_be_bytes( | ||
| key[8..16].try_into().expect("len checked to be 16"), | ||
| ); | ||
| if end_block >= start_block_height { | ||
| Some(key.clone()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .transpose()? | ||
| .unwrap_or_default() | ||
| } | ||
| }; | ||
|
|
||
| // Look for a KV entry where the range contains start_block_height | ||
| // Keys are 16 bytes: (start_block, end_block), both big-endian | ||
| let containing_key = kv_entries.iter().find_map(|(key, _)| { | ||
| if key.len() != 16 { | ||
| return None; | ||
| } | ||
| let range_start = u64::from_be_bytes(key[0..8].try_into().unwrap()); | ||
| let range_end = u64::from_be_bytes(key[8..16].try_into().unwrap()); | ||
|
|
||
| // Check if this range contains start_block_height | ||
| if range_start <= start_block_height && start_block_height <= range_end { | ||
| Some(key.clone()) | ||
| } else { | ||
| None | ||
| } | ||
| }); | ||
|
|
||
| // Determine the start_key for the query | ||
| // Use the containing range's key if found, otherwise (start_block_height, start_block_height) | ||
| let start_key = containing_key.unwrap_or_else(|| { | ||
| let mut key = Vec::with_capacity(16); | ||
| key.extend_from_slice(&start_block_height.to_be_bytes()); | ||
| key.extend_from_slice(&start_block_height.to_be_bytes()); | ||
| key | ||
| }); | ||
|
|
||
| // Verify the proof and get results using subset query | ||
| let path = vec![ | ||
| vec![RootTree::SavedBlockTransactions as u8], | ||
| vec![COMPACTED_ADDRESS_BALANCES_KEY_U8], | ||
| ]; | ||
|
|
||
| let mut query = Query::new(); | ||
| query.insert_range_from(start_key..); | ||
|
|
||
| let path_query = PathQuery::new(path, SizedQuery::new(query, limit, None)); | ||
| .unwrap_or_else(|| compacted_key(start_block_height, start_block_height)); | ||
|
|
||
| let mut forward_inner = Query::new(); | ||
| forward_inner.insert_range_from(start_key..); | ||
| Some(PathQuery::new( | ||
| forward_path.clone(), | ||
| SizedQuery::new(forward_inner, limit, None), | ||
| )) | ||
| }; | ||
|
|
||
| let (root_hash, proved_key_values) = GroveDb::verify_subset_query( | ||
| // Step 3: verify the chained queries. GroveDB enforces that every | ||
| // sub-query binds to the SAME root hash. | ||
| let (root_hash, mut results) = GroveDb::verify_query_with_chained_path_queries( | ||
| proof, | ||
| &path_query, | ||
| &boundary_query, | ||
| vec![generator], | ||
| &platform_version.drive.grove_version, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Address-balance verifier has the same one-directional-proof liveness defect, undocumented
The address-balance verifier mirrors the nullifier verifier's chained descending-boundary + ascending-forward construction, and the address-balance prover emits the same single merged prove_query_many proof. The PR description acknowledges the bug for nullifiers but the address-balance twin has no KNOWN LIVENESS BUG docstring and no analogous #[ignore]d regression test. The same realistic scenario — >=2 compacted address-balance ranges at/below start_block_height — breaks honest light-client sync of compacted address-balance changes. Fix in lockstep with the nullifier path; either by the same rekey or by an equivalent single-direction reformulation.
source: ['claude', 'codex']
There was a problem hiding this comment.
Addressed the documentation gap in 2191680: added the matching KNOWN LIVENESS BUG docstring and an #[ignore]d multiple_ranges_below_query_height_verify regression test to the address-balance verifier (previously only documented on the nullifier path). The underlying fix (the same re-key) is deferred along with the nullifier path, so leaving this thread open.
There was a problem hiding this comment.
Resolved in 2191680 — Address-balance verifier has the same one-directional-proof liveness defect, undocumented no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…back) Addresses automated review findings on the compacted absence-proof PR (the liveness bug itself remains deferred to the planned re-key): - BLOCKING (DoS): the Some(boundary_key) prover branch built the forward query with no limit and proved it via prove_query_many, dropping the caller's `limit`. The public handlers pass limit=25 "to stay within proof size limits"; bypassing it let an unauthenticated `prove=true` request from an early height force a proof over the entire compacted history (remote CPU/memory/bandwidth amplification). Both provers now prove a single query (boundary key + forward range) capped at `limit + 1` (the +1 covers the authenticated boundary point). The verifier already re-applies `limit` to its forward subset query, so the cap is soundness-neutral. - The same branch is now routed through the transaction-aware `grove_get_proved_path_query` instead of `prove_query_many` (which ignores the TransactionArg), removing a latent root-hash inconsistency under an active transaction. - Address-balance prover: use `try_into().map_err(..)?` instead of `.unwrap()` to match the nullifier prover. - Address-balance verifier: add the matching `KNOWN LIVENESS BUG` docstring and an `#[ignore]`d multi-range regression test (the bug was only documented on the nullifier path). - Add `query_past_last_range_returns_empty` tests (store (100,200), query at 300 -> Ok with zero results) to both verifiers, covering the no-containing-range fallback that was previously untested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs (1)
32-35: 💤 Low valueDocstring references outdated
prove_query_manyapproach.The docstring mentions emitting "a single merged proof (
prove_query_many)", but the implementation now builds a singlePathQuerywithinsert_key+insert_range_fromand callsgrove_get_proved_path_query. Consider updating to reflect the current approach.📝 Suggested doc update
/// We discover `start_key` with a non-proving descending query, then emit a - /// single merged proof (`prove_query_many`) that covers BOTH the boundary + /// single proof (via a combined `PathQuery`) that covers BOTH the boundary /// key and the forward range so the verifier's chained queries are both /// satisfiable.🤖 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 `@packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs` around lines 32 - 35, The docstring is outdatedly referencing a merged proof via `prove_query_many`; update it to describe the current behavior: we construct a single `PathQuery` containing `insert_key` and `insert_range_from` (discovered by the non‑proving descending query from `start_key`) and then call `grove_get_proved_path_query` to produce the proof. Replace any mention of `prove_query_many` with this `PathQuery` + `grove_get_proved_path_query` workflow and briefly note that the produced proof covers both the boundary key and the forward range for verifier chaining.
🤖 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
`@packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs`:
- Around line 32-35: The docstring is outdatedly referencing a merged proof via
`prove_query_many`; update it to describe the current behavior: we construct a
single `PathQuery` containing `insert_key` and `insert_range_from` (discovered
by the non‑proving descending query from `start_key`) and then call
`grove_get_proved_path_query` to produce the proof. Replace any mention of
`prove_query_many` with this `PathQuery` + `grove_get_proved_path_query`
workflow and briefly note that the produced proof covers both the boundary key
and the forward range for verifier chaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d747b17-7ab7-4000-a2d5-2e897c26f02a
📒 Files selected for processing (4)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs
- packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
…oach Both `prove_compacted_*_changes_v0` docstrings still described the old `prove_query_many` merged-proof approach, but commit 2191680 switched the Some(boundary_key) branch to a single combined `PathQuery` (boundary key via `insert_key` + forward `range_from`, capped at the caller's limit) proved through `grove_get_proved_path_query`. Updated both docstrings to match. Doc-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verifier review at head 2191680. Latest delta (2191680 'cap compacted-proof size + honor transaction') correctly addresses six prior findings: caller-limit is capped via saturating_add(1) on both provers, both Some(boundary_key) branches route through grove_get_proved_path_query so the TransactionArg is honored, the address-balance .unwrap() was replaced with map_err, query_past_last_range_returns_empty tests exist on both verifiers, and the address-balance verifier carries the parallel KNOWN LIVENESS BUG docstring + #[ignore]d mirror regression test. Two blocking liveness defects remain carried forward (chained descending-boundary + ascending-forward proof is unsatisfiable for one one-directional GroveDB proof on both nullifier and address-balance verifiers; the author self-flags DO NOT MERGE pending an (end_block, start_block) re-keying refactor) plus two nitpicks (duplicated compacted_key helper across four modules, unreachable results.pop() mis-mapped to CorruptedProof). No new defects introduced by the latest delta.
💬 1 nitpick(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…l, not CorruptedProof The compacted verifiers used `results.pop().ok_or_else(|| CorruptedProof)` to extract the forward result set. Because the single chained generator always returns `Some`, `verify_query_with_chained_path_queries` always yields exactly two result sets, so that branch is unreachable from caller input — it can only fire on a GroveDB-internal invariant break. Mapping it to `ProofError::CorruptedProof` (a user-facing "your proof is corrupt" error) therefore misclassified an internal-API violation as caller proof corruption. Both verifiers now destructure `let [_boundary, forward_results] = results.try_into().map_err(..)?`, which makes the [boundary, forward] cardinality explicit at the type level and reports any deviation as `DriveError::CorruptedCodeExecution` (an internal-invariant error). No behavior change on any reachable path; all verify_compacted tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at a7c31c5. Latest delta (a7c31c5) correctly reclassifies the chained-verify cardinality break as DriveError::CorruptedCodeExecution via try_into destructure on both verifiers, resolving the prior results.pop() misclassification nit. No new defects introduced by the delta. Two carried-forward blocking liveness defects remain on both compacted-proof verifiers (descending boundary + ascending forward over a single one-directional GroveDB proof is unsatisfiable when ≥2 ranges sort at/below start_block_height), plus the duplicated compacted_key helper nitpick across all four touched modules. Author self-flags DO NOT MERGE pending the planned (end_block, start_block) rekey.
🔴 2 blocking | 💬 1 nitpick(s)
3 additional finding(s)
blocking: Chained descending-boundary + ascending-forward proof is unsatisfiable in normal multi-range sync (nullifier verifier)
packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs (line 73)
The verifier issues a descending boundary subquery (Query::new_with_direction(false), insert_range_to_inclusive(..=(start, MAX)), limit 1) at lines 73-77 chained with an ascending forward subquery (insert_range_from(start_key..)) at lines 105-110, all bound to one GroveDB proof at lines 117-122. The matching prover emits a single ascending merged proof over insert_key(boundary_key) ∪ insert_range_from(forward_start..). A single GroveDB proof is one-directional, so whenever ≥2 compacted ranges sort at/below start_block_height — the normal paginated-sync case since the public handler hardcodes limit=25 and SDK clients page — honest verification fails with InvalidProof(..., "Cannot verify upper bound of queried range"). Reproduced by the #[ignore]d multiple_ranges_below_query_height_verify and containing_range_with_two_lower_ranges_verifies regressions added in this PR, and explicitly documented as a KNOWN LIVENESS BUG at lines 50-62. Security impact beyond liveness: trust-minimized clients (prove=true, the only authenticated path to detect drained-recent-tree spends from the compacted tail) are forced to stall or fall back to unauthenticated sync, which structurally re-enables the very absence-proof concealment hole this PR set out to close. Block merge until both #[ignore]d regressions pass un-ignored — the planned (end_block, start_block) rekey (single ascending range_from((H, 0)..)) closes this and the original soundness hole in one structural change.
blocking: Address-balance verifier carries the same one-directional-proof liveness defect
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (line 92)
Mirror of the nullifier defect: descending boundary at lines 92-96, ascending forward at lines 120-125, chained verify at lines 130-135, single-direction merged prover at fetch_compacted_address_balances/v0/mod.rs. Once ≥2 compacted address-balance ranges sort at/below start_block_height, honest verification fails with the same Cannot verify upper bound of queried range error — reproduced by the #[ignore]d multiple_ranges_below_query_height_verify regression and documented as KNOWN LIVENESS BUG at lines 66-77. The public compacted address-balance endpoint also paginates at limit=25, so the same DoS-on-authenticated-sync / fallback-to-unauthenticated consequence applies, re-enabling the absence-proof concealment hole this PR set out to close for the address-balance vertical. Fix in lockstep with the nullifier path via the same (end_block, start_block) rekey — both verticals share the identical compacted key shape, so it should be one coherent change.
nitpick: compacted_key helper duplicated verbatim across all four touched modules
packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs (line 12)
The identical 6-line compacted_key(start_block, end_block) -> Vec<u8> helper exists as a private free function in all four modules touched by this PR: verify_compacted_nullifier_changes/v0/mod.rs:13, verify_compacted_address_balance_changes/v0/mod.rs:20, drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs, and drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs (confirmed via grep — 4 hits). The 16-byte boundary key encoding is part of the proof contract; drift between any prover and its matching verifier would silently break chained verification (a correctness, not style, concern). With the planned (end_block, start_block) rekey on the horizon, centralizing this in a single shared private helper before that change lands eliminates the four-place rewrite footgun and yields a smaller, safer follow-up diff.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:73-122: Chained descending-boundary + ascending-forward proof is unsatisfiable in normal multi-range sync (nullifier verifier)
The verifier issues a descending boundary subquery (`Query::new_with_direction(false)`, `insert_range_to_inclusive(..=(start, MAX))`, limit 1) at lines 73-77 chained with an ascending forward subquery (`insert_range_from(start_key..)`) at lines 105-110, all bound to one GroveDB proof at lines 117-122. The matching prover emits a single ascending merged proof over `insert_key(boundary_key) ∪ insert_range_from(forward_start..)`. A single GroveDB proof is one-directional, so whenever ≥2 compacted ranges sort at/below `start_block_height` — the normal paginated-sync case since the public handler hardcodes `limit=25` and SDK clients page — honest verification fails with `InvalidProof(..., "Cannot verify upper bound of queried range")`. Reproduced by the `#[ignore]`d `multiple_ranges_below_query_height_verify` and `containing_range_with_two_lower_ranges_verifies` regressions added in this PR, and explicitly documented as a `KNOWN LIVENESS BUG` at lines 50-62. Security impact beyond liveness: trust-minimized clients (`prove=true`, the only authenticated path to detect drained-recent-tree spends from the compacted tail) are forced to stall or fall back to unauthenticated sync, which structurally re-enables the very absence-proof concealment hole this PR set out to close. Block merge until both `#[ignore]`d regressions pass un-ignored — the planned `(end_block, start_block)` rekey (single ascending `range_from((H, 0)..)`) closes this and the original soundness hole in one structural change.
- [BLOCKING] In `packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:92-135: Address-balance verifier carries the same one-directional-proof liveness defect
Mirror of the nullifier defect: descending boundary at lines 92-96, ascending forward at lines 120-125, chained verify at lines 130-135, single-direction merged prover at `fetch_compacted_address_balances/v0/mod.rs`. Once ≥2 compacted address-balance ranges sort at/below `start_block_height`, honest verification fails with the same `Cannot verify upper bound of queried range` error — reproduced by the `#[ignore]`d `multiple_ranges_below_query_height_verify` regression and documented as `KNOWN LIVENESS BUG` at lines 66-77. The public compacted address-balance endpoint also paginates at `limit=25`, so the same DoS-on-authenticated-sync / fallback-to-unauthenticated consequence applies, re-enabling the absence-proof concealment hole this PR set out to close for the address-balance vertical. Fix in lockstep with the nullifier path via the same `(end_block, start_block)` rekey — both verticals share the identical compacted key shape, so it should be one coherent change.
- [NITPICK] In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:12-18: compacted_key helper duplicated verbatim across all four touched modules
The identical 6-line `compacted_key(start_block, end_block) -> Vec<u8>` helper exists as a private free function in all four modules touched by this PR: `verify_compacted_nullifier_changes/v0/mod.rs:13`, `verify_compacted_address_balance_changes/v0/mod.rs:20`, `drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs`, and `drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs` (confirmed via grep — 4 hits). The 16-byte boundary key encoding is part of the proof contract; drift between any prover and its matching verifier would silently break chained verification (a correctness, not style, concern). With the planned `(end_block, start_block)` rekey on the horizon, centralizing this in a single shared private helper before that change lands eliminates the four-place rewrite footgun and yields a smaller, safer follow-up diff.
Inline comments were not duplicated because the still-valid findings already have open review threads; this is the cumulative review verdict for a7c31c5.
The 16-byte big-endian `(start_block, end_block)` compacted-key encoding was duplicated verbatim as a private `compacted_key` fn in all four touched modules (both provers and both verifiers). Since that encoding is part of the chained-proof contract, drift between any prover and its matching verifier would silently break chained verification — a correctness concern, not style. Moved it to a single `crate::util::common::compacted_key` (pub(crate)) and pointed all four modules at it via `use`. This also shrinks the planned `(end_block, start_block)` re-key to a one-place change instead of a four-place rewrite. No behavior change; all verify_compacted tests pass (13 passed, 3 ignored). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs (1)
52-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor
limit = Some(0)before the boundary probe.
limitis only enforced after Query 1 can already push a containing range, sofetch_compacted_address_balance_changes_v0(..., Some(0), ...)returns one entry wheneverstart_block_heightfalls inside a compacted range. Zero-limit callers should get an empty vector immediately.Suggested fix
let config = bincode::config::standard() .with_big_endian() .with_no_limit(); let mut compacted_changes = Vec::new(); let limit_usize = limit.map(|l| l as usize); + + if limit_usize == Some(0) { + return Ok(compacted_changes); + } // Query 1: Find if there's a range containing start_block_height // Query descending from (start_block_height, u64::MAX) with limit 1Also applies to: 112-117
🤖 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 `@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs` around lines 52 - 65, The function fetch_compacted_address_balance_changes_v0 currently builds and runs a boundary probe (using desc_query/desc_path_query and later the ascending probe) before enforcing the caller's limit, so callers with limit = Some(0) still get a result; add an early return: check limit_usize (derived from limit) and if it is Some(0) return Ok(Vec::new()) immediately before constructing/running the desc_query probe (and apply the same early-return placement before the second probe logic around the code at the second location referenced by the reviewer). Ensure you reference and use the existing limit_usize variable and exit before creating desc_end_key/desc_query/desc_path_query (and the analogous ascending-range code at the other spot).packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (1)
95-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed authenticated boundary keys instead of falling back.
If the boundary result carries a non-16-byte key, this
find_mappath returnsNoneandunwrap_or_elsesilently drops to(start_block_height, start_block_height). That degrades verification back to the forward-only lower bound and can accept an incomplete result set instead of classifying the proof/state as corrupted.🤖 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 `@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs` around lines 95 - 111, The closure that computes start_key currently silently falls back when any boundary key is malformed; change it to reject malformed authenticated boundary keys by adding an early check: scan boundary_results (PathKeyOptionalElementTrio) and if any key.len() != 16 return None from the closure immediately (treat proof/state as corrupted), otherwise proceed with the existing find_map logic and compacted_key fallback; update the closure around start_key so malformed keys cause an immediate None instead of reaching the unwrap_or_else fallback.packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs (1)
96-113:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe merged proof shape still has the documented multi-range liveness failure.
This
insert_key(boundary_key)+insert_range_from(forward_start..)branch proves a single ascending union query, but the verifier inpackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs:64-115still authenticates the boundary via a descendingrange_to_inclusive(..=(start, MAX))subset first. The PR notes that the new multi-range regression tests are still ignored for exactly this direction mismatch, so honest compacted-nullifier proofs can still fail verification in those cases. Please fix the proof shape before merge instead of deferring it behind ignored regressions.🤖 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 `@packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs` around lines 96 - 113, The proof shape is built as an ascending union (using Query::insert_key(boundary_key) + Query::insert_range_from(forward_start..)) but the verifier in verify_compacted_nullifier_changes expects the boundary authenticated via a descending subset (range_to_inclusive(..=(start, MAX))). Change the prover query construction in PathQuery::new(SizedQuery::new(...)) so it emits the same descending union the verifier checks: replace the insert_range_from(forward_start..) call with the corresponding descending range insertion (use Query::insert_range_to_inclusive or the API that creates a ..= (start, MAX) range) while preserving the capped_limit and inclusion of boundary_key, so the prover's Query, the SizedQuery, and the PathQuery match the verifier's range direction and shape.
🧹 Nitpick comments (2)
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (1)
10-27: ⚡ Quick winReuse the shared compacted-balances path helper here.
This local
b'c'path duplicate has to stay byte-identical to the subtree used by the prover/storage side inpackages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rsand to the helper already used by this file's test insert path at Lines 306-307. Calling the sharedDrivehelper keeps proof generation, verification, and direct test inserts on one contract.Suggested refactor
-use crate::drive::RootTree; @@ -/// The subtree key for compacted address balances storage as u8 -const COMPACTED_ADDRESS_BALANCES_KEY_U8: u8 = b'c'; @@ fn compacted_address_balances_path() -> Vec<Vec<u8>> { - vec![ - vec![RootTree::SavedBlockTransactions as u8], - vec![COMPACTED_ADDRESS_BALANCES_KEY_U8], - ] + Drive::saved_compacted_block_transactions_address_balances_path() }🤖 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 `@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs` around lines 10 - 27, The file defines a local COMPACTED_ADDRESS_BALANCES_KEY_U8 and compacted_address_balances_path which duplicate the shared subtree key/path; replace this duplication by using the existing Drive helper used by fetch_compacted_address_balances (the shared compacted-balances path helper) instead of defining COMPACTED_ADDRESS_BALANCES_KEY_U8 and the local compacted_address_balances_path function so proof generation, verification, and tests use the single canonical path; update references in this module (e.g., where compacted_address_balances_path() is called) to call the shared Drive helper function used in packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs and remove the duplicated constant and function.packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs (1)
57-59: ⚡ Quick winUse
compacted_keyfor the fetch bounds too.These two manual 16-byte builders are now the last encoding duplicates in this module, while the proof path already routes through
crate::util::common::compacted_key. Reusing the helper here keeps the read path in lockstep with the prover/verifier contract if the deferred(end_block, start_block)re-key lands later.Suggested refactor
- let mut desc_end_key = Vec::with_capacity(16); - desc_end_key.extend_from_slice(&start_block_height.to_be_bytes()); - desc_end_key.extend_from_slice(&u64::MAX.to_be_bytes()); + let desc_end_key = compacted_key(start_block_height, u64::MAX); @@ - let mut asc_start_key = Vec::with_capacity(16); - asc_start_key.extend_from_slice(&start_block_height.to_be_bytes()); - asc_start_key.extend_from_slice(&0u64.to_be_bytes()); + let asc_start_key = compacted_key(start_block_height, 0);Also applies to: 123-125
🤖 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 `@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs` around lines 57 - 59, Replace the manual 16-byte key builders with the existing compacted_key helper to construct fetch bounds so the read path matches the prover/verifier encoding; specifically, where desc_end_key/desc_start_key (and the similar asc_* keys around the later occurrence) are built from start_block_height/end_block_height and u64::MAX/0, call crate::util::common::compacted_key((end_block, start_block)) or the appropriate (start,end) tuple instead of manually extending bytes; update the call sites that currently extend_from_slice on to_be_bytes() to use compacted_key for both the desc_* and asc_* bounds so encoding stays consistent with compacted_key.
🤖 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.
Outside diff comments:
In
`@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs`:
- Around line 52-65: The function fetch_compacted_address_balance_changes_v0
currently builds and runs a boundary probe (using desc_query/desc_path_query and
later the ascending probe) before enforcing the caller's limit, so callers with
limit = Some(0) still get a result; add an early return: check limit_usize
(derived from limit) and if it is Some(0) return Ok(Vec::new()) immediately
before constructing/running the desc_query probe (and apply the same
early-return placement before the second probe logic around the code at the
second location referenced by the reviewer). Ensure you reference and use the
existing limit_usize variable and exit before creating
desc_end_key/desc_query/desc_path_query (and the analogous ascending-range code
at the other spot).
In
`@packages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rs`:
- Around line 96-113: The proof shape is built as an ascending union (using
Query::insert_key(boundary_key) + Query::insert_range_from(forward_start..)) but
the verifier in verify_compacted_nullifier_changes expects the boundary
authenticated via a descending subset (range_to_inclusive(..=(start, MAX))).
Change the prover query construction in PathQuery::new(SizedQuery::new(...)) so
it emits the same descending union the verifier checks: replace the
insert_range_from(forward_start..) call with the corresponding descending range
insertion (use Query::insert_range_to_inclusive or the API that creates a ..=
(start, MAX) range) while preserving the capped_limit and inclusion of
boundary_key, so the prover's Query, the SizedQuery, and the PathQuery match the
verifier's range direction and shape.
In
`@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:
- Around line 95-111: The closure that computes start_key currently silently
falls back when any boundary key is malformed; change it to reject malformed
authenticated boundary keys by adding an early check: scan boundary_results
(PathKeyOptionalElementTrio) and if any key.len() != 16 return None from the
closure immediately (treat proof/state as corrupted), otherwise proceed with the
existing find_map logic and compacted_key fallback; update the closure around
start_key so malformed keys cause an immediate None instead of reaching the
unwrap_or_else fallback.
---
Nitpick comments:
In
`@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs`:
- Around line 57-59: Replace the manual 16-byte key builders with the existing
compacted_key helper to construct fetch bounds so the read path matches the
prover/verifier encoding; specifically, where desc_end_key/desc_start_key (and
the similar asc_* keys around the later occurrence) are built from
start_block_height/end_block_height and u64::MAX/0, call
crate::util::common::compacted_key((end_block, start_block)) or the appropriate
(start,end) tuple instead of manually extending bytes; update the call sites
that currently extend_from_slice on to_be_bytes() to use compacted_key for both
the desc_* and asc_* bounds so encoding stays consistent with compacted_key.
In
`@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:
- Around line 10-27: The file defines a local COMPACTED_ADDRESS_BALANCES_KEY_U8
and compacted_address_balances_path which duplicate the shared subtree key/path;
replace this duplication by using the existing Drive helper used by
fetch_compacted_address_balances (the shared compacted-balances path helper)
instead of defining COMPACTED_ADDRESS_BALANCES_KEY_U8 and the local
compacted_address_balances_path function so proof generation, verification, and
tests use the single canonical path; update references in this module (e.g.,
where compacted_address_balances_path() is called) to call the shared Drive
helper function used in
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rs
and remove the duplicated constant and function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a319c61-e899-4884-a3fa-584a209a933d
📒 Files selected for processing (5)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_address_balances/v0/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_compacted_nullifier_changes/v0/mod.rspackages/rs-drive/src/util/common/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 98a62db. The latest delta cleanly resolves the prior helper-duplication nitpick by centralizing compacted_key(start, end) into crate::util::common (pub(crate)), with all four prove/verify modules importing it; no new defects introduced. The two prior blocking liveness defects on the compacted-changes verifiers remain STILL VALID: both chain a descending boundary subquery with an ascending forward subquery against a single one-directional GroveDB proof, which is unsatisfiable whenever ≥2 compacted ranges sort at/below start_block_height (the normal paginated-sync case at limit=25). The author self-flags DO NOT MERGE pending the planned (end_block, start_block) re-keying.
🔴 2 blocking
2 additional finding(s)
blocking: STILL VALID — chained descending-boundary + ascending-forward proof is unsatisfiable in normal multi-range sync (nullifier verifier)
packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs (line 56)
At head 98a62db the verifier still issues a descending boundary subquery (Query::new_with_direction(false) at line 67 with insert_range_to_inclusive(..=(start_block_height, u64::MAX)) at line 68, limit 1 at line 70) chained with an ascending forward subquery (insert_range_from(start_key..) at line 99), both bound to one GroveDB proof via verify_query_with_chained_path_queries at lines 110–115. The matching prover emits a single ascending merged proof, but a single GroveDB proof is one-directional, so whenever ≥2 compacted ranges sort at/below start_block_height — the normal paginated-sync case since query_recent_compacted_nullifier_changes_v0 hardcodes limit=25 and SDK clients page — honest verification fails with InvalidProof(..., "Cannot verify upper bound of queried range"). Both reviewer pipelines reproduced this at the current SHA via the #[ignore]d multiple_ranges_below_query_height_verify and containing_range_with_two_lower_ranges_verifies regressions added in this PR, and the defect is explicitly documented as KNOWN LIVENESS BUG at lines 43–55. Beyond liveness, trust-minimized clients on prove=true (the only authenticated path to detect drained-recent-tree spends from the compacted tail) are forced either to stall (DoS the authenticated discovery path) or fall back to unauthenticated sync, structurally re-enabling the very absence-proof concealment hole this PR set out to close. Block merge until both #[ignore]d regressions pass un-ignored — the planned (end_block, start_block) re-keying (single ascending range_from((H, 0)..)) closes this liveness defect AND the original soundness hole in one structural change.
blocking: STILL VALID — address-balance verifier carries the same one-directional-proof liveness defect
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (line 71)
Mirror of the nullifier defect at head 98a62db: descending boundary at lines 85–89, ascending forward at lines 113–118, chained verify at lines 123–128; the address-balance prover in fetch_compacted_address_balances/v0/mod.rs emits the same single-direction merged proof. The KNOWN LIVENESS BUG docstring at lines 59–70 and the #[ignore]d multiple_ranges_below_query_height_verify regression are in place and confirmed failing at this SHA with InvalidProof(..., "Cannot verify upper bound of queried range") once ≥2 compacted address-balance ranges sort at/below start_block_height. The public compacted address-balance endpoint also paginates at limit=25, so the same DoS-on-authenticated-sync / fallback-to-unauthenticated consequence applies, re-enabling the absence-proof concealment hole for the address-balance vertical (an attacker can hide balance changes from light clients that rely on prove=true). Fix in lockstep with the nullifier path via the same (end_block, start_block) re-keying — both verticals share the identical compacted key shape, so it should be one coherent change.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:56-127: STILL VALID — chained descending-boundary + ascending-forward proof is unsatisfiable in normal multi-range sync (nullifier verifier)
At head 98a62db6 the verifier still issues a descending boundary subquery (`Query::new_with_direction(false)` at line 67 with `insert_range_to_inclusive(..=(start_block_height, u64::MAX))` at line 68, limit 1 at line 70) chained with an ascending forward subquery (`insert_range_from(start_key..)` at line 99), both bound to one GroveDB proof via `verify_query_with_chained_path_queries` at lines 110–115. The matching prover emits a single ascending merged proof, but a single GroveDB proof is one-directional, so whenever ≥2 compacted ranges sort at/below `start_block_height` — the normal paginated-sync case since `query_recent_compacted_nullifier_changes_v0` hardcodes `limit=25` and SDK clients page — honest verification fails with `InvalidProof(..., "Cannot verify upper bound of queried range")`. Both reviewer pipelines reproduced this at the current SHA via the `#[ignore]`d `multiple_ranges_below_query_height_verify` and `containing_range_with_two_lower_ranges_verifies` regressions added in this PR, and the defect is explicitly documented as `KNOWN LIVENESS BUG` at lines 43–55. Beyond liveness, trust-minimized clients on `prove=true` (the only authenticated path to detect drained-recent-tree spends from the compacted tail) are forced either to stall (DoS the authenticated discovery path) or fall back to unauthenticated sync, structurally re-enabling the very absence-proof concealment hole this PR set out to close. Block merge until both `#[ignore]`d regressions pass un-ignored — the planned `(end_block, start_block)` re-keying (single ascending `range_from((H, 0)..)`) closes this liveness defect AND the original soundness hole in one structural change.
- [BLOCKING] In `packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:71-140: STILL VALID — address-balance verifier carries the same one-directional-proof liveness defect
Mirror of the nullifier defect at head 98a62db6: descending boundary at lines 85–89, ascending forward at lines 113–118, chained verify at lines 123–128; the address-balance prover in `fetch_compacted_address_balances/v0/mod.rs` emits the same single-direction merged proof. The `KNOWN LIVENESS BUG` docstring at lines 59–70 and the `#[ignore]`d `multiple_ranges_below_query_height_verify` regression are in place and confirmed failing at this SHA with `InvalidProof(..., "Cannot verify upper bound of queried range")` once ≥2 compacted address-balance ranges sort at/below `start_block_height`. The public compacted address-balance endpoint also paginates at `limit=25`, so the same DoS-on-authenticated-sync / fallback-to-unauthenticated consequence applies, re-enabling the absence-proof concealment hole for the address-balance vertical (an attacker can hide balance changes from light clients that rely on `prove=true`). Fix in lockstep with the nullifier path via the same `(end_block, start_block)` re-keying — both verticals share the identical compacted key shape, so it should be one coherent change.
Inline findings were already present on older review threads, so I carried the verified current-head findings forward in this exact-SHA top-level review body instead of duplicating inline comments.
…dary, shared path/key) Addresses the automated-review quick-wins on the compacted-proof code (the liveness bug itself remains deferred to the planned re-key): - fetch_compacted_address_balances: return an empty vec immediately when `limit == Some(0)` — previously a zero-limit caller still got one entry whenever start_block_height fell inside a compacted range (the limit was only enforced after the boundary probe). - Both verifiers: reject a malformed (non-16-byte) AUTHENTICATED boundary key by returning `None` from the chained-query generator (which fails verification) instead of silently degrading to the (start, start) forward-only lower bound and accepting an incomplete result set. - verify_compacted_address_balance_changes: drop the duplicated local `COMPACTED_ADDRESS_BALANCES_KEY_U8` const + `compacted_address_balances_path` fn (and the now-unused RootTree import); use the canonical `Drive::saved_compacted_block_transactions_address_balances_path_vec()` so proof gen, verification, and test inserts share one path contract. - fetch_compacted_address_balances: build the desc/asc query bounds with the shared `compacted_key` helper instead of manual byte builders. No behavior change on existing paths; 30 compacted tests pass (3 ignored liveness repros). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
At 404f1be both prior blocking liveness defects in the compacted-changes verifiers (shielded nullifier + address balance) remain STILL VALID and are convergently flagged by both agents. The descending limit-1 boundary subquery chained with an ascending forward subquery cannot be satisfied by a single one-directional GroveDB proof whenever ≥2 compacted ranges sort at/below start_block_height — the normal paginated-sync case at limit=25 — reproduced by the two #[ignore]d regressions added in this PR and self-flagged DO NOT MERGE by the author. The 404f1be hardening delta (limit=0 short-circuit, shared compacted_key helper, canonical path-vec, malformed-boundary-key rejection) resolved three prior findings cleanly and introduced no new defects, but exposed a stale comment + miscategorized error class on the post-chain cardinality destructure that now mishandles attacker-supplied malformed boundary keys as CorruptedCodeExecution instead of CorruptedProof.
🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs:122-133: Stale comment + miscategorized error after new malformed-boundary-key generator-None path
The 404f1beb delta added `if boundary_results.iter().any(|(_p, key, _e)| key.len() != 16) { return None; }` at lines 87–89 (mirrored at address-balance verifier lines 89–91), correctly refusing to silently degrade to the `(start, start)` fallback when the authenticated boundary key is malformed. That introduces a new path in which `generator` returns `None` for caller-supplied (proof-derived) reasons. The destructure at lines 128–133 maps a non-2 cardinality to `Error::Drive(DriveError::CorruptedCodeExecution("chained verification invariant: expected [boundary, forward] result sets"))`. The adjacent comment at lines 123–127 still asserts "A different cardinality can only be a GroveDB-internal invariant break (never caller-supplied proof corruption)" — that assertion is now false, since a malicious node can craft a proof whose authenticated boundary key has the wrong length and trigger this exact path. The user-facing error class is therefore wrong (an attacker-induced proof problem reported as an internal code-execution bug). Either handle the malformed boundary key before invoking the chained verifier and classify the failure as `Err(Error::Proof(ProofError::CorruptedProof(...)))`, or split the destructure into two distinct error branches (malformed-key path → `CorruptedProof`; true invariant break → `CorruptedCodeExecution`) and update the stale comment. The same change is needed at `verify_compacted_address_balance_changes/v0/mod.rs` lines 122–133. Severity is suggestion because the entire chained-results destructure goes away once the planned `(end_block, start_block)` rekey lands.
… WASM build) Regression from 404f1be: the compacted address-balance dedup pointed the verifier at `Drive::saved_compacted_block_transactions_address_balances_path_vec()`, which lives in a `server`-gated impl. The verifier is compiled under the `verify` feature only (that's how wasm-drive-verify builds rs-drive), so `cargo check -p drive --no-default-features --features verify` failed with E0599 — breaking the WASM/JS build. Move the canonical path into the `verify`-available `crate::util::common` (alongside `compacted_key`): the verifier uses it directly, and the server-side `saved_compacted_block_transactions_address_balances_path_vec` now delegates to it. So storage and verification still share one path definition (the reviewer's dedup intent) AND the verify-only build compiles. Verified: `cargo check -p drive --no-default-features --features verify` succeeds; `cargo test -p drive --features fixtures-and-mocks compacted` = 30 passed, 3 ignored, no warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dProof Review follow-up on the malformed-boundary-key rejection from 404f1be. The generator returned `None` on a non-16-byte authenticated key, which made `verify_query_with_chained_path_queries` fail with a generic GroveDB `InvalidInput` — a proof-derived condition surfaced as a non-proof error — and left the adjacent "generator always returns Some" destructure comment factually stale. Both verifiers now: the generator skips a malformed key in its `find_map` (so it still always returns `Some`, keeping the [boundary, forward] cardinality and its CorruptedCodeExecution invariant accurate), and the authenticated boundary key length is validated AFTER the chained verify on the returned `boundary_results`, returning `ProofError::CorruptedProof`. This gives the right user-facing error class for a malformed-key proof while preserving the "don't silently degrade to (start, start)" behavior. Comments updated to match. No behavior change on honest proofs; verify-only build compiles and verify_compacted tests pass (13 passed, 3 ignored). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at 04097f1. Latest delta is a small WASM-build refactor that hoists compacted_address_balances_path() into the verify-available util/common module; no proof-logic changes. Two carried-forward blocking findings (STILL VALID): the chained descending-boundary + ascending-forward proof scheme cannot satisfy honest multi-range sync for either the nullifier or address-balance verifier, and the author has self-flagged the PR as DO NOT MERGE with #[ignore]d regression tests on file. One carried-forward suggestion: the comment that justifies the cardinality-mismatch CorruptedCodeExecution classification still claims the generator 'always returns Some', which no longer holds after the malformed-boundary-key None path was added. One new latest-delta nitpick: the hoisted helper hardcodes b'c' instead of sharing one source of truth with COMPACTED_ADDRESS_BALANCES_KEY_U8.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs:122-133: Stale comment: generator no longer 'always returns Some' after the malformed-boundary-key guard
Lines 123-127 justify treating a `[boundary, forward]` cardinality mismatch as `DriveError::CorruptedCodeExecution` on the grounds that 'We pass exactly one chained (forward) generator that always returns `Some`.' That justification is no longer accurate after the malformed-boundary-key guard at lines 87-89 (`if boundary_results.iter().any(|(_p, key, _e)| key.len() != 16) { return None; }`). A caller-supplied proof with a non-16-byte boundary key now makes the generator return `None`. In practice GroveDB surfaces a generator-`None` as its own error from `verify_query_with_chained_path_queries` before this `try_into` runs, so the `CorruptedCodeExecution` branch is still unreachable from that path — but the comment is misleading and the same verbatim text exists at the address-balance verifier (lines 123-127). Tighten the comment to acknowledge the malformed-key `None` path (and that the error surfaces from the chained-verify call, not here), or rephrase the invariant to one that genuinely holds.
…byte Review nit follow-up: `util::common::compacted_address_balances_path` (the verify-available canonical helper) hardcoded the subtree byte as the literal `b'c'` while the server-gated `saved_block_transactions::queries` defined its own `COMPACTED_ADDRESS_BALANCES_KEY_U8 = b'c'`. Two literals for one proof-contract byte: a rename/repurpose on the server side would update with `cargo check` while the verify-side helper silently kept `b'c'`, and chained verification would then break against the new server path. Move the canonical `COMPACTED_ADDRESS_BALANCES_KEY_U8` constant into `util::common` (compiled in both the `verify` and `server` feature sets) and have `saved_block_transactions::queries` re-export it with `pub use`. The byte is now written once; the verify-side path helper references the constant directly, and every existing import path is unchanged because the re-export flows through the pre-existing `pub use queries::*` (server callers, drive initialization, and rs-drive-abci all keep `saved_block_transactions::COMPACTED_ADDRESS_BALANCES_KEY_U8`). No behavior change. Verify-only and server builds both compile; verify_compacted tests pass (13 passed, 3 ignored). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative+incremental review at head 89e1d36. Latest-delta scope is narrow: the malformed-authenticated-boundary-key path was reshaped so the generator now always returns Some (skipping non-16-byte keys) and a new post-verification check explicitly returns Error::Proof(ProofError::CorruptedProof) — this correctly resolves prior finding #3 and makes the 'generator always returns Some' justification on the CorruptedCodeExecution arm accurate. Carried forward as STILL VALID and blocking: the chained descending-boundary + ascending-forward one-directional-proof liveness defect on both compacted verifiers (nullifier and address-balance), with the author's own KNOWN LIVENESS BUG docstrings, two checked-in #[ignore]d regression tests, and a DO-NOT-MERGE banner on the PR. Carried forward as nitpick: the verify-available helper still duplicates the b'c' subtree byte with comment-only drift protection against COMPACTED_ADDRESS_BALANCES_KEY_U8.
🔴 1 blocking | 💬 1 nitpick(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs:64-118: Nullifier verifier chains a descending boundary query with an ascending forward query against one GroveDB proof — fails honest multi-range sync
STILL VALID at head 89e1d360 (unchanged by the latest delta). The verifier issues a descending boundary subquery (`Query::new_with_direction(false)` + `insert_range_to_inclusive(..=(start, u64::MAX))`, limit 1, lines 66-70) chained with an ascending forward subquery (`insert_range_from(start_key..)`, line 102) against a single GroveDB proof. A single GroveDB proof is one-directional, and the matching prover emits one ascending merged proof (boundary key unioned into the forward range). Whenever ≥2 compacted ranges sort at/below `start_block_height` — the realistic paginated-sync case, since the public handler hardcodes limit=25 — honest verification fails with `InvalidProof(... "Cannot verify upper bound of queried range")`. The KNOWN LIVENESS BUG docstring at lines 43-55 acknowledges this; the two `#[ignore]`d regression tests `multiple_ranges_below_query_height_verify` and `containing_range_with_two_lower_ranges_verifies` reproduce it; codex re-ran them at this SHA and both still fail; the PR description self-banners as DO NOT MERGE. Consequence on the trust boundary this PR was written to harden: trust-minimized clients cannot complete authenticated compacted-nullifier sync past the first compacted range — the only authenticated path for detecting double-spends in compacted history — and would either get stuck or fall back to un-proved sync, re-introducing the very concealment vector this PR set out to close. The author's planned fix (re-key compacted entries by `(end_block, start_block)` so retrieval collapses to a single ascending `range_from((H, 0)..)`) is the right shape and structurally also closes the original absence-proof soundness hole. Block merge until the rekey lands and both `#[ignore]`d regression tests pass un-ignored.
| // Step 1: boundary query — authenticate the single greatest compacted | ||
| // key <= (start_block_height, u64::MAX). Descending, limit 1. | ||
| let boundary_end_key = compacted_key(start_block_height, u64::MAX); | ||
| let mut boundary_inner = Query::new_with_direction(false); // descending | ||
| boundary_inner.insert_range_to_inclusive(..=boundary_end_key); | ||
| let boundary_query = | ||
| PathQuery::new(path.clone(), SizedQuery::new(boundary_inner, Some(1), None)); | ||
|
|
||
| // Step 2: generator — derive the forward query's lower bound from the | ||
| // AUTHENTICATED boundary result (not from raw proof bytes). | ||
| let forward_path = path.clone(); | ||
| let generator = | ||
| move |boundary_results: Vec<PathKeyOptionalElementTrio>| -> Option<PathQuery> { | ||
| // The boundary key contains start_block_height when its end_block is | ||
| // at or beyond it. Use the authenticated key directly in that case; | ||
| // otherwise there is no containing range and we start at | ||
| // (start_block_height, start_block_height). A non-16-byte key can't | ||
| // be parsed, so it is skipped here; the post-verification check | ||
| // below then rejects it as proof corruption rather than letting it | ||
| // silently degrade the forward lower bound. | ||
| let start_key = boundary_results | ||
| .iter() | ||
| .find_map(|(_path, key, _element)| { | ||
| if key.len() != 16 { | ||
| return None; | ||
| } | ||
| let end_block = u64::from_be_bytes( | ||
| key[8..16].try_into().expect("len checked to be 16"), | ||
| ); | ||
| if end_block >= start_block_height { | ||
| Some(key.clone()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(|| compacted_key(start_block_height, start_block_height)); | ||
|
|
||
| let mut forward_inner = Query::new(); | ||
| forward_inner.insert_range_from(start_key..); | ||
| Some(PathQuery::new( | ||
| forward_path.clone(), | ||
| SizedQuery::new(forward_inner, limit, None), | ||
| )) | ||
| }; | ||
|
|
||
| let (root_hash, proved_key_values) = GroveDb::verify_subset_query( | ||
| // Step 3: verify the chained queries. GroveDB enforces that every | ||
| // sub-query binds to the SAME root hash, so the boundary key authenticated | ||
| // in step 1 and the forward results in step 3 are consistent with one | ||
| // another and with the real state. | ||
| let (root_hash, results) = GroveDb::verify_query_with_chained_path_queries( | ||
| proof, | ||
| &path_query, | ||
| &boundary_query, | ||
| vec![generator], | ||
| &platform_version.drive.grove_version, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Nullifier verifier chains a descending boundary query with an ascending forward query against one GroveDB proof — fails honest multi-range sync
STILL VALID at head 89e1d36 (unchanged by the latest delta). The verifier issues a descending boundary subquery (Query::new_with_direction(false) + insert_range_to_inclusive(..=(start, u64::MAX)), limit 1, lines 66-70) chained with an ascending forward subquery (insert_range_from(start_key..), line 102) against a single GroveDB proof. A single GroveDB proof is one-directional, and the matching prover emits one ascending merged proof (boundary key unioned into the forward range). Whenever ≥2 compacted ranges sort at/below start_block_height — the realistic paginated-sync case, since the public handler hardcodes limit=25 — honest verification fails with InvalidProof(... "Cannot verify upper bound of queried range"). The KNOWN LIVENESS BUG docstring at lines 43-55 acknowledges this; the two #[ignore]d regression tests multiple_ranges_below_query_height_verify and containing_range_with_two_lower_ranges_verifies reproduce it; codex re-ran them at this SHA and both still fail; the PR description self-banners as DO NOT MERGE. Consequence on the trust boundary this PR was written to harden: trust-minimized clients cannot complete authenticated compacted-nullifier sync past the first compacted range — the only authenticated path for detecting double-spends in compacted history — and would either get stuck or fall back to un-proved sync, re-introducing the very concealment vector this PR set out to close. The author's planned fix (re-key compacted entries by (end_block, start_block) so retrieval collapses to a single ascending range_from((H, 0)..)) is the right shape and structurally also closes the original absence-proof soundness hole. Block merge until the rekey lands and both #[ignore]d regression tests pass un-ignored.
source: ['claude', 'codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative+incremental review at head 3e20289. Latest delta only resolves the prior u8 subtree-byte drift by single-sourcing COMPACTED_ADDRESS_BALANCES_KEY_U8 in verify-available util::common and re-exporting from saved_block_transactions::queries — verified. Two carried-forward blocking findings remain STILL VALID at this SHA: both compacted verifiers (nullifier and address-balance) chain a descending boundary subquery with an ascending forward subquery against a single one-directional GroveDB proof, with author-written KNOWN LIVENESS BUG docstrings, two #[ignore]'d regression tests, and a DO-NOT-MERGE banner in the PR. One latest-delta nit added: the adjacent byte-slice form COMPACTED_ADDRESS_BALANCES_KEY: &[u8; 1] = b"c" remains independently defined and is unused in-repo, so the b'c' byte is still written twice on the server side.
💬 1 nitpick(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…single-sourced u8
Follow-up to the prior single-sourcing commit: the adjacent byte-slice form
`COMPACTED_ADDRESS_BALANCES_KEY: &[u8; 1] = b"c"` still wrote the subtree byte
independently of the now-single-sourced `COMPACTED_ADDRESS_BALANCES_KEY_U8`,
partially undoing the goal — the slice could drift or be picked up by future
server code with no compiler-enforced linkage to the u8 (and it has no consumer
in-repo today).
Derive the slice from the constant: `&[COMPACTED_ADDRESS_BALANCES_KEY_U8]`. The
`b'c'` byte is now written in exactly one place (`util::common`); both the u8
re-export and the slice form resolve to it, so they cannot drift. Also corrects
the slice's stale doc comment ("address balances" -> "compacted address
balances"). Server-side only (the const lives in the `server`-gated
`saved_block_transactions::queries`); no behavior change. Server and verify-only
builds both compile.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Caution
This fix correctly closes the absence-proof soundness hole, but the implementation has a critical liveness regression: the chained verification uses a descending boundary query (greatest key
<= bound) and an ascending forward query (paginatedrange_from), but a single GroveDB proof is one-directional. The prover emits an ascending merged proof (prove_query_many), so the descending boundary verification fails.Symptom: when ≥2 compacted ranges sort at/below the query height — the normal sync case (the query handler hardcodes
limit=25and the SDK paginates in a loop) — the honest prove→verify roundtrip fails:The original tests only exercised the single-range and empty cases (one key ≤ bound), which is why this slipped through. Two
#[ignore]d regression tests (multiple_ranges_below_query_height_verify,containing_range_with_two_lower_ranges_verifies) now reproduce it, and the verifier carries a# KNOWN LIVENESS BUGnote.Planned fix (deferred): re-key compacted entries by
(end_block, start_block)instead of(start_block, end_block). Then "ranges overlapping[H, ∞)" is a single ascendingrange_from((H, 0)..)— no boundary query, no direction conflict — and the containing range becomes an ordinary in-range result that a malicious node cannot omit, so the soundness hole is closed structurally as a bonus. Both the nullifier and address-balance paths need this. (Alternative considered: a two-proof response — rejected as a larger wire/API change.)The same direction bug affects the address-balance compacted path in this PR.
Issue being fixed or feature implemented
Critical proof-soundness hole (double-spend / balance-change concealment for light clients).
The shielded-pool compacted nullifier changes verifier — and its twin, the compacted address-balance changes verifier — derived the forward-scan lower bound (
start_key) from raw, not-yet-authenticated proof bytes (extract_kv_entries_from_merk_proof), then verifiedrange_from(start_key..).Compacted keys are 16 bytes
(start_block_be, end_block_be). A range like(100, 200)that contains a requested height150sorts lexicographically before(150, 150). A malicious node could therefore proverange_from((150, 150)..)directly — a perfectly valid GroveDB proof in which the containing range(100, 200)appears only as a hash-only boundary node that the extractor skips. The verifier saw no containing range, accepted the incomplete proof as complete, and silently returned zero results for a block range that actually contained a spend.Because compaction drains the recent-nullifier tree, the compacted tree is the only source for those nullifiers, so hiding the containing range hides the spend from any trust-minimized client that relies on
prove=truesync (the SDK does). Consensus/full-node validation is unaffected (full nodes hold the whole set); this is a client-trust/soundness break.What was done?
Never trust the proof bytes to self-report the boundary. Both verifiers now use GroveDB's
verify_query_with_chained_path_queries:range_to_inclusive(..=(start_block_height, u64::MAX)), limit 1 — authenticates the single greatest compacted key<= (start, MAX)against the real root hash. A malicious prover cannot substitute or omit this key without breaking the root.start_keyfrom that authenticated boundary: the containing range iff itsend_block >= start_block_height, otherwise(start, start).range_from(start_key..)verified against the same root.The provers (
prove_compacted_nullifier_changes_v0,prove_compacted_address_balance_changes_v0) now emit a single merged proof viaprove_query_manycovering both the boundary key and the forward range so the chained queries are both satisfiable.PathQuery::mergerejects per-query limits, so the caller limit is applied on the verifier's forward subset query (subset verification accepts the superset proof).Both parallel instances are fixed (shielded nullifiers and address-fund balances).
How Has This Been Tested?
cargo test -p drive --features fixtures-and-mocks verify_compacted— 11 passed, 0 failed, including:malicious_skip_descending_proof_is_rejected(nullifier + address): a proof crafted the old vulnerable way (range_from((150,150)..), hiding a(100,200)spend) is now rejected — the boundary query authenticates(100,200)as the true greatest key<= (150,MAX), and the forward-only proof cannot satisfy the resulting forward query (Proof is missing data for query). It can never silently return zero.should_return_containing_range_for_start_inside_it(nullifier + address): honest proof from a height inside(100,200)correctly surfaces it.should_prove_and_verify_compacted_*_changes_roundtripand_empty_*: honest paths still verify.End-to-end
drive-abcistrategy tests for compacted changes and no-overlap were also exercised; lib suites forshielded,saved_block_transactions, andaddress_fundspass; clippy clean;cargo fmt --allapplied.Breaking Changes
None. This is a pre-release shielded-pool / saved-block feature (protocol v12, behind feature gates). The change alters the internal compacted-changes proof format but the verifier function signatures and the SDK-facing query interface are unchanged.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit