fix(drive): strengthen execution proof result validation (tagged-outcome variant)#4207
Conversation
…on-proof-binding # Conflicts: # packages/swift-sdk/build_ios.sh
Resolve the tests-rs-workspace.yml conflict to the base side: the base already carries the coverage-cleanup retry loop and its own timeout tuning, so this PR no longer needs to ship workflow changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ransitions verify_state_transition_was_executed_with_proof keeps failing closed for the transition families whose proof cannot be bound to execution (balance top-ups, credit transfers and withdrawals, address funds movements, shields, no-history token burn/mint/transfer). A new sibling entry point, verify_state_transition_affected_state_with_proof, serves the consumers those errors broke: it behaves identically for execution-verifiable types and, for the receipt-gated families, returns a verified snapshot of the affected state instead — keys derived from the transition, values authenticated as of the proof's block, explicitly documented as height-pinned snapshots rather than execution evidence. The SDK wait path (FromProof for StateTransitionProofResult) now uses the affected-state entry point, restoring the address funds, top-up, withdrawal, credit-transfer and no-history token transfer flows that matched result variants nothing produced anymore. wasm-drive-verify gains the matching verifyStateTransitionAffectedStateWithProof export. Also relax the identity-update disabled-key check from exact equality with the proof block's timestamp to at-or-before it: proofs may be generated at a later block than the one that executed the update, and disabled_at never changes afterwards, so retried or replayed proofs were failing verification spuriously. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Alternative design to the two-entry-point split: a single verify_state_transition_was_executed_with_proof returns (RootHash, StateTransitionProofOutcome), where the outcome is ExecutionProved(result) when the proof binds this specific transition's execution and AffectedState(result) when it can only authenticate the affected keys' state at the committed block (balance top-ups, credit transfers and withdrawals, address funds movements, shields, no-history token operations — the last decided by the contract's keeps-history flags at verification time). The guarantee is part of the type, so a snapshot cannot be mistaken for execution evidence and callers choose explicitly by matching. This replaces the StateTransitionProofSemantics parameter and the verify_state_transition_affected_state_with_proof sibling entry point (and its platform-version gate): the v0 body sets an affected_state flag in the snapshot arms and wraps the result once at the tail. Downstream: - rs-drive-proof-verifier FromProof exposes the inner result unchanged; callers needing execution evidence match the outcome. - wasm-drive-verify returns a single result object with an executionProved getter instead of a second export. - drive-abci strategy tests assert the snapshot tag on real post-state proofs (stronger than the previous expect-error: the proof must parse AND stay un-upgraded to execution evidence); the shield state-proof test flips accordingly. - drive verifier tests assert ExecutionProved on happy paths and AffectedState on the snapshot families; the former fail-closed reject tests merge into the snapshot assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
📝 WalkthroughWalkthroughThe PR introduces tagged execution-versus-affected-state proof outcomes, updates strict and affected-state wait semantics, adds WASM and JavaScript affected-state APIs, strengthens identity proof validation, and migrates affected-state SDK and wallet operations. ChangesProof outcome and verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK
participant Drive
participant ProofVerifier
Client->>SDK: broadcastAndWaitForAffectedState
SDK->>Drive: broadcast and wait for outcome
Drive->>ProofVerifier: verify proof
ProofVerifier-->>Drive: ExecutionProved or AffectedState
Drive-->>SDK: verified affected-state result
SDK-->>Client: proof result
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4207 +/- ##
===========================================
Coverage 87.51% 87.52%
===========================================
Files 2667 2667
Lines 336972 338070 +1098
===========================================
+ Hits 294903 295890 +987
- Misses 42069 42180 +111
🚀 New features to boost your workflow:
|
Codex Security review — execution-proof outcome semanticsVerdict: use #4207 as the foundation, but do not treat it as a complete security fix in its current form. The tagged-outcome design is architecturally better than closed PR #4155, but the security guarantee is still lost before production SDK callers can enforce it, and several transition arms receive the wrong tag. I reviewed the effective landing tree Why #4207 is the better design
Main blocker: the new tag is erased
Ok((Some(outcome.into_result()), mtd.clone(), proof.clone()))That conversion deliberately discards whether Drive returned This conflicts with the PR description's statement that callers can match on the outcome: the production FindingsThe review produced 10 reportable findings: 6 Medium and 4 Low, all with high static confidence. There is no High/Critical finding, consensus-state forgery, validator termination, or chain-stall path in this change.
All nine operation-specific branches leave the shared Two additional classifier candidates were validated but not promoted after impact calibration:
Realistic attack boundaryThe attacker is a Byzantine or compromised DAPI/proof-serving node selected by the client. It cannot forge GroveDB state, a Tenderdash quorum signature, the victim's signature, balances, or consensus execution. It can, however:
The resulting impact is false client-side completion: wallet activity, accounting, nonce/retry state, audit state, or a dependent business workflow can advance for a request that did not execute. The no-surplus shield case has the strongest external-settlement story because a counterparty may rely on consumption of a shared asset-lock outpoint as payment confirmation. Recommended fix sequence
The classifier and end-to-end type-propagation changes should be direct code fixes and should not require a protocol-version upgrade. A new committed receipt/proof format likely would require versioning and coordinated activation. Merge recommendationI would keep #4207 rather than revive #4155, but request the classifier and end-to-end tag-preservation changes before considering the execution-proof issue closed. Otherwise the PR establishes the right abstraction while production callers continue receiving effectively the same untagged success result. Validation was performed through full diff inventory, independent discovery, semantic deduplication, centralized static validation, focused negative-case construction, and source-to-sink attack-path analysis. Exact-revision source checkers and regression patches were prepared and apply/format checked. Bounded Cargo runs stopped during cold native dependency compilation before the focused test binaries executed, so this review does not claim a completed runtime or live-DAPI reproduction. |
Replace the mutable affected_state flag with an exhaustive classifier (state_transition_proof_binds_execution): every transition family — and for tokens, the contract's keeps-history configuration and group usage — is matched explicitly, so a new variant fails to compile until it is classified and nothing silently inherits ExecutionProved. Reclassify nine snapshot-only arms that were wrongly tagged as execution evidence (per the Codex security review): data contract update, identity credit transfer to addresses, identity top-up from addresses, shield from asset lock (both surplus branches), identity create from shielded pool, and the ungrouped no-history token freeze, unfreeze, direct purchase and set-price operations. Their proofs authenticate current state (contract body, identity/address balances, frozen flags, pricing schedules, consumed outpoints) without binding the request's amounts, nonces, signature, or occurrence. Strategy tests for top-up-from-addresses and credit-transfer-to- addresses now assert the snapshot tag on real post-state proofs; the data contract update happy-path test flips the same way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FromProof implementation for broadcast waits erased the StateTransitionProofOutcome tag before SDK callers could see it, so strict callers still received effectively untagged success results. - FromProof<BroadcastStateTransitionRequest> now targets StateTransitionProofOutcome, carrying the tag through verification. - The public wait APIs (wait_for_response, broadcast_and_wait, and their _with_metadata forms) are now STRICT: an AffectedState outcome returns the new typed Error::ExecutionNotProved instead of a success the caller could mistake for execution confirmation. The error is non-retriable — no other node can upgrade a snapshot into evidence. - New wait_for_affected_state / broadcast_and_wait_for_affected_state APIs (plus _with_metadata forms) accept snapshot outcomes explicitly, documented as height-pinned state, never execution evidence. - Every snapshot-family flow moved to the affected-state APIs: identity top-up, credit withdrawal, credit transfer (plain and to-addresses), top-up from addresses, address funds transfer / funding / withdrawal, identity create from shielded pool, the wallet's shield and fund-from-asset-lock waits, and the data contract update wait (via a type dispatch in Waitable). Token operation flows use the snapshot APIs because bindability depends on the contract's keeps-history configuration, which the SDK cannot know statically. - wasm-sdk exposes waitForAffectedState and broadcastAndWaitForAffectedState (mirrored in the evo-sdk facade) and maps the new error to its own ExecutionNotProved kind; FFI flows inherit the correct semantics through the rs-sdk helpers unchanged. - Regression tests: a classifier table test pins the snapshot-only families in drive, and an rs-sdk unit test pins the strict wait's rejection of AffectedState outcomes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both requested work streams are implemented and pushed (44a29c6 → f4857e0). 1. Classifier containment (all 10 findings addressed). The mutable 2. End-to-end tag preservation. Regression tests: a classifier table test pins the snapshot-only families in drive; an rs-sdk unit test pins the strict wait's rejection; strategy tests assert the snapshot tag against real post-state proofs for the address-family transitions; and the flipped data-contract-update happy-path test pins that family with a real proof. Deliberately deferred: per-family two-request regression tests beyond the above (the strategy tests cover the address family end-to-end; the rest need bespoke proof fixtures), and the long-term committed-receipt binding, which needs a versioned proof format and coordinated activation as you note. Validation: drive 3,234 lib tests pass (68 in the verifier module), dash-sdk 174, platform-wallet 493; drive-abci test targets compile; wasm-sdk (wasm32), rs-sdk-ffi, platform-wallet-ffi compile; clippy clean on dash-sdk and platform-wallet; the |
…fier Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Issue being fixed or feature implemented
Alternative design to #4155 — pick one, close the other. This branch contains everything on #4155 plus one commit (44a29c6) that replaces its two-entry-point verifier design with a tagged return type.
Both PRs solve the same problem: state-transition proofs for some transition families (balance top-ups, credit transfers/withdrawals, address funds movements, shields, no-history token operations) cannot be bound to the execution of one specific transition — they only authenticate the affected keys' state at the committed block — and callers must not mistake such snapshots for execution evidence.
verify_state_transition_was_executed_with_prooffails closed for those families, andverify_state_transition_affected_state_with_proofreturns the verified snapshot. The chosen function is the guarantee.(RootHash, StateTransitionProofOutcome), where the outcome isExecutionProved(result)orAffectedState(result). The guarantee is part of the type; callers match on it. Since the classification is derivable from the transition type (plus the contract's keeps-history flags for tokens), there is no semantics parameter and no second entry point.What was done?
StateTransitionProofOutcomein dpp'sproof_resultmodule (ExecutionProved/AffectedState, withresult(),into_result(),is_execution_proved()).affected_stateflag in the snapshot arms and tags the result once at the tail; theStateTransitionProofSemanticsenum, the sibling entry point, and its platform-version gate are removed. Net −159 lines vs design 1.FromProof(SDK wait path) exposes the inner result unchanged — same five broadcast flows work; callers that need execution evidence can match the outcome.verifyStateTransitionWasExecutedWithProofwhose result gains anexecutionProvedgetter (the second export from design 1 is gone).ExecutionProvedon happy paths andAffectedStateon the snapshot families; design 1's fail-closed reject tests merge into the snapshot assertions (they were the same proofs).Trade-off to weigh: design 1 gives a function whose
Okalways means "executed" (misuse = calling the wrong function); design 2 gives one function whose result type forces the distinction at every use site (misuse = writing.into_result()without thinking). Design 2 touches more call sites now but leaves a smaller, single-entry API surface.How Has This Been Tested?
drive lib: 3,233 tests pass (67 in the verifier module, including tag assertions for both guarantees and the disabled-key regression test). dpp proof_result: 4 pass. drive-abci: all test targets compile; the shield state-proof test passes with the new snapshot-tag assertion. dash-sdk: 173 tests pass. Clippy clean on
drive --all-features --all-targets;drive --no-default-features --features verifyandwasm-drive-verify(wasm32) compile.Breaking Changes
None consensus-wise. Rust API: the verifier's return type changes and the affected-state sibling from #4155 never ships (neither is released).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes