Repository navigation
fix(platform-wallet): retry an oversized InstantSend asset-lock transition with a ChainLock proof - #5460
Conversation
…ition with a ChainLock proof An InstantSend asset-lock proof embeds the whole funding transaction and its InstantLock, ~184 B per funding input, while a ChainLock proof is ~40 B. A lock funded by many small coins (dashwallet-ios#1042: 83 inputs) made the shield transition exceed max_state_transition_size; DAPI refused it with an untyped InvalidArgument, so no fallback fired and Finish now replayed the same proof forever. - rs-dpp: validate_state_transition_size, the one definition of the size check, now used by rs-dapi and rs-sdk. - rs-sdk: checked_broadcast_request refuses an oversized transition before dispatch with the typed StateTransitionMaxSizeExceededError, on every path that sends one. - platform-wallet: needs_chain_lock_proof retries IS→CL when Platform rejects the IS lock or when a ChainLock proof would bring the transition under the limit; used by shielded funding, identity registration and top-up, invitation claims and platform-address funding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Size validation contract packages/rs-dpp/src/state_transition/mod.rs, packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs, packages/rs-sdk/src/platform/transition/broadcast_request.rs, packages/rs-sdk/src/error.rs, packages/rs-drive-abci/.../decode_raw_state_transitions/mod.rs |
The shared validator checks serialized transition sizes against the platform-version limit and returns a typed error. The SDK adds checked request construction, and tests compare client-side and decoder size refusals. |
Broadcast entry points packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs, packages/rs-sdk/src/platform/transition/broadcast.rs, packages/rs-sdk/src/platform/transition/broadcast_identity.rs, packages/rs-sdk/src/platform/transition/transfer_document.rs, packages/rs-sdk/src/platform/transition/vote.rs |
DAPI and SDK broadcast paths check transition size before dispatch. Identity, transfer, and vote requests use the checked builder. |
SDK fallback eligibility and invitation claims packages/rs-sdk/src/platform/transition/asset_lock_submission.rs, packages/rs-sdk/src/platform/dashpay/invitation.rs |
The SDK classifies InstantSend errors that can use a ChainLock proof, including size errors where the replacement fits. Invitation claims retry once with the fallback proof when eligible. |
Wallet asset-lock proof recovery
| Layer / File(s) | Summary |
|---|---|
Proof lookup and resumed recovery packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs, packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs, packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs |
Resumed waits can use mined-height lookup when a record cannot prove finality. ChainLock proofs are persisted before submission, and rejected-proof invalidation handles RecoveredFromChain rows. |
Shared fallback and wallet integrations packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs, packages/rs-platform-wallet/src/wallet/identity/network/registration.rs, packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs, packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs, packages/rs-platform-wallet/src/error.rs, packages/rs-platform-wallet/src/wallet/persister.rs, packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs, packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs |
A shared submission helper applies ChainLock-height retries and eligible InstantSend fallback. Registration, top-up, address funding, and shielded funding use it. Related docs and error exports are updated. |
Priority:
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant WalletFlow
participant AssetLockManager
participant ProofSync
participant Platform
WalletFlow->>AssetLockManager: Submit transition with InstantSend proof
AssetLockManager->>Platform: Retry submission with ChainLock-height handling
Platform-->>AssetLockManager: Return eligible proof or size error
AssetLockManager->>ProofSync: Upgrade and persist ChainLock proof
AssetLockManager->>Platform: Retry submission with ChainLock proof
Platform-->>WalletFlow: Return submission result and effective proof
Possibly related issues
- dashpay/platform#5573: Reports the same transition-size error, but its stated Orchard action-count cause is not addressed by the ChainLock proof fallback.
- dashpay/platform#5518: Concerns the shield guard during a ChainLock wait added to the shielded funding path.
- dashpay/platform#5605: Concerns
RecoveredFromChainstatus handling in the changed proof-upgrade and resume logic. - dashpay/dashwallet-ios#1231: Concerns removal-barrier handling in identity registration and top-up flows changed here.
- dashpay/platform#5513: Concerns the same asset-lock funding paths and an Exact-lock input limit not changed here.
Possibly related PRs
- dashpay/platform#5471: Moves invitation claim processing into
rs-sdk, the invitation path updated here.
Merge Risk: 
e2a58
-
An oversized shield can block other shield operations while it waits for ChainLock. This needs an explicit decision to accept that delay or a guard-scope fix before merging.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the primary change: retrying oversized InstantSend asset-lock transitions with a ChainLock proof. It is specific and relevant to the main objective. Linked Issues check Issue [ #1042] requires a client-side recovery path for an oversized asset-lock transition. The SDK now validates transition size before dispatch and returnsStateTransitionMaxSizeExceededError. The …Out of Scope Changes check The changes remain within [ #1042]. Shared size validation, decoder parity tests, SDK pre-dispatch checks, and wallet proof recovery support the same asset-lock failure mode. Registration, top-up, invi…Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Full details: Linked Issues check
-
Explanation
Issue [
#1042] requires a client-side recovery path for an oversized asset-lock transition. The SDK now validates transition size before dispatch and returnsStateTransitionMaxSizeExceededError. The wallet retries an oversized InstantSend proof with a measured ChainLock proof when the replacement fits. The wallet persists and resumes the replacement proof. Tests cover size boundaries, fallback eligibility, proof persistence, resume, and rejected proofs. This addresses the reported failure without requiring transaction splitting or a warning.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within [
#1042]. Shared size validation, decoder parity tests, SDK pre-dispatch checks, and wallet proof recovery support the same asset-lock failure mode. Registration, top-up, invitation claims, and platform-address funding reuse the shared fallback for the same proof and transition-size conditions. No unrelated consensus or Drive behavior change is shown.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
⛔ Final review complete — 1 blocking finding(s) (commit e2a583c) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 5e35308; no actionable in-scope findings remain. The suggested proof-variant guard addresses a synthetic error/proof combination, while the adjacent observations do not establish regressions requiring changes here. Validation was static only: the supplied CI snapshot reports successful Rust workspace tests, a skipped dedicated Rust wallet job, and pending Swift validation and several JavaScript suites.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff adds a shared size check and extends existing proof-fallback paths across several funding operations, but does not substantially redesign funds movement, proof verification, or consensus rules, so it falls below the critical threshold. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 99% left, 5h 98% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
/self-reviewed |
github-actions
Bot
removed
the
waiting-self-review
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR correctly adds client-side transition-size validation and ChainLock fallback handling. I found no blocking defect: the reported 100 KB serializer-order issue does not apply because the derive macro currently ignores the second serialization attribute, while three non-blocking API and consistency issues remain.
🟡 3 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Reuse the shared size validator in the authoritative Drive decoders
packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/decode_raw_state_transitions/v0/mod.rs:52-64
The PR describes validate_state_transition_size as the single definition of the maximum-size check, but both authoritative Drive decoder generations still duplicate the length comparison and StateTransitionMaxSizeExceededError construction. The current behavior is equivalent, but future changes to the boundary or error construction could make DAPI/SDK preflight disagree with consensus processing. Have the v0 and v1 decoders call the DPP helper and adapt its returned error into their existing ConsensusError container while preserving each generation's current wrapper behavior.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🟡 Suggestion: Do not expose an unchecked request as ready for broadcast
packages/rs-sdk/src/platform/transition/broadcast_request.rs:49-61
The public BroadcastRequestForStateTransition::broadcast_request_for_state_transition method still serializes an oversized transition without applying the new typed size check. A caller can obtain that request and execute it through the public DAPI request API, receiving DAPI's untyped InvalidArgument instead of the SDK's StateTransitionMaxSizeExceededError; only the internal high-level send paths use checked_broadcast_request. Either expose a public checked request-building API that receives the active PlatformVersion, or make the unchecked serializer explicitly internal/serialization-only and retain a separate raw reconstruction path for proof waiting.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, cross-cutting change to asset-lock proof selection and wallet funding flows, directly affecting funds movement and cryptographic InstantSend/ChainLock proof handling, including platform-versioned transition-size validation. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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-platform-wallet/src/wallet/asset_lock/orchestration.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs:466-469: Use the actual outpoint when estimating ChainLock proof size
The fallback-size estimate uses `OutPoint::null()` even though the replacement ChainLock proof will use the same outpoint as the InstantSend proof. With bincode's varint encoding, the null outpoint's `vout` is `u32::MAX` and takes five bytes, whereas normal asset-lock outputs such as index zero take one byte. This can overestimate the replacement proof by up to four bytes and cause a transition that would fit with the actual ChainLock proof to be rejected without attempting the fallback. Preserve the known outpoint while keeping the maximum height conservative.
In `packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/decode_raw_state_transitions/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/decode_raw_state_transitions/v0/mod.rs:52-64: Reuse the shared size validator in the authoritative Drive decoders
The PR describes `validate_state_transition_size` as the single definition of the maximum-size check, but both authoritative Drive decoder generations still duplicate the length comparison and `StateTransitionMaxSizeExceededError` construction. The current behavior is equivalent, but future changes to the boundary or error construction could make DAPI/SDK preflight disagree with consensus processing. Have the v0 and v1 decoders call the DPP helper and adapt its returned error into their existing `ConsensusError` container while preserving each generation's current wrapper behavior.
In `packages/rs-sdk/src/platform/transition/broadcast_request.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/transition/broadcast_request.rs:49-61: Do not expose an unchecked request as ready for broadcast
The public `BroadcastRequestForStateTransition::broadcast_request_for_state_transition` method still serializes an oversized transition without applying the new typed size check. A caller can obtain that request and execute it through the public DAPI request API, receiving DAPI's untyped `InvalidArgument` instead of the SDK's `StateTransitionMaxSizeExceededError`; only the internal high-level send paths use `checked_broadcast_request`. Either expose a public checked request-building API that receives the active `PlatformVersion`, or make the unchecked serializer explicitly internal/serialization-only and retain a separate raw reconstruction path for proof waiting.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- The serialization derive macro ignores subsequent platform_serialize attributes — The derive implementation uses
.find(...)and parses only the firstplatform_serializeattribute.StateTransitionhas separateunversionedandlimit = 100000attributes, so the current macro expansion ignores the limit. This is a pre-existing derive-macro defect rather than a regression introduced by this PR, but the new constant and test make the discrepancy relevant to future maintenance.- Follow-up: Track a separate fix in rs-platform-serialization-derive to parse and combine all matching platform_serialize attributes.
|
@coderabbitai review No review for |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR correctly implements the typed transition-size validation and InstantSend-to-ChainLock recovery path, and the CI snapshot shows the Rust workspace, Swift, Kotlin, and relevant JavaScript checks passing; the evo-sdk test remains in progress. Two wallet-state issues remain: the new persistence helper can overwrite a concurrent RecoveredFromChain status, and one rejected defensive rebroadcast path returns before consulting locator-backed recovery.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Run the locator fallback before returning on a rejected defensive rebroadcast
packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1503-1618
When the defensive rebroadcast returns BroadcastError::Rejected, the no-conflict branch performs only a zero-duration record probe and then returns TransactionBroadcastUnconfirmed if no local proof is available. A readiness snapshot can still contain peers even though the broadcaster loses connectivity before dispatch, so an earlier attempt may already have been mined. In that case the new SPV-verified mined-height locator is never consulted during this resume, even though it can recover a tracked row whose record is missing or heightless. The deferred retry covers only the transport_missed case and does not close this post-ready race. Route this branch through an equivalent locator-backed recovery, or add the connected-peer/readiness transition needed to guarantee a later resume before returning the provisional outcome.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, tenderdash-consensus-security)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to asset-lock funds movement and proof handling, including cryptographic proof selection, client/server transition-size validation, and peer-facing state-transition decoding in rs-drive-abci. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🤖 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-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1503-1618: Run the locator fallback before returning on a rejected defensive rebroadcast
When the defensive rebroadcast returns `BroadcastError::Rejected`, the no-conflict branch performs only a zero-duration record probe and then returns `TransactionBroadcastUnconfirmed` if no local proof is available. A readiness snapshot can still contain peers even though the broadcaster loses connectivity before dispatch, so an earlier attempt may already have been mined. In that case the new SPV-verified mined-height locator is never consulted during this resume, even though it can recover a tracked row whose record is missing or heightless. The deferred retry covers only the `transport_missed` case and does not close this post-ready race. Route this branch through an equivalent locator-backed recovery, or add the connected-peer/readiness transition needed to guarantee a later resume before returning the provisional outcome.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:502-522: Preserve RecoveredFromChain when persisting a ChainLock fallback
(existing thread: https://github.com/dashpay/platform/pull/5460#discussion_r4389386400)
`upgrade_to_persisted_chain_lock_proof` waits for the ChainLock proof and then unconditionally advances the tracked row to `ChainLocked`. During that asynchronous wait, reconciliation can move the row to `RecoveredFromChain`, which records that Core finality is known but Platform-side consumption remains unknown. `advance_asset_lock_status` only protects the consumed tombstone and does not compare the expected prior status, so this stale completion can overwrite the unknown-consumption marker and make the wallet report an ordinary in-flight lock. This helper is newly shared by all four funding flows, including identity paths that previously did not persist the upgraded proof. Use a status-aware compare-and-set that preserves `RecoveredFromChain` while attaching the ChainLock proof, and add a race regression.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Avoid consuming a nonce before local size validation — Withdrawn because the nonce reservation before local validation predates this PR and already occurs for existing structural-validation failures; correcting it requires a transactional nonce-cache change outside this fallback.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
|
Re-review on
Dismissing the changes request on that basis. |
|
Merge order with #5487: this PR goes in first, and #5487's follow-up builds on it.
Proposal: #5487 (comment) 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
/skip-bots |
|
Bots are done — your move: post |
|
/self-reviewed |
|
Ready for review — |
|
Correction to my comment above: the #5487 interceptor will not hook into
Nothing in this PR has to change for that. One small cleanup would help, though: 🤖 Co-authored by Claudius the Magnificent AI Agent |
Gaps found in an architecture pass (head
|
|
More precisely on the "two submissions on one outpoint" gap: the root problem is ownership, not concurrency. The wallet relies on an unwritten rule that the client never resumes an asset lock while a submission for it is still running. To follow that rule, the app has to track which lock the wallet is working on (#5476's outpoint callback, matched against tracked rows) and guess whether a long call is waiting for a ChainLock or stuck. That is the same pattern as #5487: the app keeps track of what the wallet sent and decides from it what is safe to do. This PR does not introduce it, but it stretches the window from seconds to minutes. The wallet should own this:
The app then only asks and displays, and #5476's callback becomes optional UI information rather than something correctness depends on. 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Bots are done — your move: address shumkov left a review thread unresolved, then post |
|
/self-reviewed |
|
Ready for review — re-review or resolve: shumkov. |
|
Policy satisfied — this can merge. |
Basic explanation
What this does: A wallet pays for Platform actions (creating an identity, topping it up, moving Dash to the shielded pool, funding a Platform address) with an "asset lock" — a Core transaction — plus a proof that the lock exists. The fast proof (InstantSend) carries the whole funding transaction, so it grows with every coin spent; the slower proof (ChainLock) is a fixed ~40 bytes but needs the transaction to be in a chainlocked block. When the fast proof makes the transition too big for Platform's 20 KB limit, the wallet now waits for the ChainLock and sends the small proof instead.
Value: Wallets holding many small coins (masternode payouts, frequent small receipts) can move their balance to the shielded pool again. Before this, a shield of ~65+ coins failed right after Confirm, the funds sat in an asset lock, and Finish now replayed the same oversized proof forever (dashwallet-ios#1042). Identity registration/top-up, invitation claims and platform-address funding share the same fallback, but in practice they stay under 20 KB: an asset lock that Core relays and Platform accepts has at most 100 inputs (
max_asset_lock_transaction_inputs), and those transitions are smaller than a shield.Risks: Low. No consensus change: Drive's decoders keep their own size check, unchanged, and a drive-abci test pins that they agree with the shared one at every protocol version; DAPI keeps its wire behaviour. The SDK now refuses an oversized transition locally with the typed
StateTransitionMaxSizeExceededErrorinstead of sending it for DAPI to refuse with an untypedInvalidArgument. A transfer that needs the fallback completes a few minutes after Confirm instead of instantly.Issue being fixed or feature implemented
Fixes dashpay/dashwallet-ios#1042, mirrored as #4460.
An 83-input asset lock produced a 23 KB
ShieldFromAssetLocktransition. DAPI refused it invalidate_state_transition_byteswith an untypedInvalidArgument, so the wallet's IS→CL fallback (which only matchedInvalidInstantAssetLockProofSignatureError) never fired, and every later resume rebuilt the same InstantSend proof.What was done?
state_transition::validate_state_transition_size(size_bytes, platform_version): the client-side definition of themax_state_transition_sizecheck, returningStateTransitionMaxSizeExceededError.validate_state_transition_bytesuses it; the response is unchanged.decode_raw_state_transitionsgenerations keep their own size check, so replay of shipped protocol versions does not depend on client-facing code. A parity test runs every protocol version from 1 toLATEST_VERSIONatmaxandmax + 1bytes and asserts that the decoder's size verdict and error matchvalidate_state_transition_size.checked_broadcast_requestbuilds the request and refuses an oversized transition before dispatch. Every sending path goes through it:broadcast_with_retries,broadcast_identity,transfer_document,vote.From<StateTransitionMaxSizeExceededError> for Erroradded next to the other per-error conversions.asset_lock_submission(where feat(sdk): move the DashPay invitation link codec and claim into rs-sdk #5471 moved the asset-lock matchers) —needs_chain_lock_proof(error, proof): retry with a ChainLock proof for an InstantSend proof when Platform rejected the IS lock, or when the size refusal would pass with a ChainLock proof for the same outpoint in its place (the proof lengths are measured; content that is too large on its own is not retried after a pointless ChainLock wait).as_state_transition_max_size_exceededadded, and re-exported fromplatform_wallet::errorwith the other matchers. The invitation claim seam indashpay/invitation.rsuses it, renamedsubmit_claim_with_chain_lock_fallback.needs_chain_lock_proofand now share the same ChainLock-proof bookkeeping, which only shielded funding had in full:upgrade_to_persisted_chain_lock_proofattaches the upgraded proof to the tracked row (ChainLocked) before the resubmit, so a failed resubmit does not leave a refused InstantSend proof for every resume to rebuild (identity registration and top-up skipped this);resolve_chain_proof_after_is_timeout(no longershielded-only) does the same after an IS-lock timeout before resuming;invalidate_rejected_chain_proof(no longershielded-only) takes a ChainLock proof Platform rejects for its height off the row.AssetLockManager::submit_with_chain_lock_fallback(new) — the submit, IS→CL retry and rejected-proof invalidation that the four funding flows share.validate_or_upgrade_proof: when neither memory nor the persister has the funding record, it now returns the InstantSend proof asOk(proof)instead of anAssetLockProofWaiterror. Nothing proves a ChainLock without the record, and if Platform cannot use the IS proof the flow's IS→CL fallback builds the ChainLock one. Callers: the fresh funding path (build.rs) and the resume arms inrecovery.rs.upgrade_to_chain_lock_proof: a missing record no longer fails at once. The ChainLock wait places the transaction through the SPV-verified mined-height locator and re-reads the record every round. A transient persister miss is read as a miss (record_or_persister_tolerating_transient); any other persister error is still returned.resume_asset_lock): theBuilt,Broadcastand proof-lessRecoveredFromChainarms wait withwait_for_resumed_proof, which also asks the mined-height locator, within the wait's bound, when the record cannot prove finality (missing, without a height, or contradicted by the SPV headers). The fresh funding wait stays record-only, so a just-broadcast lock makes no DAPI request. The proof-lessRecoveredFromChainarm only accepts a ChainLock proof: an InstantSend record is upgraded withupgrade_to_chain_lock_proof.invalidate_rejected_chain_proofalso handlesRecoveredFromChainrows: they keep their status, take a different ChainLock proof from the record if there is one, and otherwise lose the proof, so the next resume goes through the proof-less arm. It never puts an InstantSend proof on such a row.How Has This Been Tested?
rs-dppstate_transition/mod.rs:validate_state_transition_sizeboundary at every protocol version (the limit passes, one byte more is refused with both numbers).rs-drive-abcidecode_raw_state_transitions: decoder size verdict and error matchvalidate_state_transition_sizeat every protocol version.rs-sdkbroadcast_request.rs/broadcast.rs: a transition at the limit builds; one byte over, or past 100 KB, gets the typed error; an oversized transition is never dispatched.rs-sdkasset_lock_submission.rs: retry for an oversized IS transition that fits with a chain proof, measured with the lock's own outpoint (the boundary byte is pinned); no retry when it would still be too large, for a chain proof (also on a stale-islock error), or for unrelated errors.dashpay/invitation.rsclaim seam: fallback submitted on a size refusal, retry signal without a fallback, no resubmit when the fallback would not fit.platform-walletproof.rs:upgrade_to_persisted_chain_lock_proofleaves the rowChainLockedwith the upgraded proof;evicted_record: chain proof built after a ChainLock evicts the record (runs with and withoutkeep-finalized-transactions). No test drives a size refusal through each of the four funding call sites; they share the classifier and the persisting upgrade, which are tested.platform-walletrejected-proof invalidation tests now also run without theshieldedfeature.platform-walletchain_lock_wait_tests: resume through the locator with and without a bound, a proof-lessRecoveredFromChainrow with no record or an InstantSend record, a heightless record, a placement above the wallet's ChainLock, a contradicted placement, no lookup for a fresh wait or a zero bound;RecoveredFromChainrejected-proof invalidation (replace, clear, rebuild through the lookup).cargo test -p platform-wallet1614 (--all-features) / 1371 (default),-p dash-sdk413,-p platform-wallet-ffi428; clippy clean on the touched crates with--all-features.Breaking Changes
None. Public API is only added to. Behaviour change: an oversized transition now fails in the SDK with
Error::Protocol(ConsensusError(StateTransitionMaxSizeExceededError))before being sent, instead of DAPI's untypedInvalidArgument.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
PR Hygiene ·
e2a583crust-dapi(packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs) — approved by shumkovdpp(packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs,packages/rs-dpp/src/state_transition/mod.rs) — approved by shumkovrs-drive-abci(packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/decode_raw_state_transitions/mod.rs) — approved by shumkovrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs) — approved by llbartekllrs-platform-wallet(packages/rs-platform-wallet/src/error.rs,packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs,packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rsand 7 more) — approved by llbartekllrust-sdk(packages/rs-sdk/src/error.rs,packages/rs-sdk/src/platform/dashpay/invitation.rs,packages/rs-sdk/src/platform/transition/asset_lock_submission.rsand 5 more) — approved by shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.