Repository navigation
fix(platform-wallet): retry an oversized InstantSend asset-lock transition with a ChainLock proof - #5460
fix(platform-wallet): retry an oversized InstantSend asset-lock transition with a ChainLock proof#5460romchornyi wants to merge 13 commits into
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Define size validation and checked requests packages/rs-dpp/src/state_transition/mod.rs, packages/rs-sdk/src/error.rs, packages/rs-sdk/src/platform/transition/broadcast_request.rs, packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs |
A shared validator and checked request builder return a typed error when serialized transition size exceeds the platform limit. Tests cover size boundaries and InstantSend and ChainLock proof sizes. |
Apply size checks at 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 uses the shared validator. SDK broadcast paths use checked request construction with the platform version before dispatch. |
Classify InstantSend proof retry eligibility packages/rs-sdk/src/platform/transition/asset_lock_submission.rs, packages/rs-platform-wallet/src/error.rs, packages/rs-sdk/src/platform/dashpay/invitation.rs |
A shared predicate selects InstantSend proofs for ChainLock retry when the proof is invalid or its replacement brings an oversized transition within the limit. |
Persist and use ChainLock fallback in wallet flows packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs, packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.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/wallet/asset_lock/sync/tracking.rs, packages/rs-platform-wallet/src/wallet/persister.rs, packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs |
Wallet flows use shared submission and timeout recovery. ChainLock upgrades persist tracked state before resubmission. Tests cover persistence failures, record eviction, bounded reconciliation, and resumability. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant WalletAssetLockFlow
participant needs_chain_lock_proof
participant ChainLockProofUpgrade
participant checked_broadcast_request
participant PlatformDAPI
WalletAssetLockFlow->>needs_chain_lock_proof: classify the submission error and proof
needs_chain_lock_proof-->>WalletAssetLockFlow: select ChainLock retry when eligible
WalletAssetLockFlow->>ChainLockProofUpgrade: upgrade and persist the funding proof
ChainLockProofUpgrade-->>WalletAssetLockFlow: return ChainLock proof
WalletAssetLockFlow->>checked_broadcast_request: build a checked request
checked_broadcast_request->>PlatformDAPI: dispatch the request when within the size limit
Possibly related issues
- dashpay/platform#5513 — The changes add a size fallback for asset-lock transitions but do not reject over-cap input counts before broadcast.
- dashpay/platform#5518 — The changes add the oversized InstantSend-to-ChainLock wait but do not release
shield_guardduring that wait. - dashpay/dashwallet-ios#1231 — The changes centralize asset-lock submission but do not add a wallet-removal barrier.
- dashpay/platform#5516 — The changes centralize persistence and submission but do not add wallet-removal exclusion.
Suggested reviewers: quantumexplorer
Merge Risk: 
e09f2
-
Oversized asset-lock transitions are now rejected locally before dispatch and retried with a ChainLock proof when that proof fits. No concrete merge-blocking defect was identified. One known limitation remains: a fresh oversized shield can hold the shield guard while waiting for a ChainLock, and the author tracks this separately in
#5518.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 79.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 126 functions across 21 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the primary change: retrying oversized InstantSend asset-lock transitions with a ChainLock proof. Linked Issues check Issue [ #1042] requires a client-side recovery path for an oversized asset-lock transition. The PR adds typed local size validation and retries an oversized InstantSend-funded transition with a measure…Out of Scope Changes check The changes remain connected to [ #1042]. Centralized size validation inrs-dpp, DAPI integration, SDK pre-dispatch checks, and shared ChainLock fallback logic support the recovery path. Related regi…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Full details: Linked Issues check
-
Explanation
Issue [
#1042] requires a client-side recovery path for an oversized asset-lock transition. The PR adds typed local size validation and retries an oversized InstantSend-funded transition with a measured ChainLock proof when the replacement fits. Thers-dppregression test covers the 83-input case, and SDK and wallet tests cover size boundaries, fallback, persistence, resume, and rejected-proof handling. This alternative fixes the reported failure without requiring input splitting or a warning.
Full details: Out of Scope Changes check
-
Explanation
The changes remain connected to [
#1042]. Centralized size validation inrs-dpp, DAPI integration, SDK pre-dispatch checks, and shared ChainLock fallback logic support the recovery path. Related registration, top-up, invitation-claim, and platform-address flows reuse the same asset-lock behavior. The reported tests and documentation support these changes. No unrelated Drive or consensus change is shown.
-
-
✨ Finishing Touches: 💡 1
-
📝 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 4d413cf) |
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
github-actions
Bot
added
bot-review-skipped
|
/self-reviewed |
|
Ready for review — |
github-actions
Bot
added
ready-for-human
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Re-approving on f7dcf5c. The rs-platform-wallet and rs-sdk changes are identical to the e09f238 I approved. Since then the PR moved the Drive decoder size check (decode_raw_state_transitions v0/v1) onto validate_state_transition_size and merged v5.1-dev. Neither touches the wallet. The in-place change looks consensus-neutral to me: same len > max_state_transition_size comparison, same two numbers, and From<StateTransitionMaxSizeExceededError> for ConsensusError maps to the same BasicError variant. The v0 merge resolution only keeps the StateTransitionNotActiveError imports from #5337. Drive/DPP sign-off stays with their code owners.
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
I reviewed the dpp and drive-abci parts.
The decoder refactor doesn't change consensus today. I checked it: the comparison is still len > system_limits.max_state_transition_size, the error is still StateTransitionMaxSizeExceededError::new(len, max), and the From impl wraps it in exactly BasicError::StateTransitionMaxSizeExceededError. I'd still like it reverted; details inline.
Other notes:
AssetLockProof::serialized_len_in_state_transitionis fine. Its config matches what#[platform_serialize]emits forStateTransition, and the top-up parity test catches any drift.- PR description: "In-place changes to shipped generations" says the helper is tested at versions 1, 13 and 14, but the test iterates
1..=LATEST_VERSION. If the decoder edits are reverted, the section goes away anyway. - Merge order: this PR and #5476 conflict in
rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rsandwallet/shielded/fund_from_asset_lock.rs. #5476 calls itself a follow-up, so I'd merge this one first and rebase #5476.
| ), | ||
| ), | ||
| ); | ||
| if let Err(error) = validate_state_transition_size( |
There was a problem hiding this comment.
Please revert the decoder edits in v0 and v1 so both generations are byte-identical to the base again.
This isn't a consensus bug: the output is the same today. The problem is the shape. v0 (PV1–13, mainnet) and v1 (PV14, active on testnet, plus PV15) used to own their size check. Now replay of every shipped protocol version goes through dpp::state_transition::validate_state_transition_size, an unversioned pub fn whose doc invites the SDK and DAPI to call it before sending. If someone edits it for a client reason, every shipped protocol version changes with it, and only a "frozen" doc comment stands in the way. Your 2026-10-20 reply made exactly this argument, and I agree with it.
To keep the two in step without coupling them, add a parity test in drive-abci instead. For every protocol version in 1..=LATEST_VERSION, run decode_raw_state_transitions on inputs of max and max + 1 bytes and assert that its size verdict and error match validate_state_transition_size. That leaves one tested definition of "too large" and keeps replay independent of client code. Then drop the "Consensus depends on this" paragraph from the helper's doc, and the In-place section from the description.
There was a problem hiding this comment.
Reverted in cb58c71: v0 and v1 are byte-identical to the base again. The parity test is should_agree_with_the_client_size_check_at_every_protocol_version in decode_raw_state_transitions/mod.rs: for every protocol version in 1..=LATEST_VERSION it decodes max and max + 1 bytes through the dispatcher and asserts the size verdict and error equal validate_state_transition_size's. Checked that it catches a drift (a temporary >= in v1 fails it at PV14, 20480 bytes). The "In-place" section is gone from the description. Agreed on the shape; I should have kept my 2026-10-20 position.
| ), | ||
| ), | ||
| ); | ||
| if let Err(error) = validate_state_transition_size( |
There was a problem hiding this comment.
Same as v0: please revert this one too, since PV14 is active on testnet. The new PV14 boundary test can stay; it also passes against the original code and gives v1 the coverage v0 already had.
There was a problem hiding this comment.
Reverted in cb58c71. I dropped the PV14 boundary test as well, so v1 is fully back to the base: the parity test now covers the exact-limit pass and the one-byte-over refusal, with both numbers, at PV14 and every other version.
| /// `max_state_transition_size`: a transition of `size_bytes` is refused when | ||
| /// it is larger, with the error Drive reports for one. | ||
| /// | ||
| /// Consensus depends on this: Drive refuses oversized transitions through it |
There was a problem hiding this comment.
Once the decoders go back to their own check, this paragraph no longer holds, so please remove it. Please also drop dashwallet-ios#1042 from this doc: tracker references belong in the PR or the commit, not in a DPP doc comment.
There was a problem hiding this comment.
Done in cb58c71: the consensus paragraph and the tracker reference are removed. The doc now says the decoders keep their own copy and names the drive-abci parity test that keeps them in step.
| } | ||
|
|
||
| // --------------------------------------------------------------------- | ||
| // Asset-lock proof size (dashwallet-ios#1042) |
There was a problem hiding this comment.
Nit: please keep tracker references and incident history out of the code. That covers dashwallet-ios#1042 here, ISSUE_1042_INPUTS, the ..._issue_1042_... test names and "A wallet fed by many small payouts…". Name them by behaviour instead, e.g. MANY_INPUT_LOCK_INPUTS and should_overflow_max_size_with_a_many_input_instant_proof. The history belongs in the PR description.
There was a problem hiding this comment.
Done in cb58c71, across the PR, not only here: no tracker references or incident history left in the code (dpp, drive-abci, rs-sdk, platform-wallet comments).
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn should_exceed_max_size_with_the_issue_1042_instant_proof() { |
There was a problem hiding this comment.
This test checks the data and today's limit, not DPP code: a per-input cost of 180–190 B, and an 83-input lock being over 20,480 B. If a later protocol version raises max_state_transition_size, the test fails with a message about the wallet's fallback, which DPP doesn't own. The invariant DPP does own, that a ChainLock proof's size doesn't depend on the input count, is already covered by the next test. I'd drop this one, or move the overflow premise into the wallet/SDK tests. Low priority.
There was a problem hiding this comment.
Dropped in cb58c71. With the overflow test gone, the 83-input chain-proof test only re-checked what seed_pool_batch_fits_max_state_transition_size already checks (a larger, 6-action shield with a ChainLock proof fits), so I dropped it as well: signing_tests.rs is back to the base.
|
Bots are done — your move: address shumkov requested changes, then post |
github-actions
Bot
added
waiting-self-review
…ty by test Revert decode_raw_state_transitions v0 and v1 to the base: replay of shipped protocol versions must not depend on the unversioned, client-facing validate_state_transition_size. A drive-abci test now runs every protocol version at max and max + 1 bytes and asserts the decoder's size verdict and error match the helper. Drop the "consensus depends on this" paragraph from the helper doc, the tracker references from code comments, and the DPP test that pinned the per-input cost of an InstantSend proof against today's limit (a premise DPP does not own; the chain-proof fit is covered by the seeding test). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. 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 size validation and InstantSend-to-ChainLock fallback are generally consistent, but rejected ChainLock proofs can be demoted into a recovery state that cannot reconstruct a replacement proof when the finalized transaction record has been evicted. This leaves automatic wallet catch-up unable to complete the asset lock, while a retained record can cause the same rejected height to be retried. The shared fallback also still lacks a test that reaches an actual funding call site; Rust workspace and suite checks were still in progress in the supplied CI snapshot.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Do not demote a rejected ChainLock proof into a record-only recovery path
packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:603-616
After a InvalidAssetLockProofTransactionHeightError, this code probes wait_for_proof and demotes the row to Broadcast with no proof when the record is missing or only reproduces the rejected ChainLock proof. The (Broadcast, None) resume path re-broadcasts and calls wait_for_proof, but that path only reads the in-memory/persisted transaction record; it does not invoke the mined-height locator used by upgrade_to_chain_lock_proof/wait_for_chain_lock. With the default keep-finalized-transactions feature disabled and a persistence backend returning Ok(None), a transaction can already be ChainLocked while its record has been evicted, leaving automatic catch-up unable to construct any proof and repeatedly timing out. If the record is retained, the same rejected ChainLock height can be read back and submitted again. Preserve a usable replacement only after locating a different height, or make this post-rejection recovery force the SPV-verified mined-height locator path.
source: gpt-6.1-sol (phase2-reviewer: general)
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: muse-spark-1.3-contributor (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, intricate change affecting funds movement and asset-lock proof handling across wallet, SDK, DPP, DAPI, and Drive state-transition decoding, including peer-facing deserialization and protocol-versioned behavior. - 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 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/sync/tracking.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:603-616: Do not demote a rejected ChainLock proof into a record-only recovery path
After a `InvalidAssetLockProofTransactionHeightError`, this code probes `wait_for_proof` and demotes the row to `Broadcast` with no proof when the record is missing or only reproduces the rejected ChainLock proof. The `(Broadcast, None)` resume path re-broadcasts and calls `wait_for_proof`, but that path only reads the in-memory/persisted transaction record; it does not invoke the mined-height locator used by `upgrade_to_chain_lock_proof`/`wait_for_chain_lock`. With the default `keep-finalized-transactions` feature disabled and a persistence backend returning `Ok(None)`, a transaction can already be ChainLocked while its record has been evicted, leaving automatic catch-up unable to construct any proof and repeatedly timing out. If the record is retained, the same rejected ChainLock height can be read back and submitted again. Preserve a usable replacement only after locating a different height, or make this post-rejection recovery force the SPV-verified mined-height locator path.
In `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:512-534: Exercise a real size refusal through the wallet funding fallback
(existing thread: https://github.com/dashpay/platform/pull/5460#discussion_r4338186643)
The shared `submit_with_chain_lock_fallback` tests now verify persistence and resumability, but they inject `oversized()` directly into a scripted closure. No test reaches this shielded funding call site with an actually oversized InstantSend transition, so a regression in the closure wiring, proof forwarding, or ChainLock rebuild could leave the production path broken while the shared-helper tests remain green. Add a deterministic wallet-level test around this funding closure, using a mocked submit/build boundary if constructing a real Halo 2 proof is too expensive.
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.
- Separate the ChainLock wait from the shield guard — A fresh oversized shield can hold the shield guard while waiting indefinitely for ChainLock/SPV progress, blocking other shield operations. The existing InstantSend-timeout path has the same lifecycle behavior, and refactoring the guard scope requires coordinating key binding, removal, and both finality paths rather than changing only this branch.
- Follow-up: Continue the tracked guard-scope refactor in #5518.
… record kept A ChainLock proof Platform refuses for its height is taken off the row, which drops to Broadcast with no proof. With the funding record evicted and a persister that keeps none, the existing-lock resume's record-only wait expires into the IS-timeout outcome, and the fallback rebuilds the proof through the SPV-verified mined-height lookup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@thepastaclaw on the 🔴 "Do not demote a rejected ChainLock proof into a record-only recovery path" ( Evicted record, persister answers Retained record. Here the record is the only height evidence there is. Both cases are pre-existing behaviour of On the 🟡 (a wallet-level size refusal through the shielded call site): same answer as in r4338186643. Reaching that closure with a really oversized transition needs a Halo 2 proof per run. Since e09f238 all four flows go through |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
|
@romchornyi on it — I've queued this as job |
|
Thanks for checking both paths and adding the regression test. This is follow-up to an existing review finding; reviewsys owns review disposition and will answer/re-review it, so I’m leaving this with reviewsys rather than taking further action here. |
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Updated after discussion: 1 finding(s) of this review were settled in the inline threads; 1 blocking finding(s) still stand.
- ✅ Withdrawn: 🔴 Keep the oversized-transition fixture below the serializer cap
The change introduces a recovery gap when a rejected ChainLock proof is demoted to a proof-less Broadcast row: the public generic resume path still waits only on transaction records and does not use the mined-height locator, so an evicted record can remain unrecoverable there. One newly added SDK test also cannot reach the typed size validator because its fixture exceeds the StateTransition serializer's 100,000-byte cap first. CI otherwise has successful relevant jobs; the Rust workspace failure is in an unchanged query initializer, and one browser shard remains in progress.
🔴 2 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Do not demote a rejected ChainLock proof into a record-only recovery path
packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:603-616
When this invalidator clears a rejected ChainLock proof, it stores the row as (Broadcast, None). The public generic resume_asset_lock path, including asset_lock_manager_resume and launch catch-up, enters the Broadcast arm and calls wait_for_proof, which only re-reads in-memory or persisted transaction records; it does not invoke the mined-height locator used by upgrade_to_chain_lock_proof. If keep-finalized-transactions has evicted the record and the persister returns Ok(None), that generic resume can only time out or report an unconfirmed broadcast. The new regression covers the specialized funding path through resolve_chain_proof_after_is_timeout, but not this public generic resume surface. Either make generic recovery use the locator after this demotion or retain a state that routes every resume through the locator-backed ChainLock recovery.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-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:
criticalbygpt-6.1-sol(effort low) — This is a large cross-cutting change to asset-lock proof handling and wallet funding orchestration, directly affecting funds movement and cryptographic proof selection inasset_lock_submission::needs_chain_lock_proofand the platform-wallet asset-lock flows. - 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— 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/sync/tracking.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:603-616: Do not demote a rejected ChainLock proof into a record-only recovery path
When this invalidator clears a rejected ChainLock proof, it stores the row as `(Broadcast, None)`. The public generic `resume_asset_lock` path, including `asset_lock_manager_resume` and launch catch-up, enters the Broadcast arm and calls `wait_for_proof`, which only re-reads in-memory or persisted transaction records; it does not invoke the mined-height locator used by `upgrade_to_chain_lock_proof`. If `keep-finalized-transactions` has evicted the record and the persister returns `Ok(None)`, that generic resume can only time out or report an unconfirmed broadcast. The new regression covers the specialized funding path through `resolve_chain_proof_after_is_timeout`, but not this public generic resume surface. Either make generic recovery use the locator after this demotion or retain a state that routes every resume through the locator-backed ChainLock recovery.
In `packages/rs-sdk/src/platform/transition/broadcast_request.rs`:
- [BLOCKING → WITHDRAWN in discussion; do not fix] packages/rs-sdk/src/platform/transition/broadcast_request.rs:186-200: Keep the oversized-transition fixture below the serializer cap
The `StateTransition` derive uses `#[platform_serialize(limit = 100000)]`, and `STATE_TRANSITION_MAX_ENCODED_BYTES` is also 100,000. This fixture puts 100,000 bytes into the signature and adds the enum and transition framing, so `serialize_to_bytes()` fails at the serialization limit inside `broadcast_request_for_state_transition` before `validate_state_transition_size` runs. Consequently `checked_broadcast_request` returns a serialization `MaxEncodedBytesReachedError`, while `is_size_exceeded` expects `StateTransitionMaxSizeExceededError`; the test will fail instead of verifying the intended typed size error. Use a fixture larger than the protocol limit but below 100,000 bytes, such as a signature sized to `STATE_TRANSITION_MAX_ENCODED_BYTES / 2`, and assert that the resulting length exceeds `max_size()`.
In `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:514-533: Exercise a real size refusal through the wallet funding fallback
(existing thread: https://github.com/dashpay/platform/pull/5460#discussion_r4338186643)
The shielded call site now passes its transition-building closure to `submit_with_chain_lock_fallback`, but the added tests invoke that shared helper with a scripted error rather than exercising this closure or constructing a real `ShieldFromAssetLock` transition. The shared helper tests prove that a typed size refusal reaches a ChainLock resubmission, yet they would remain green if this call site later passed the wrong proof into the closure or bypassed the fallback while constructing the shielded transition. Add a lightweight call-site-level regression, or otherwise assert that the shielded closure receives and submits the replacement ChainLock proof.
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.
- Rust workspace CI failure is outside this change — The Rust workspace check fails while compiling the unchanged
packages/rs-sdk/src/platform/identities_contract_keys_query.rs, whose QuerySettings-related initializers lackverified_protocol_version. The base branch contains the subsequent fix, and no base commit touches the files changed by this pull request.- Follow-up: Rebase onto the current v5.1-dev tip or fix the query initializer separately before relying on the workspace check.
| fn should_refuse_a_transition_past_the_encoded_bytes_budget_with_the_typed_size_error() { | ||
| let transition = StateTransition::IdentityTopUp(IdentityTopUpTransition::V0( | ||
| IdentityTopUpTransitionV0 { | ||
| signature: BinaryData::new(vec![0u8; STATE_TRANSITION_MAX_ENCODED_BYTES]), | ||
| ..Default::default() | ||
| }, | ||
| )); | ||
|
|
||
| let error = checked_broadcast_request(&transition, PlatformVersion::latest()) | ||
| .expect_err("an oversized transition is refused"); | ||
|
|
||
| assert!( | ||
| is_size_exceeded(&error, |actual| actual | ||
| > STATE_TRANSITION_MAX_ENCODED_BYTES as u64), | ||
| "unexpected error: {error:?}" |
There was a problem hiding this comment.
✅ Withdrawn at
4d413cf2; see the replies below.
🔴 Blocking: Keep the oversized-transition fixture below the serializer cap
The StateTransition derive uses #[platform_serialize(limit = 100000)], and STATE_TRANSITION_MAX_ENCODED_BYTES is also 100,000. This fixture puts 100,000 bytes into the signature and adds the enum and transition framing, so serialize_to_bytes() fails at the serialization limit inside broadcast_request_for_state_transition before validate_state_transition_size runs. Consequently checked_broadcast_request returns a serialization MaxEncodedBytesReachedError, while is_size_exceeded expects StateTransitionMaxSizeExceededError; the test will fail instead of verifying the intended typed size error. Use a fixture larger than the protocol limit but below 100,000 bytes, such as a signature sized to STATE_TRANSITION_MAX_ENCODED_BYTES / 2, and assert that the resulting length exceeds max_size().
| fn should_refuse_a_transition_past_the_encoded_bytes_budget_with_the_typed_size_error() { | |
| let transition = StateTransition::IdentityTopUp(IdentityTopUpTransition::V0( | |
| IdentityTopUpTransitionV0 { | |
| signature: BinaryData::new(vec![0u8; STATE_TRANSITION_MAX_ENCODED_BYTES]), | |
| ..Default::default() | |
| }, | |
| )); | |
| let error = checked_broadcast_request(&transition, PlatformVersion::latest()) | |
| .expect_err("an oversized transition is refused"); | |
| assert!( | |
| is_size_exceeded(&error, |actual| actual | |
| > STATE_TRANSITION_MAX_ENCODED_BYTES as u64), | |
| "unexpected error: {error:?}" | |
| /// A transition larger than the protocol budget but below the | |
| /// StateTransition serializer cap still gets the typed protocol-size error. | |
| #[test] | |
| fn should_refuse_a_transition_above_the_protocol_budget_with_the_typed_size_error() { | |
| let transition = StateTransition::IdentityTopUp(IdentityTopUpTransition::V0( | |
| IdentityTopUpTransitionV0 { | |
| signature: BinaryData::new(vec![0u8; STATE_TRANSITION_MAX_ENCODED_BYTES / 2]), | |
| ..Default::default() | |
| }, | |
| )); | |
| let error = checked_broadcast_request(&transition, PlatformVersion::latest()) | |
| .expect_err("an oversized transition is refused"); | |
| assert!( | |
| is_size_exceeded(&error, |actual| actual > max_size() as u64), | |
| "unexpected error: {error:?}" | |
| ); | |
| } |
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
The test passes, locally (cargo test -p dash-sdk --lib -- transition::broadcast_request: 3 passed) and in every CI run since it was added. #[platform_serialize(limit = 100000)] does not stop encoding. serialize_enum.rs builds the config with with_limit::<100000>(), but bincode 2 checks that limit only when decoding. encode_to_vec writes into a growing Vec and never compares against it, so serialize_to_bytes() returns the full ~100 KB and validate_state_transition_size refuses it with the typed error. That is the behaviour this test pins: a transition past STATE_TRANSITION_MAX_ENCODED_BYTES (an IS proof over a funding tx of ~500 inputs) still encodes, so it gets the same typed error with its real length. A fixture under 100 KB would no longer cover that case. The 21 KB case is should_refuse_an_oversized_transition_with_the_typed_size_error just above. Keeping the test as it is.
There was a problem hiding this comment.
I checked the derive and bincode implementation: with_limit::<100000>() is passed to the encoder, but encode_to_vec uses a growing VecWriter whose write implementation does not enforce that byte limit. The fixture therefore reaches validate_state_transition_size and can correctly exercise the typed error with an encoded length above 100,000 bytes; my original finding was incorrect.
Withdrawing this finding.
|
@thepastaclaw on the repeated 🔴 "Do not demote a rejected ChainLock proof into a record-only recovery path": I'm keeping the code as it is. I agree that the generic
The 🔴 on |
|
@romchornyi on it — I've queued this as job |
|
Thanks for the detailed clarification. I understand the generic-resume behavior and the scope rationale; no code change is needed from me on these points. reviewsys handles the review and will re-review any pushed changes. |
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 one 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.validate_or_upgrade_proofdeliberately unchanged —upgrade_to_chain_lock_proofalready builds the proof for a record that a ChainLock evicted from memory (regression test added).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.cargo test -p platform-wallet1573 (--all-features) / 1329 (default),-p dash-sdk413,-p platform-wallet-ffibuilds; clippy clean on the touched crates.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
New Features
Bug Fixes
PR Hygiene ·
4d413cf/skip-botsproceeds without the ones not yet reported/self-reviewedrust-dapi(packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs) — QuantumExplorer or lklimek or shumkovdpp(packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs,packages/rs-dpp/src/state_transition/mod.rs) — QuantumExplorer or shumkovrs-drive-abci(packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/decode_raw_state_transitions/mod.rs) — QuantumExplorer or shumkovrs-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 6 more) — HashEngineering or ZocoLini or 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) — lklimek or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.