fix(platform-wallet): couple a sweep's payment flips to their own persistence round - #4442
fix(platform-wallet): couple a sweep's payment flips to their own persistence round#4442romchornyi wants to merge 2 commits into
Conversation
…sistence round Reattach the bit-11 behavioral block extracted from the sweep core PR, byte-identical to how it was reviewed there: SweptPaymentFlips / PaymentFlipUndo and the evidence-classed resolve_sent_payment_by_txid with the shared sent-status transition table in payments.rs; the wallet-event adapter's flip staging with same-fold retraction (retract_reinstated_payment_flips), the rollback ledger and rejected-wallet replay, cross-drain re-validation under the manager read lock (commit_batch_with_payment_revalidation / retract_superseded_payment_flips) and WalletBatch::payments_overlay; the adapter-owned reinstatement confirmation riding the reinstating record's round; and the ROUND_COUPLED_PAYMENT_FLIPS composite (DASHPAY_PAYMENTS | ATOMIC_CHANGESETS) that gates all staging. A backend failing the composite degrades to a payments-blind host: the in-memory flip still happens with nothing round-coupled — funds-safe, since payment entries are display metadata and the funds-critical half gates on CORE_SWEEP_REMOVAL in the base PR. Co-Authored-By: Claude Opus 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 ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
⛔ Blockers found — Opus deferred (commit 51b9bb9) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The round-coupled persistence and rollback paths are well tested, but the live confirmation path still makes an invalid temporal assumption: independently spawned hooks can apply an older confirmation after a newer sweep. The transition table should also enumerate its legal edges explicitly instead of allowing every destination from Pending.
Source: Codex reviewer evidence (codex-general, codex-rust-quality, codex-security-auditor, and codex-ffi-engineer); final verifier: Claude Agent SDK. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:1009-1012: Order live confirmation hooks against newer sweeps
`LIVE_CONFIRM_EVIDENCE` treats the payment's current state when a hook executes as proof that the hook's event postdates the sweep, but each wallet event is cloned into an independently spawned task in `DashPayPaymentHandler::on_wallet_event`, so execution order does not preserve emission order. Upstream explicitly permits a chainlocked transaction to evict an earlier InstantSend-locked conflicting transaction. If that earlier confirmation task is delayed until after the newer sweep stores `Failed`, this set authorizes the stale `Failed -> Confirmed` write; if it runs just before the adapter stages the sweep, its `Pending -> Confirmed` write makes the terminal-state check skip the newer failure. Either interleaving can leave the dead payment durably `Confirmed`, and later sweeps cannot repair it because `Confirmed` is terminal. Route sent-payment verdicts through the ordered persistence adapter or carry an event sequence/generation that is checked under the manager lock. Add a regression that parks a pre-sweep confirmation hook until the newer sweep has been emitted.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:1368-1372: Enumerate the payment state-machine transitions explicitly
The documented state machine permits `Pending -> Confirmed`, `Pending -> Failed`, and `Failed -> Confirmed`, but `(PaymentStatus::Pending, _)` also accepts `Pending -> Pending` and will silently accept every future `PaymentStatus` variant. Because this function is the shared transition authority, enumerate the legal edges so extending the enum forces an explicit state-machine review.
| const LIVE_CONFIRM_EVIDENCE: &[crate::wallet::identity::types::dashpay::payment::PaymentStatus] = &[ | ||
| crate::wallet::identity::types::dashpay::payment::PaymentStatus::Pending, | ||
| crate::wallet::identity::types::dashpay::payment::PaymentStatus::Failed, | ||
| ]; |
There was a problem hiding this comment.
🔴 Blocking: Order live confirmation hooks against newer sweeps
LIVE_CONFIRM_EVIDENCE treats the payment's current state when a hook executes as proof that the hook's event postdates the sweep, but each wallet event is cloned into an independently spawned task in DashPayPaymentHandler::on_wallet_event, so execution order does not preserve emission order. Upstream explicitly permits a chainlocked transaction to evict an earlier InstantSend-locked conflicting transaction. If that earlier confirmation task is delayed until after the newer sweep stores Failed, this set authorizes the stale Failed -> Confirmed write; if it runs just before the adapter stages the sweep, its Pending -> Confirmed write makes the terminal-state check skip the newer failure. Either interleaving can leave the dead payment durably Confirmed, and later sweeps cannot repair it because Confirmed is terminal. Route sent-payment verdicts through the ordered persistence adapter or carry an event sequence/generation that is checked under the manager lock. Add a regression that parks a pre-sweep confirmation hook until the newer sweep has been emitted.
source: ['codex']
There was a problem hiding this comment.
Resolved in 51b9bb9 — Order live confirmation hooks against newer sweeps no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| use crate::wallet::identity::types::dashpay::payment::PaymentStatus; | ||
| matches!( | ||
| (from, to), | ||
| (PaymentStatus::Pending, _) | (PaymentStatus::Failed, PaymentStatus::Confirmed) | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Enumerate the payment state-machine transitions explicitly
The documented state machine permits Pending -> Confirmed, Pending -> Failed, and Failed -> Confirmed, but (PaymentStatus::Pending, _) also accepts Pending -> Pending and will silently accept every future PaymentStatus variant. Because this function is the shared transition authority, enumerate the legal edges so extending the enum forces an explicit state-machine review.
| use crate::wallet::identity::types::dashpay::payment::PaymentStatus; | |
| matches!( | |
| (from, to), | |
| (PaymentStatus::Pending, _) | (PaymentStatus::Failed, PaymentStatus::Confirmed) | |
| ) | |
| use crate::wallet::identity::types::dashpay::payment::PaymentStatus; | |
| matches!( | |
| (from, to), | |
| (PaymentStatus::Pending, PaymentStatus::Confirmed) | |
| | (PaymentStatus::Pending, PaymentStatus::Failed) | |
| | (PaymentStatus::Failed, PaymentStatus::Confirmed) | |
| ) |
source: ['codex']
There was a problem hiding this comment.
Resolved in 51b9bb9 — Enumerate the payment state-machine transitions explicitly no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…n the wallet-event adapter The DashPay payment hooks confirmed sent payments from independently spawned tasks off dash-spv's bounded, lossy event broadcast, so execution order did not preserve emission order. Upstream permits a chainlocked transaction to evict an IS-locked conflict, so a pre-sweep confirmation task delayed past the newer sweep flipped the durable Failed verdict back to Confirmed (LIVE_CONFIRM_EVIDENCE admitted Failed), and one that ran just before the sweep staged made the terminal-state check skip the failure — either way a dead payment ended durably Confirmed with no later sweep re-emission to repair it. Route every sent-payment verdict through the adapter's single ordered drain of the lossless persistence channel instead: - confirm_final_sent_payments_for_store generalizes the reinstatement-only confirm to ALL finality evidence (final records, and TransactionInstantLocked by txid), staging Pending/Failed -> Confirmed on the event's own store round; the hooks now only record incoming payments (idempotent inserts with no state machine to race). - flip_swept_sent_payments_for_store gains Confirmed -> Failed: with writers ordered, every Confirmed visible at sweep-fold time was written from evidence the sweep postdates, and upstream never sweeps a currently final record, so the newer sweep verdict must win. - sent_status_transition_allowed enumerates its legal edges explicitly (review suggestion): the (Pending, _) wildcard admitted Pending -> Pending and would silently admit any future PaymentStatus variant. - The commit-stage re-validation (commit_batch_with_payment_revalidation / retract_superseded_payment_flips) is removed as redundant: its only purpose was dropping staged rows an unordered hook had outrun, and no such writer remains — the reconcile pass, the one off-adapter confirmer, persists memory-and-store atomically under the manager write lock with Pending-only evidence, so staged Failed/Confirmed rows cannot be superseded between fold and store. Regressions cover both reviewed interleavings by parking a pre-sweep IS-lock confirmation hook until the newer sweep is durable and then releasing it, plus the same-fold [IS-lock, Swept] case committing only the newer verdict, and an exhaustive transition-table test.
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The ordered-adapter change and explicit transition table resolve both prior findings. One blocking fold-journal defect remains: payment overlays coalesce repeated verdicts, while their rollback ledger retains every intermediate write, breaking both rejected-round rollback and same-fold reinstatement. Source: Codex reviewer backend — gpt-5.6-sol; final verifier backend — grok-4.5 via Claude Agent SDK; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/changeset/core_bridge.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1029-1032: Coalesce repeated flips before recording the rollback
`payments_overlay` folds repeated writes with last-write-wins semantics, but `payment_rollbacks` appends every intermediate undo. In a rejected buffered `[TransactionInstantLocked(X), TransactionsSwept(X)]` round starting from durably `Pending`, memory ends at `Failed` and the ledger contains `Pending → Confirmed` followed by `Confirmed → Failed`; the forward guarded rollback skips the first undo and only restores `Confirmed`, rather than the durable `Pending` state. The same mismatch breaks a `[finality(X), sweep(X), reinstating record(X)]` fold: retraction removes the final `Failed` row and undo, restoring memory to `Confirmed`, but it does not restore the overwritten `Confirmed` overlay, so the subsequent confirmation no-ops and the reinstating record can commit without its payment correction. Coalesce rollback state per `(owner, txid)`, retaining the first pre-round entry and final staged status, and make same-fold retraction update that coalesced state consistently. Add rejected-store and three-event reinstatement regressions.
| payment_rollbacks | ||
| .entry(wallet_id) | ||
| .or_default() | ||
| .extend(flips.rollback); |
There was a problem hiding this comment.
🔴 Blocking: Coalesce repeated flips before recording the rollback
payments_overlay folds repeated writes with last-write-wins semantics, but payment_rollbacks appends every intermediate undo. In a rejected buffered [TransactionInstantLocked(X), TransactionsSwept(X)] round starting from durably Pending, memory ends at Failed and the ledger contains Pending → Confirmed followed by Confirmed → Failed; the forward guarded rollback skips the first undo and only restores Confirmed, rather than the durable Pending state. The same mismatch breaks a [finality(X), sweep(X), reinstating record(X)] fold: retraction removes the final Failed row and undo, restoring memory to Confirmed, but it does not restore the overwritten Confirmed overlay, so the subsequent confirmation no-ops and the reinstating record can commit without its payment correction. Coalesce rollback state per (owner, txid), retaining the first pre-round entry and final staged status, and make same-fold retraction update that coalesced state consistently. Add rejected-store and three-event reinstatement regressions.
source: ['codex']
Issue being fixed or feature implemented
Extracted from #4406 to shrink the surface a reviewer has to hold at once. This is the payment half of the swept-transaction work: what happens to a DashPay sent payment when the transaction that carried it loses a double-spend race, and what happens when a chainlocked reinstatement brings it back.
The code here is unchanged from #4406 — it is the same commits, moved. Every finding listed below was raised and resolved there, on the linked threads, and each fix arrives with the regression test that pins it. Nothing is open against this block: the two findings still live on #4406 (the balance-handler
try_readsnapshot and the JNI winner-array allocation) belong to the remainder and stayed there.Based on
chore/bump-rust-dashcore-dev-961(#4406), so the diff shows only the payments block. It rebases ontov4.2-devonce #4406 merges.What was done?
A sweep's payment consequence rides the sweep's own store round, rather than being persisted separately and hoping for a retry.
The flip. When a sweep removes the transaction that carried a
Sentpayment, the entry flipsPending → Failed. That flip is staged as adashpay_payments_overlayrow on the samePlatformWalletChangeSetas the sweep, so a rejectedstore()discards both together, the wallet faults, and the replayed sweep recomputes the flip. The alternative — persisting it on its own round — loses it exactly once, permanently: a sweep never re-emits once its round is durable.The reinstatement. A chainlocked reinstatement corrects
Failed → Confirmed, and it too rides the reinstating record's own round, for the same reason in reverse: that correction is one-shot, since a record that arrived already chainlocked gets no later detection to retry from.Ordering. Three mechanisms compose so no writer can overwrite a newer verdict:
Evidence classes.
resolve_sent_payment_by_txidnow takes what the caller's evidence can speak for, intersected with the shared transition table: a live signal may applyFailed → Confirmed(a live event for a dead txid is authoritative reinstatement), a reconciler snapshot may not (its read can predate a racing sweep's verdict).Capability. Staging is gated on
ROUND_COUPLED_PAYMENT_FLIPS=DASHPAY_PAYMENTS | ATOMIC_CHANGESETS. The payments bit alone attests per-callback durability, which a host whose callbacks commit independently truthfully provides — but on such a host the record and watermark can commit before the payments write, stranding a one-shot reinstatement beside a durablyFailedpayment. A host failing the gate keeps the in-memory flip with nothing round-coupled: funds-safe, since payment entries are display metadata, and the funds-critical half of the sweep still gates onCORE_SWEEP_REMOVAL.Findings resolved here, with the test that pins each
swept_payment_flip_rides_the_sweeps_round_and_rolls_back_on_rejectionFailedoverlay on the round (r3805302098 family)a_reinstating_record_in_the_same_fold_retracts_the_payment_flipa_confirmation_landing_before_the_sweeps_store_retracts_its_stale_failed_rowrollback_does_not_clobber_a_concurrently_confirmed_entryConfirma swept paymenta_stale_reconcile_snapshot_cannot_confirm_a_swept_paymenta_chainlocked_reinstatement_rides_the_records_round_and_survives_rejectionROUND_COUPLED_PAYMENT_FLIPScompositean_atomicity_blind_backend_is_not_handed_payment_flips_on_the_rounda_payments_blind_backend_is_not_handed_the_sweeps_flip_on_the_roundHow Has This Been Tested?
Every regression above was verified failing without its fix — each was revert-tested individually, not merely observed passing.
Gates on this branch:
platform-wallet697,platform-wallet-ffi277,platform-wallet-storageall suites including 24/24 sweep,cargo clippy --workspace --all-targetsclean,cargo fmt --all -- --checkclean.The extraction itself was verified lossless: with this branch's commit applied on top of the removal commit on #4406,
git diffagainst #4406's pre-extraction head is empty.No Swift or Kotlin code is touched, so those suites are unaffected.
Breaking Changes
None.
ROUND_COUPLED_PAYMENT_FLIPSis a new composite over existing bits; no bit value changes and no FFI signature changes. A host that attestsDASHPAY_PAYMENTSwithoutATOMIC_CHANGESETSstops receiving round-coupled overlays and keeps the in-memory flip — a deliberate narrowing, and funds-safe.Checklist: