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 17 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
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit 34fe4ef. Normal review starts when eligible; priority review starts as soon as a slot is available. |
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
|
@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
1831b7d9; 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.
There was a problem hiding this comment.
Withdrawn (re-reviewed at 1831b7d9): The fixture is valid because bincode 2 does not enforce the configured limit during encoding; it reaches the typed protocol-size check and exercises the intended error.
|
@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. |
…rove it A resumed asset lock whose funding record is gone (evicted after its ChainLock promotion, or never kept by the persister), has no height, or is contradicted by the SPV headers could only wait out its bound on the record-only proof wait. The generic resume (host resume, launch catch-up) then ended in FinalityTimeout or TransactionBroadcastUnconfirmed on every retry, most directly for a row whose rejected ChainLock proof invalidate_rejected_chain_proof had cleared. The three waiting resume arms (Built, Broadcast, proof-less RecoveredFromChain) now use wait_for_resumed_proof, which also asks the mined-height locator, within the wait's deadline, and builds a ChainLock proof once the SPV header chain and the wallet's ChainLock cover the placement, with the same throttle and the same record gate as the ChainLock wait. A zero bound makes no lookup. The fresh funding wait stays record-only, so a just-broadcast lock makes no DAPI request. 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": fixed in 1831b7d. This replaces my earlier "out of scope" answer: the generic resume now uses the locator too.
New tests in
|
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
|
@romchornyi on it — I've queued this as job |
|
@romchornyi thanks for pushing the generic-resume locator fallback and regression coverage. reviewsys handles the review disposition and will answer on the finding thread and re-review on push; I am leaving this with reviewsys. |
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 centralizes the InstantSend-to-ChainLock fallback and adds locator-backed recovery for resumed locks whose records cannot prove finality. Two wallet recovery gaps remain: rejected ChainLock proofs on RecoveredFromChain rows are not cleared, and defensive rebroadcast rejection can return before the new locator path is attempted. Rust workspace CI currently fails in an unchanged rs-sdk test initializer, while two suite jobs remain in progress.
🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Invalidate rejected ChainLock proofs on RecoveredFromChain rows
packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:621-635
The shared submission helper now calls invalidate_rejected_chain_proof for every failed ChainLock submission, but both conditional updates still require the current status to be ChainLocked. resume_asset_lock preserves RecoveredFromChain and returns its stored ChainLock proof, so a recovered row whose proof is rejected for its height remains unchanged. Subsequent funding resumes return and resubmit the same rejected proof indefinitely, bypassing the locator-backed recovery added by this PR. Handle RecoveredFromChain as an expected current status for this cleanup, while preserving its consumption-unknown semantics, and add a regression covering a recovered row with a rejected ChainLock proof.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🟡 Suggestion: Run the locator fallback before returning on a rejected defensive rebroadcast
packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1503-1518
The new wait_for_resumed_proof locator path is reached only after the defensive rebroadcast branch completes. When a Broadcast row has no local proof, the record is evicted, and SpvBroadcaster returns BroadcastError::Rejected because no peers are connected, the zero-duration record probe fails and the branch returns TransactionBroadcastUnconfirmed before calling wait_for_resumed_proof. If the transaction was already mined, the DAPI-plus-SPV locator could still recover its ChainLock proof, but this launch/offline path never tries it. Route the no-proof rejection path through a bounded locator-aware wait before returning, while retaining the unknown-outcome and no-dispatch semantics.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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: 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 cross-cutting change that alters asset-lock proof handling and transition submission for wallet funds movement, including cryptographic InstantSend/ChainLock proof paths and protocol-versioned decoding. - 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`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:621-635: Invalidate rejected ChainLock proofs on RecoveredFromChain rows
The shared submission helper now calls `invalidate_rejected_chain_proof` for every failed ChainLock submission, but both conditional updates still require the current status to be `ChainLocked`. `resume_asset_lock` preserves `RecoveredFromChain` and returns its stored ChainLock proof, so a recovered row whose proof is rejected for its height remains unchanged. Subsequent funding resumes return and resubmit the same rejected proof indefinitely, bypassing the locator-backed recovery added by this PR. Handle `RecoveredFromChain` as an expected current status for this cleanup, while preserving its consumption-unknown semantics, and add a regression covering a recovered row with a rejected ChainLock proof.
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-1518: Run the locator fallback before returning on a rejected defensive rebroadcast
The new `wait_for_resumed_proof` locator path is reached only after the defensive rebroadcast branch completes. When a `Broadcast` row has no local proof, the record is evicted, and `SpvBroadcaster` returns `BroadcastError::Rejected` because no peers are connected, the zero-duration record probe fails and the branch returns `TransactionBroadcastUnconfirmed` before calling `wait_for_resumed_proof`. If the transaction was already mined, the DAPI-plus-SPV locator could still recover its ChainLock proof, but this launch/offline path never tries it. Route the no-proof rejection path through a bounded locator-aware wait before returning, while retaining the unknown-outcome and no-dispatch semantics.
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 shield guard from the unbounded ChainLock fallback wait — A user-facing oversized shield can hold the shield-operation guard while waiting for ChainLock finality. This is a concrete availability and lifecycle concern, but it also affects the pre-existing InstantSend-timeout path and requires a broader guard-scope refactor than this proof fallback.
- Follow-up: Address the shared guard-scope refactor in #5518.
…romChain row invalidate_rejected_chain_proof only acted on ChainLocked rows. A RecoveredFromChain row whose ChainLock proof Platform refused for its height kept it, and every later resume submitted the same proof again. Such a row now takes a different ChainLock proof from the record, or otherwise loses the proof, and keeps its status, which says Platform-side consumption is unknown. It never takes an InstantSend proof. A proof-less RecoveredFromChain row is resumed through the record, then the mined-height lookup. The status is read first and the update is one compare-and-set on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@thepastaclaw on the two 🟡 from the 1831b7d review: "Invalidate rejected ChainLock proofs on RecoveredFromChain rows": fixed in 03e2918.
The status is read first, and the update is one compare-and-set on it. New tests cover the replacement, the clear (an InstantSend record, and a record at the refused height), and the rebuild through the lookup on the next resume. "Run the locator fallback before returning on a rejected defensive rebroadcast": I'm leaving this path as it is. It does not strand the lock:
|
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The reviewed head implements the size-triggered InstantSend-to-ChainLock fallback coherently, with the previously identified persistence, locator, sizing, and decoder concerns addressed. Two non-blocking wallet recovery issues remain: a rejected defensive rebroadcast can bypass locator-backed recovery, and a proofless RecoveredFromChain row can resume with an InstantSend proof. The Rust workspace test failure is reported in an unchanged identities contract-keys query file and appears unrelated; two broader test-suite checks are still running.
🟡 2 suggestion(s)
2 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-1518
After await_broadcast_ready reports readiness, the broadcaster can still lose all peers before broadcast runs. In that case Rejected leaves transport_missed false; when there is no input conflict, the branch returns TransactionBroadcastUnconfirmed at lines 1609-1618 before reaching wait_for_resumed_proof, and no readiness-deferred retry is scheduled. A previously mined lock whose transaction record was evicted therefore cannot use the SPV-verified mined-height locator during this resume, even though the later resumed-proof path supports that recovery. Route this no-proof rejection through the same bounded resumed retry or otherwise schedule a transport-independent follow-up when the row remains tracked.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)
🟡 Suggestion: Do not resume a proof-less RecoveredFromChain row with an InstantSend proof
packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1748-1753
When invalidate_rejected_chain_proof handles a RecoveredFromChain row whose record supplies an InstantSend proof, the current match clears the proof while preserving the RecoveredFromChain status. On the next resume, wait_for_resumed_proof returns that InstantSend record immediately at lines 1191-1198, before the mined-height locator is consulted; validate_or_upgrade_proof then preserves it. The row can consequently be handed the same InstantSend proof again even though RecoveredFromChain is intended to use only a ChainLock proof because Platform-side consumption is unknown. Use the chain-only upgrade path for this status and add a regression covering a cleared row with an InstantSend record and a locator-provided ChainLock height.
let proof = self
.upgrade_to_chain_lock_proof(
out_point,
timeout.or(Some(UNCONFIRMED_BROADCAST_PROOF_TIMEOUT)),
)
.await?;
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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 that modifies funds-movement and asset-lock proof handling across the platform wallet and SDK, including validation and retry behavior for cryptographic proofs. - 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/recovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1503-1518: Run the locator fallback before returning on a rejected defensive rebroadcast
After `await_broadcast_ready` reports readiness, the broadcaster can still lose all peers before `broadcast` runs. In that case `Rejected` leaves `transport_missed` false; when there is no input conflict, the branch returns `TransactionBroadcastUnconfirmed` at lines 1609-1618 before reaching `wait_for_resumed_proof`, and no readiness-deferred retry is scheduled. A previously mined lock whose transaction record was evicted therefore cannot use the SPV-verified mined-height locator during this resume, even though the later resumed-proof path supports that recovery. Route this no-proof rejection through the same bounded resumed retry or otherwise schedule a transport-independent follow-up when the row remains tracked.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:1748-1753: Do not resume a proof-less RecoveredFromChain row with an InstantSend proof
When `invalidate_rejected_chain_proof` handles a `RecoveredFromChain` row whose record supplies an InstantSend proof, the current match clears the proof while preserving the `RecoveredFromChain` status. On the next resume, `wait_for_resumed_proof` returns that InstantSend record immediately at lines 1191-1198, before the mined-height locator is consulted; `validate_or_upgrade_proof` then preserves it. The row can consequently be handed the same InstantSend proof again even though `RecoveredFromChain` is intended to use only a ChainLock proof because Platform-side consumption is unknown. Use the chain-only upgrade path for this status and add a regression covering a cleared row with an InstantSend record and a locator-provided ChainLock height.
… ChainLock proof The proof-less RecoveredFromChain arm took whatever the resumed proof wait returned, so a row whose record holds an InstantSend lock resumed with an InstantSend proof, although the status asserts a ChainLock proof. An InstantSend result is now upgraded through upgrade_to_chain_lock_proof, which builds the proof from the record's height or the mined-height lookup. A ChainLock result is kept as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@thepastaclaw on the two 🟡 from the 03e2918 review: "Do not resume a proof-less RecoveredFromChain row with an InstantSend proof": fixed in 50b7bfe. The arm still enters "Run the locator fallback before returning on a rejected defensive rebroadcast": you're right that my earlier answer was incomplete. A transport that reports ready and then loses its peers before the send leaves
Closing this needs a readiness signal that tracks dash-spv's connected-peer count, or a readiness transition, rather than the snapshot. That is a broadcaster/SPV change, outside this PR. Until then the row stays tracked at |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
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.
- ✅ Withdrawn: 🟡 Avoid consuming a nonce before local size validation
The size-validation and InstantSend-to-ChainLock fallback changes are generally coherent, and the prior recovery, persistence, proof-kind, and decoder concerns are resolved or intentionally deferred. Two in-scope suggestions remain: a rejected defensive rebroadcast can bypass locator-based recovery, and direct transfer/vote paths reserve a nonce before local size validation. Rust workspace failure is unrelated base drift; one browser check remains in progress.
🟡 2 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
After the readiness preflight, the broadcaster can still return Rejected when dash-spv loses its peers before dispatch, while transport_missed remains false. If the zero-duration local proof probe misses, the no-conflict branch returns TransactionBroadcastUnconfirmed at line 1609 without reaching wait_for_resumed_proof, so the new mined-height locator is not consulted. The conflict branch can likewise return a provisional contested result before the locator when readiness had been missed. Because the post-ready race does not schedule resume_when_transport_ready, an already mined and chainlocked transaction with an evicted or unusable record is reported as unresolved until a later explicit resume. Preserve the unknown-outcome safety, but route this rejection through a readiness-aware retry or a bounded locator-backed reconciliation before returning.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, tenderdash-consensus-security)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 16: 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 that directly affects funds movement through asset-lock proof selection and state-transition submission, while also modifying 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 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— 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,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
After the readiness preflight, the broadcaster can still return `Rejected` when dash-spv loses its peers before dispatch, while `transport_missed` remains false. If the zero-duration local proof probe misses, the no-conflict branch returns `TransactionBroadcastUnconfirmed` at line 1609 without reaching `wait_for_resumed_proof`, so the new mined-height locator is not consulted. The conflict branch can likewise return a provisional contested result before the locator when readiness had been missed. Because the post-ready race does not schedule `resume_when_transport_ready`, an already mined and chainlocked transaction with an evicted or unusable record is reported as unresolved until a later explicit resume. Preserve the unknown-outcome safety, but route this rejection through a readiness-aware retry or a bounded locator-backed reconciliation before returning.
In `packages/rs-sdk/src/platform/transition/transfer_document.rs`:
- [SUGGESTION → WITHDRAWN in discussion; do not fix] packages/rs-sdk/src/platform/transition/transfer_document.rs:69-95: Avoid consuming a nonce before local size validation
The direct document-transfer path reserves an identity-contract nonce with `bump_first = true` before constructing the request and calling the new `checked_broadcast_request`. An oversized transition therefore returns a local typed size error after advancing the SDK cache, without dispatching anything. The nonce cache deliberately preserves a higher cached value over Platform's lower value, so repeated oversized submissions can advance the cache beyond the 24-revision future window; a subsequent valid transfer can then be rejected as `NonceTooFarInFuture`. The analogous direct vote paths reserve an identity nonce before the same pre-dispatch size check and need the same treatment. Make nonce reservation transactional for pre-dispatch validation failures, or otherwise release/refresh the reservation before returning, and cover repeated refusals followed by a valid submission.
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 shield guard lifetime from unbounded ChainLock waits — User-facing shielded funding can hold
_shield_guardacross the unbounded ChainLock wait introduced for oversized transitions, blocking other shielded operations while SPV is unavailable. This is a broader lifecycle issue shared with the existing InstantSend-timeout path and is tracked separately.- Follow-up: #5518
| .await?; | ||
| ensure_valid_state_transition_structure(&transition, sdk.version())?; | ||
|
|
||
| let request = transition.broadcast_request_for_state_transition()?; | ||
| let request = checked_broadcast_request(&transition, sdk.version())?; |
There was a problem hiding this comment.
✅ Withdrawn at
50b7bfeb; see the replies below.
🟡 Suggestion: Avoid consuming a nonce before local size validation
The direct document-transfer path reserves an identity-contract nonce with bump_first = true before constructing the request and calling the new checked_broadcast_request. An oversized transition therefore returns a local typed size error after advancing the SDK cache, without dispatching anything. The nonce cache deliberately preserves a higher cached value over Platform's lower value, so repeated oversized submissions can advance the cache beyond the 24-revision future window; a subsequent valid transfer can then be rejected as NonceTooFarInFuture. The analogous direct vote paths reserve an identity nonce before the same pre-dispatch size check and need the same treatment. Make nonce reservation transactional for pre-dispatch validation failures, or otherwise release/refresh the reservation before returning, and cover repeated refusals followed by a valid submission.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
There was a problem hiding this comment.
The size refusal doesn't change what happens to the nonce, so I'm leaving it as it is in this PR.
- Before this change, an oversized transfer or vote reserved the same nonce, went to Platform and was rejected there. Now the same transition is refused locally after that reservation. Either way the cached nonce ends up one ahead and nothing rolls it back.
refresh_identity_noncewould not help either: the cache mergesmax(cached, platform)on the next fetch, so a stale mark never lowers it.ensure_valid_state_transition_structureon the line above already refuses locally after the reservation, so this is the same pre-existing class: a local refusal after a nonce reservation in the direct paths.
A real fix needs the nonce cache to release a reservation that never reached the network. That is a change to internal_cache, used by every transition type, and it belongs in its own PR.
There was a problem hiding this comment.
You're right. get_identity_contract_nonce(..., true) reserved the nonce before request construction both before and after this change; previously the oversized request was sent and rejected by DAPI, while now it is rejected locally by checked_broadcast_request. The cache therefore has the same advancement behavior, so this PR does not introduce or worsen the nonce-reservation issue. A transactional reservation or rollback remains a valid follow-up for the shared cache, but I withdraw this finding against the PR.
Withdrawing this finding.
|
@thepastaclaw on the repeated 🟡 "Run the locator fallback before returning on a rejected defensive rebroadcast" (50b7bfe review): I'm keeping the code as it is. Since your last review there is one new point.
Until then the row stays tracked with its inputs reserved, and the next catch-up or explicit resume recovers it through the locator. |
|
@romchornyi on it — I've queued this as job |
|
@romchornyi thanks for clarifying both readiness paths. reviewsys handles the review disposition and will answer on the finding thread and re-review on push; I am leaving this with reviewsys. |
|
/self-reviewed |
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
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 ·
34fe4efrust-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-ffi(packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs) — HashEngineering or ZocoLini or 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) — 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.