fix(drive): strict merged-query verification for unshield & shielded withdrawal proofs#3814
Merged
Merged
Conversation
…withdrawal proofs The `Unshield` and `ShieldedWithdrawal` execution-proof verify paths checked their sub-proofs with subset verification, which tolerates extra subtree data in the proof. PR #3793 (commit 63c31f7) tightened the sibling `ShieldFromAssetLock` surplus path to a single strict verification against the exact merged query the prover builds; this brings the two remaining shielded arms to the same pattern. Both arms now reconstruct the prover's exact merged `PathQuery` (same sub-queries, same cleared limits, same `PathQuery::merge`), set a non-`None` limit (`u16::MAX`, unreachable by the legitimate result set) since the strict verifier requires one, verify it once with `GroveDb::verify_query_with_absence_proof`, and partition the returned `proved_key_values` by path into the typed results (nullifier spend statuses + address balance / withdrawal document). This is defense-in-depth / canonicality, not a known exploit: the verified keys are derived from the signed transition and the sub-proofs are bound by a shared root hash, so a padded proof could not forge or misroute. The verifier now accepts exactly the data it asked for and rejects the rest. Adds a padded-proof negative test for each arm (mirroring `test_shield_from_asset_lock_padded_proof_is_rejected_by_strict_verify`) that proves a superset query padded with an unrelated `Pools` subtree and asserts the strict merged verify rejects it while the old subset verify would accept it. The existing nullifier/address/document roundtrip tests continue to pass. Closes #3812 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
✅ Review complete (commit f98de59) |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTightens Unshield and ShieldedWithdrawal verifiers to reconstruct the prover's merged PathQuery and run a single strict merged verify; refactors ShieldFromAssetLock to use a shared helper and adds negative tests that assert padded (superset) proofs are rejected while subset verification still accepts them. ChangesStrict merged query verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs (1)
1171-1194: ⚡ Quick winExercise the actual unshield verifier with
padded_proof.This currently proves the raw GroveDB primitive rejects the padded proof, but it still won't catch a regression in
Drive::verify_state_transition_was_executed_with_proofitself if that arm ever rebuilds the wrong merged query or switches back to the subset verifier.Suggested addition
+ assert!( + Drive::verify_state_transition_was_executed_with_proof( + &transition, + &BlockInfo::default(), + &padded_proof, + &|_| Ok(None), + platform_version, + ) + .is_err(), + "production unshield verifier must reject a padded proof" + ); + // The STRICT verifier (production behavior) MUST reject the padded proof. let strict_result = GroveDb::verify_query_with_absence_proof( &padded_proof, &production_pq, &platform_version.drive.grove_version,🤖 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-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs` around lines 1171 - 1194, Add a test call that runs the higher-level Drive verifier against the padded_proof to ensure Drive::verify_state_transition_was_executed_with_proof rejects the padded proof (not just GroveDb::verify_query_with_absence_proof); call Drive::verify_state_transition_was_executed_with_proof (or the test-accessible wrapper used in this module) with padded_proof, production_pq and platform_version.drive.grove_version and assert it returns an Err, mirroring the existing strict_result assertion so regressions where Drive rebuilds the merged query or switches to the subset verifier are caught.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs (1)
1361-1384: ⚡ Quick winAssert the padded proof fails the top-level shielded-withdrawal verifier too.
Right now this only checks the lower-level GroveDB behavior. If the ShieldedWithdrawal arm later drifts in how it rebuilds the merged query or selects the verifier primitive, this test still stays green.
Suggested addition
+ let withdrawals_data_contract = + Arc::new( + load_system_data_contract(SystemDataContract::Withdrawals, platform_version) + .expect("should load withdrawals contract"), + ); + + assert!( + Drive::verify_state_transition_was_executed_with_proof( + &transition, + &BlockInfo::default(), + &padded_proof, + &|id| { + if *id == withdrawals_contract::ID { + Ok(Some(Arc::clone(&withdrawals_data_contract))) + } else { + Ok(None) + } + }, + platform_version, + ) + .is_err(), + "production shielded-withdrawal verifier must reject a padded proof" + ); + // The STRICT verifier (production behavior) MUST reject the padded proof. let strict_result = GroveDb::verify_query_with_absence_proof( &padded_proof, &production_pq, &platform_version.drive.grove_version,🤖 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-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs` around lines 1361 - 1384, The test currently only verifies GroveDb::verify_query_with_absence_proof and GroveDb::verify_subset_query_with_absence_proof against padded_proof; extend it to call the top-level ShieldedWithdrawal verifier used by the state transition (the same code path that rebuilds the merged query / selects the verifier primitive) with padded_proof and assert it returns an error too, so that the high-level shielded-withdrawal behavior fails the padded proof; reference padded_proof and the state-transition’s shielded-withdrawal verification entry point (the function/method that the test already exercises for applying or validating ShieldedWithdrawal) and add an assert!(result.is_err(), ...) for that call.
🤖 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-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs`:
- Around line 1361-1384: The test currently only verifies
GroveDb::verify_query_with_absence_proof and
GroveDb::verify_subset_query_with_absence_proof against padded_proof; extend it
to call the top-level ShieldedWithdrawal verifier used by the state transition
(the same code path that rebuilds the merged query / selects the verifier
primitive) with padded_proof and assert it returns an error too, so that the
high-level shielded-withdrawal behavior fails the padded proof; reference
padded_proof and the state-transition’s shielded-withdrawal verification entry
point (the function/method that the test already exercises for applying or
validating ShieldedWithdrawal) and add an assert!(result.is_err(), ...) for that
call.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs`:
- Around line 1171-1194: Add a test call that runs the higher-level Drive
verifier against the padded_proof to ensure
Drive::verify_state_transition_was_executed_with_proof rejects the padded proof
(not just GroveDb::verify_query_with_absence_proof); call
Drive::verify_state_transition_was_executed_with_proof (or the test-accessible
wrapper used in this module) with padded_proof, production_pq and
platform_version.drive.grove_version and assert it returns an Err, mirroring the
existing strict_result assertion so regressions where Drive rebuilds the merged
query or switches to the subset verifier are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cc2f438-5745-410d-a7e7-84125b3b30ae
📒 Files selected for processing (3)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
thepastaclaw
reviewed
Jun 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Defensive hardening PR that correctly extends the strict merged-query pattern from PR #3793 to Unshield and ShieldedWithdrawal. The verifier-side merged query is byte-identical to the prover (sub-queries with cleared limits, same merge order, post-merge u16::MAX limit), path-partitioning is safe against out-of-set entries, and the new negative tests demonstrate the closed gap. No blocking issues; a small bound-check regression and a Drive-level test-coverage gap are worth addressing.
🟡 2 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/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:1141-1173: Lost u16-bound check on nullifier count when replacing verify_shielded_nullifiers_v0
The old `verify_shielded_nullifiers_v0` set `limit = Some(u16::try_from(nullifiers.len())?)` and returned a typed error if the keyset exceeded the verifier's u16 capacity. Both new arms (Unshield here, and ShieldedWithdrawal at line 1289) now unconditionally set `merged_pq.query.limit = Some(u16::MAX)` and rely on the post-loop "all returned are spent" check. If a transition ever carried more than `u16::MAX - 1` nullifiers, the strict verifier would silently truncate the result set and the spent-check would pass on a subset rather than reject the proof. Today upstream validation almost certainly bounds nullifier count well below this, so this is defense-in-depth — but the explicit bound made the `u16::MAX` sentinel locally sound, and re-asserting it costs almost nothing. Note: the existing ShieldFromAssetLock arm at line ~1500 has the same gap; fixing all three together would keep the pattern uniform.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs:1171-1194: Negative tests assert GroveDB behavior, not the Drive verifier entry point this PR changes
The new test reconstructs `production_pq` and confirms `GroveDb::verify_query_with_absence_proof` rejects the padded proof while the subset verifier accepts it. That demonstrates the GroveDB-level distinction, but it never feeds the padded proof through `Drive::verify_state_transition_was_executed_with_proof` — which is the dispatch site this PR actually changed. If a future regression accidentally restored subset verification in the Drive arm, or built a slightly different merged query than the hand-built `production_pq`, this test would still pass. The sibling ShieldedWithdrawal negative test has the same gap. Adding an assertion that the Drive-level verify call rejects the padded proof would lock the production code path into the test.
shumkov
reviewed
Jun 8, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3814 +/- ##
=============================================
- Coverage 87.16% 69.91% -17.26%
=============================================
Files 2627 19 -2608
Lines 322573 2712 -319861
=============================================
- Hits 281185 1896 -279289
+ Misses 41388 816 -40572
🚀 New features to boost your workflow:
|
Contributor
|
✅ 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: "b9af2b46ac2fb198e16905ef50e0d9d606428c64d97ea3c34a5403c1d727fe2a"
)Xcode manual integration:
|
…ed proofs The Unshield, ShieldedWithdrawal, and ShieldFromAssetLock (surplus) arms each inlined the same merged multi-root proof verification: clear every sub-query limit, merge, set an unreachable u16::MAX limit, then run the strict verify_query_with_absence_proof. Collapse the three copies (and their copy-pasted soundness rationale) into a single Drive::verify_merged_query_strict helper, mirroring the existing Drive::verify_shielded_nullifiers factoring. No behavior change: the helper clears sub-query limits and sets the same u16::MAX merged limit the arms set before. Each arm still partitions the returned key/values against its own sub-query paths. Addresses review feedback to deduplicate this block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er entry point The unshield and shielded_withdrawal padded-proof tests only proved that the low-level GroveDb::verify_query_with_absence_proof primitive rejects a superset proof. That left the dispatch site this change actually rewrote untested: a regression that rebuilt the merged query differently, or fell back to the subset verifier inside the Unshield/ShieldedWithdrawal arm, would keep both tests green. Route padded_proof through Drive::verify_state_transition_was_executed_with_proof and assert it errors, locking the production code path into the tests. The shielded_withdrawal case loads the withdrawals system contract for the lookup fn, mirroring its positive round-trip test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oof verify The Unshield arm did balances.insert(address, ..) on each clear-address entry, which would silently last-write-wins if a second address entry ever appeared, while ShieldedWithdrawal explicitly returns CorruptedProof on a second document entry. Both sub-queries target a single key under a singleton subtree, so make the invariant uniform: Unshield now also rejects a second entry instead of overwriting. This guards a state the strict verifier already makes unreachable (the address sub-query carries exactly one key, so verify_query_with_absence_proof returns at most one address entry), so it is defense-in-depth for maintainer clarity, not a reachable bug fix — hence no test exercises the new branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
reviewed
Jun 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental review of fcfd1fe over ceca33f. Latest delta: (1) refactor extracting verify_merged_query_strict helper used by all three shielded arms (clean dedup, behavior-preserving); (2) tests now route padded proofs through Drive::verify_state_transition_was_executed_with_proof and assert is_err(). No new in-scope blocking issues. Prior finding status: #1 (u16 bound check) STILL VALID — the refactor centralized but did not re-add the bound; #2 (test entry point) FIXED by fcfd1fe; #3 (Unshield silent overwrite vs ShieldedWithdrawal explicit reject) STILL VALID — partition loops unchanged; #4 (status set cross-check) STILL VALID — post-partition checks unchanged. All three carried-forward findings are low-severity defense-in-depth, so the review action is COMMENT.
🟡 1 suggestion(s) | 💬 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/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:1620-1628: [carried-forward] verify_merged_query_strict drops the prior u16-bound check on queried-key cardinality
Carried forward from the prior review (ceca33f1 → fcfd1fe5: STILL VALID; the refactor relocated this pattern into the new helper but did not address the gap). The replaced `verify_shielded_nullifiers_v0` set `limit = Some(u16::try_from(nullifiers.len())?)` and surfaced a typed error when the queried key set exceeded `u16` capacity. The new `verify_merged_query_strict` unconditionally clears sub-query limits and sets `merged_pq.query.limit = Some(u16::MAX)` for every shielded arm, then relies entirely on the post-loop "every returned entry is spent" check.
If a transition ever carried more than `u16::MAX` total queried keys (nullifiers ∪ one singleton address/document/outpoint), the strict verifier would silently truncate the result set; the spent-only loop would then pass on a subset and accept the proof. Upstream DPP structure validation (`max_shielded_transition_actions = 16`) keeps honest inputs far below this today, so there is no live exploit — but the helper's safety comment explicitly leans on the limit being unreachable, while the local invariant that makes it unreachable now lives elsewhere and is not asserted at the verifier boundary. Re-asserting it at the top of `verify_merged_query_strict` (or at each caller) via `u16::try_from(expected_count)?` would make the `u16::MAX` sentinel locally sound rather than transitively sound.
thepastaclaw
reviewed
Jun 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The latest 9-line delta resolves prior finding #2 by adding the duplicate-output-address rejection in the Unshield arm (lines 1212-1220), mirroring ShieldedWithdrawal's singleton-subtree invariant. Two prior defense-in-depth findings remain valid at f98de59: the strict merged-query helper still uses an unconditional u16::MAX limit without enforcing the queried-key bound, and neither shielded arm explicitly cross-checks the returned status keys against the signed nullifier set. No new defects in the latest delta; the fix is sound and well-aligned with the symmetric ShieldedWithdrawal pattern.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 carried-forward finding(s) are still valid at this commit and were already raised as open threads on this PR, so I am not re-posting duplicate inline comments.
Carried-forward prior findings still valid
-
suggestion — [carried-forward] verify_merged_query_strict relies on but does not enforce the u16 cardinality bound
-
nitpick — [carried-forward] Returned nullifier-status set is not cross-checked against the signed nullifier set (Unshield)
Prior findings fixed in this push
- FIXED — Unshield silently overwrites duplicate address entries; ShieldedWithdrawal explicitly rejects them: FIXED at f98de59. Lines 1212-1220 now explicitly reject a second entry under the clear-address pool with
ProofError::CorruptedProof('unshield proof contained more than one output-address entry'), mirroring ShieldedWithdrawal's singleton-subtree invariant. Matches the suggested fix from the prior review.
New findings in latest delta
None.
shumkov
approved these changes
Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Closes #3812.
The
UnshieldandShieldedWithdrawalexecution-proof verify paths checked their sub-proofs with subset verification, which tolerates extra subtree data in the proof. A dishonest full node could therefore return a proof padded with unrelated state and verification would still pass. PR #3793 (commit63c31f7c6e) tightened the siblingShieldFromAssetLocksurplus path to a single strict verification against the exact merged query the prover builds; this PR brings the two remaining shielded arms to the same, tighter pattern.This is defense-in-depth / canonicality, not a known exploit: in all cases the verified keys are derived from the signed transition (not from proof data) and the sub-proofs are bound by a shared root hash, so a padded proof could not forge or misroute. But a proof verifier should accept exactly the data it asked for and reject the rest.
What was done?
Both arms live in
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs. The prover side (prove_state_transition/v0/mod.rs) already merges each arm's sub-queries into onePathQuery; the verifier now mirrors that exactly:Unshield— reconstruct the prover's merged{nullifiers} ∪ {output-address}PathQuery(same sub-queries, same cleared limits, samePathQuery::merge), setlimit = Some(u16::MAX)(the strict verifier requires a non-Nonelimit;u16::MAXis unreachable by the legitimate result set so it never truncates an honest proof), verify it once with the strictGroveDb::verify_query_with_absence_proof, then partition the returnedproved_key_valuesby path into nullifier spend-statuses and the output-address balance (inlining theItemWithSumItemdecode fromverify_addresses_infos_v0).ShieldedWithdrawal— same shape, but the second sub-proof is the Core withdrawal document (SingleDocumentDriveQuery). Reconstruct the merged{nullifiers} ∪ {withdrawal-document}query, strict-verify once, partition by path into nullifier statuses + the single withdrawal document, and decode the document element.Notes carried over from the
ShieldFromAssetLockwork and applied here:PathQuery::mergeclears the limit and the strict verifier requires one; a limit smaller than the true result count can early-break before a sibling subtree's succinctness check runs, so an unreachableu16::MAXis used.How Has This Been Tested?
test_unshield_padded_proof_is_rejected_by_strict_verifyandtest_shielded_withdrawal_padded_proof_is_rejected_by_strict_verify(mirroringtest_shield_from_asset_lock_padded_proof_is_rejected_by_strict_verify): execute a real transition, prove a superset query padded with an unrelated genesisPoolssubtree, and assert the strict merged verify rejects it while the old subset verify accepts it — plus a liveness check that an honest proof for the production merged query strict-verifies.test_unshield_prove_and_verify_nullifiers_and_address,test_shielded_withdrawal_prove_and_verify_nullifiers_and_document) continue to pass.cargo test -p drive-abci --lib return_proof— 9 passed (incl. all shielded roundtrips + the 2 new negatives).cargo test -p drive --lib verify_state_transition_was_executed_with_proof— 60 passed.cargo check -p drive --no-default-features --features verify,cargo fmt --all, andcargo clippy -p drive --no-default-features --features verifyare clean.Breaking Changes
None. The proof format is unchanged (the prover already produced the merged proof); only the verifier is tightened to reject padded proofs. Not consensus-breaking — this path is used by light-client proof verification.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Improvements