fix(drive): verify identity-create-from-shielded-pool results without unbounded terminal-key queries#3859
Conversation
… unbounded terminal-key queries
The type-20 verifier arm rebuilt the merged {nullifiers, full-identity}
query and verified it with verify_query_with_absence_proof, which
enumerates the query's terminal keys — impossible for
full_identity_query's unbounded all-keys RangeFull. grovedb rejected the
query outright ("terminal keys are not supported with unbounded
ranges"), so every honest type-20 result proof failed client-side even
though the transition executed on-chain, leaving an orphaned-but-live
identity the client never persisted.
Verify with strict GroveDb::verify_query instead (succinctness still
rejects proofs padded with extra branches); derive nullifier spend
statuses from membership in the proved result set and reject
unrequested nullifier entries. The prove side is unchanged.
Adds a drive-abci regression test that executes a type-20 success
action through the real execute_event path (synthetic action, no Halo 2
proving), commits, then round-trips prove_state_transition →
verify_state_transition_was_executed_with_proof — failing with the
exact devnet error before the fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ Review complete (commit 65599df) |
|
Caution Review failedPull request was closed or merged during review 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 (3)
📝 WalkthroughWalkthroughThis PR refactors the ChangesIdentityCreateFromShieldedPool strict merged-query verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Narrowly scoped client-side verifier fix for type-20 (IdentityCreateFromShieldedPool) result proofs: swaps verify_query_with_absence_proof for verify_query because the embedded full-identity query has an unbounded all-keys range that the absence-proof variant cannot enumerate. Strict succinctness is preserved, spent-status is derived from membership in the proved-present set, and a regression test was added. All six agents converged on no in-scope findings.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3859 +/- ##
=============================================
- Coverage 87.15% 70.73% -16.43%
=============================================
Files 2641 20 -2621
Lines 327793 2788 -325005
=============================================
- Hits 285701 1972 -283729
+ Misses 42092 816 -41276
🚀 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: "9d7744bad2d12d83b767b1ffb00c52e78f5492098724faa7bf791cbf3064929c"
)Xcode manual integration:
|
2 participants
Issue being fixed or feature implemented
On devnet (paloma, drive 4.0.0-rc.1, 2026-06-11), fetching the execution-result proof for an
IdentityCreateFromShieldedPool(state transition type 20) always failed with:even though the transition executed successfully on-chain (pool balance dropped by the denomination, notes +2, chain kept advancing). The client's
broadcast_and_wait::<StateTransitionProofResult>therefore reported the registration as failed and never persisted the new identity — leaving an orphaned-but-live identity.Root cause (client-side verification, not proof generation): the type-20 verifier arm rebuilds the merged {nullifiers, full-identity} query and verified it with
GroveDb::verify_query_with_absence_proof. That entry point enumerates the query's terminal keys to synthesize absence entries — which is impossible forfull_identity_query, whose all-keys sub-query is an unboundedRangeFullover the identity keys tree, so grovedb rejects the query outright for every honest proof. The other shielded spend types (unshield/transfer/withdrawal) only merge explicit-key sub-queries, which is why their result proofs round-tripped fine; the component unique to type 20 is the full-identity query.What was done?
verify_state_transition_was_executed_with_proofnow verifies the same merged query with strictGroveDb::verify_query(succinctness on, no absence synthesis). Absence entries were never needed: every queried element (the spent nullifiers and the created identity) must be PRESENT, so each nullifier's spend status is now derived from membership in the proved result set, with an explicit rejection of unrequested nullifier entries. The succinctness check still rejects proofs padded with branches beyond {nullifiers, identity}, preserving the strict guarantee from Tighten Unshield & ShieldedWithdrawal execution proofs to a single strict merged query (parity with ShieldFromAssetLock) #3812.executed_transition_result_proof_roundtrips) that executes a type-20 success action through the realexecute_eventpath (synthetic action — no Halo 2 proving), commits, generates the result proof withprove_state_transition, and verifies it — failing with the exact devnet error before the fix, passing after.How Has This Been Tested?
GroveDB(QueryError(NotSupported("terminal keys are not supported with unbounded ranges")))and passes post-fix (verified both ways by stashing/restoring the fix).cargo test -p drive --features full,verify --lib verify_state_transition_was_executed_with_proof— 61 passed.cargo test -p drive-abci --lib shielded— 144 passed, 0 failed (includes the real-Orchard prove/verify round-trips for the other shielded types).cargo check -p drive --no-default-features --features verify(verify-only build),cargo check -p drive --all-features --tests,cargo check -p drive-abci --all-features --tests,cargo fmt --all -- --check, clippy clean on the changed code.Breaking Changes
None — this is a node/SDK query-proof verification fix; no consensus or wire-format change.
Checklist:
Follow-up note (not fixed here): SwiftExampleApp misattributes this broadcast-result error to the "Generating Halo 2 proof" progress step in the registration progress view (
packages/swift-sdk/SwiftExampleApp) — the proof had already been generated and the transition broadcast/executed by the time this error fires. Worth a separate UI fix so result-proof failures surface as their own step.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes
Documentation