fix(platform-wallet): act on swept transactions at the persistence seam - #4560
fix(platform-wallet): act on swept transactions at the persistence seam#4560romchornyi wants to merge 4 commits into
Conversation
|
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: Team 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 |
|
🔍 Review in progress — actively reviewing now (commit 66a7c74) |
b7f2e47 to
4861a82
Compare
8a655b8 to
cffc998
Compare
|
@coderabbitai review |
|
|
@coderabbitai review |
|
cffc998 to
267ecca
Compare
0c26f6d to
82fe1e3
Compare
267ecca to
48db83c
Compare
82fe1e3 to
deaae54
Compare
48db83c to
f4053ad
Compare
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Approving — the producer logic is sound and well covered. One verification request below that I'd like done before the stack lands; it's not an objection to the code.
Verified while reviewing
Three things in the description I checked rather than took on trust, all of which held up:
The pin bump loses nothing. The SHA-level compare looks alarming — 4db5c367...21aaafed is ahead 22, behind 10, diverged, i.e. ten commits reachable from the old pin are not reachable from the new one, including 4db5c367 itself (#991, the payload-finalization seam this branch's base requires). That's purely an artifact of the curated line having been rebased. Every one of the ten exists on dev under a different SHA:
| missing from new pin | same commit on dev |
|---|---|
4db5c367 #991 payload-finalization seam |
5f2de2e0 |
33030acf #980 non-English BIP-39 parse paths |
b66db390 |
e8928f8b #947 QRInfo masternode-sync recovery |
1a6fb3bf |
3acfdb33 #960 / 9a928518 #964 dash-spv tick fixes |
54b7f7e5 / e9ef99c5 |
| #963, #970, #965, #967 (seeds, bench) | all present |
So "dev carries everything" is accurate. 21aaafed is also a single-parent squash-merge on dev rather than a branch tip that can be rebased away, which makes the pin stable.
The mnemonic adaptation is complete. Swept the workspace for missed call sites. The two remaining Language references are both unaffected: wasm-sdk/src/wallet/key_derivation.rs uses the bip39 crate directly (not key_wallet, note SimplifiedChinese vs ChineseSimplified), and rs-unified-sdk-jni uses FFILanguage on the generation path, where a wordlist is still required. All eight workspace deps and twelve Cargo.lock entries moved consistently; no stale pins.
The partial-reinstatement release is safe. The case I went looking for: a batch removes losers A and B, A's untaken input Y lands in released_outpoints, then A returns chainlocked in the same fold. CoreChangeSet::merge retracts A from txids but deliberately keeps the release set, so on the face of it Y gets freed while a surviving record spends it. #4559 closes this — core_state.rs builds claimed_by_survivors from records no batch sweeps and filters them out of released, then surviving_stored_input_claims re-checks against the unpruned on-disk history, with a_released_coin_a_surviving_record_reclaims_stays_spent pinning it.
Worth stating explicitly somewhere, because the two halves are coupled: the store's veto only works because the merge retracted A from txids first. Had the retraction not happened, A would be in swept_txids, its record would be excluded from the survivor set, and Y would be released. The Swift and Kotlin persisters in PRs 4 and 5 need the same veto — the merge-level retraction alone does not protect them.
The one request: run the Rust checks manually
No Rust CI ran on this PR. tests.yml is gated to master, v*-dev and ci/*, and this targets split/4406-2-storage, so there's been no cargo build, no cargo test, no clippy. (CodeRabbit skipped for the same reason, and the two @coderabbitai review retries hit the rate limit.)
That matters more here than it would on a normal PR: the bump touches 97 files upstream across 22 commits, the whole workspace depends on dashcore transitively, and the Mnemonic::from_phrase signature change is proof that breaking changes ride along. cargo test -p platform-wallet -p platform-wallet-ffi -p platform-wallet-storage covers three crates of roughly thirty — it wouldn't catch a second breaking change landing in rs-drive, rs-dpp, dapi or rs-sdk.
Could you run cargo check --workspace and cargo clippy --workspace on this branch (or dispatch Tests manually — workflow_dispatch is in the triggers) and confirm? If the plan is that full CI catches this on the final PR into v4.2-dev, saying so is enough and this is moot. I'd just rather not discover a compile break at PR 5 of 5 and have to walk it back down the stack.
What's good
- The gate ordering is the load-bearing part and it's right: strip
synced_heightbeforestore(), fault after.a_coalesced_sweep_and_watermark_never_commits_the_heightpins exactly the shape that would otherwise lose data silently, and checking the capability separately from thestore()result — rather than trusting anOkfrom a host that never sawcore.sweeps— is the correct read of the size-negotiated FFI slot. - Documenting
Mergeas "associative but NOT commutative" at the fold, with both order-dependent behaviours named, is the kind of thing that stops a future parallelization from quietly corrupting spend decisions.a_later_sweep_that_keeps_a_coin_spent_outlives_an_earlier_releaseis a real regression test for the union-the-release-sets mistake, not a restatement of the implementation. AssetLockChangeSet::mergenow holds an actual invariant — never an upsert and a tombstone for the same outpoint — instead of depending on stores applying upserts first. Strictly better contract than the one it replaces.- The no-op arms explain why nothing happens and where the consequence actually lands, and
transactions_swept_does_not_drive_payment_hookspins the no-op so a later "fix" can't quietly route it back here.
Non-blocking nits
- Stale doc comment,
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:25-32— still says callers "must walk the language list themselves" and thatkey_wallet::Mnemonic"only exposes language-tagged constructors", which is precisely what #981 removed. It now contradicts the inline comment three lines below it. - Four identical copies of
parse_mnemonic_any_language(platform-wallet-ffi/derivation.rs,identity_keys_from_mnemonic.rs,manager/wallet_lifecycle.rs,rs-sdk-ffi/signer_simple.rs) are now one-line wrappers around the same call with the same error string. Post-#981 there's nothing left to share but the&'static strnarrowing — good moment to collapse or inline them. - Vestigial block in
commit_wallet— the{ … }wrapping the body is a leftover from the loop it was extracted from; dropping it would de-indent the new code by one level. DASHPAY_PAYMENTSintransactions_swept_removes_the_tracked_asset_lock_it_fundedis declared with a comment about the flip's overlay being "staged for a payment-durable backend", but nothing in this diff writesdashpay_payments_overlay— that's #4442. The bit is harmless; the comment describes behaviour that isn't here yet.last_processed_heightisn't stripped alongsidesynced_heightin the sweep gate. That matches the existing #4069 guard exactly, so it's consistent — but the justification given ("a height that claims blocks are scanned while the removal never landed") reads as applying to both watermarks. Deliberate?core_bridge.rs:715— "retried against (hopefully, by then) a capable backend" is optimistic in-session, since the persister doesn't change under a running adapter. Recovery is really "next launch after the host ships its persister", which is what the PR description says.
deaae54 to
46e63c8
Compare
f4053ad to
ec4ec10
Compare
46e63c8 to
2956d22
Compare
ec4ec10 to
3b6c526
Compare
Bumps the rust-dashcore pin to dev and projects the `TransactionsSwept` event the bump brings with it. The two halves are one commit by construction: `WalletEvent` is not `#[non_exhaustive]` and platform has four exhaustive matches over it, so new-pin code cannot compile without the arms — and arms that did nothing would be worse than none, because upstream's removal is unconditional. The wallet drops the losing rows in memory; a store that keeps them replays them at the next load and re-creates the phantom balance the upstream fix exists to kill. The projection is one `SweepBatch` per event, and a sweep-only round is counted in `is_empty_no_records` so a round carrying nothing but a sweep still reaches the persister. The gate is what makes every intermediate host state safe. A backend that has not attested `CORE_SWEEP_REMOVAL` is not known to have applied the round's subtractive half, so its watermark is stripped BEFORE the store and the wallet faults exactly as it would on a rejection — reporting the height durable first and faulting after cannot retract a height a legacy backend already committed. Such a host freezes its sync watermark on the first sweep it meets instead of diverging: fail-closed, funds-safe, and unfrozen the moment its persister ships. A record arriving after a sweep of the same txid retracts that txid from the folded sweep, since persisters write records before replaying sweeps and would otherwise delete a row the wallet has brought back. The asset-lock half mirrors it: a sweep removes the tracked entry its funding transaction created, and `AssetLockChangeSet::merge` now cancels a folded tombstone against a reinstating upsert (and vice versa), so no store ever sees an upsert/tombstone pair for one outpoint whose outcome depends on which it applies first. The pin also carries rust-dashcore#981, which collapses BIP-39 parsing onto one auto-detecting path. Platform's four hand-rolled "try every wordlist" helpers are now that function, and the call sites drop their `Language` argument. It is unrelated to sweeps and rides here only because the sweep chain and the payload-finalization seam this branch's base already depends on both sit above it on dev. `spend_observer`'s two projections gain sweep arms that report no observed spend: a sweep's released outpoints are coins that came back free, and the inputs it kept spent are precisely the ones it does not name, so the held set cannot be derived from the event at all.
…ouched `cargo fmt --check --all` is a CI gate and the collapsed `Mnemonic::from_phrase` calls left two of them wrapped.
Review nits, all documentation. `parse_mnemonic_any_language`'s doc still said `key_wallet::Mnemonic` "only exposes language-tagged constructors" and that callers "must walk the language list themselves" — precisely what rust-dashcore#981 removed, and it contradicted the inline comment three lines below. The wrapper is kept: 20 call sites narrow upstream's error to the `&'static str` they report, and that narrowing is now what the doc says it does. The sweep gate's recovery note read as if a capable backend might appear mid-session. It cannot: the persister does not change under a running adapter, so a host without the slot stays frozen until it ships one and relaunches. Freezing is the point. `last_processed_height` is now documented as deliberately NOT stripped beside `synced_height`, matching the #4069 guard: `synced_height` is the durable "scanned AND persisted" claim that must not outrun an unapplied removal, while `last_processed_height` is the adapter's own progress marker whose retention makes nothing safer. And the asset-lock test's `DASHPAY_PAYMENTS` attestation no longer describes an overlay this PR writes — nothing here stages `dashpay_payments_overlay`; the bit is declared so the fixture still describes a fully capable backend once #4442 lands. Not taken: de-indenting the vestigial block in `commit_wallet`. It spans 152 lines, so removing it would bury the reviewable diff under a whitespace-only change and force another rebase of the four PRs stacked above this one.
…reason CI lints these crates with `-D warnings`, so clippy's seven-argument threshold is an error, and the #4370 merge gave `commit_wallet` an eighth: the `settled` set the panic arm in `run_wallet_event_adapter` reads back to decide which wallets have an unknown outcome. Every parameter is a distinct piece of drain state this function reads and writes, and the borrow split is what keeps them separately mutable — bundling them would rename the same eight.
2956d22 to
b95a50d
Compare
3b6c526 to
66a7c74
Compare
4 participants
Issue being fixed or feature implemented
Nothing yet emits a sweep. This PR bumps the rust-dashcore pin and projects the
TransactionsSweptevent the bump brings with it, so the seam (#4558) and the store (#4559) finally carry the removal a losing double-spend requires.The pin bump and the arms are one commit by construction.
WalletEventis not#[non_exhaustive]and platform has four exhaustive matches over it, so new-pin code cannot compile without the arms — and arms that did nothing would be worse than none, because upstream's removal is unconditional (wallet_checker.rs): the wallet drops the losing rows in memory, and a store that keeps them replays them at the next load.What was done?
The pin
4db5c367→ rust-dashcoredev(21aaafed).Worth knowing why it moves this far:
v4.2-devwas pinned to a curated rebase line (chore/sync-fixes-payload-seam) that deliberately omits the whole sweep chain (#961/#962/#966/#969/#975) but carries #991'sset_payload_finalizer, whichmasternode/update_service.rsnow requires. Our previous pin had the reverse. No revision carrying both existed, so this takesdev, which carries everything.devalso carries rust-dashcore#981, which collapses BIP-39 parsing onto one auto-detecting path. Platform's four hand-rolled "try every wordlist" helpers become that function and the call sites drop theirLanguageargument (13 files). Unrelated to sweeps; it rides here only because the sweep chain and the payload-finalization seam this branch's base already depends on both sit above it ondev.The producer
TransactionsSwept→ oneSweepBatch(core_bridge.rs);is_empty_no_recordscounts sweeps, so a sweep-only round still reaches the persister.balance_handler.rs(routes the post-removal balance snapshot — a sweep is the one event that can lower a balance) andpayment_handler.rs(deliberate no-ops; the payment coupling is fix(platform-wallet): couple a sweep's payment flips to their own persistence round #4442).spend_observer.rsgains sweep arms that report no observed spend: a sweep's released outpoints are coins that came back free, and the inputs it kept spent are precisely the ones it does not name, so the held set cannot be derived from the event at all.The gate — what makes every intermediate host state safe
A backend that has not attested
CORE_SWEEP_REMOVALis not known to have applied the round's subtractive half, so its watermark is stripped before the store and the wallet faults exactly as on a rejection. Order is load-bearing: reporting the height durable first and faulting after cannot retract a height a legacy backend already committed. Such a host freezes its sync watermark on the first sweep it meets instead of diverging — fail-closed, funds-safe, and unfrozen the moment its persister ships (#4406's Swift and Kotlin PRs).Reinstatement
A record arriving after a sweep of the same txid retracts that txid from the folded sweep, since persisters write records before replaying sweeps and would otherwise delete a row the wallet has brought back. The asset-lock half mirrors it: a sweep removes the tracked entry its funding transaction created, and
AssetLockChangeSet::mergecancels a folded tombstone against a reinstating upsert (and vice versa), so no store sees an upsert/tombstone pair whose outcome depends on which it applies first.How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-ffi -p platform-wallet-storage— 928 + 310 + 138 pass, plus every integration suite in those crates.Tests travelling with the change (
core_bridge.rs):sweep_without_declared_capability_freezes_the_wallet_despite_a_successful_storepins the gate;sweep_names_the_dead_transactions_and_nothing_else,sweep_reaches_the_persister,merged_sweeps_stay_separate_and_ordered;transactions_swept_removes_the_tracked_asset_lock_it_fundedanda_reinstating_reconstruction_folded_after_a_sweep_cancels_its_tombstone;transactions_swept_does_not_drive_payment_hookspins the handler no-op.Breaking Changes
None for platform's own API.
Release-timing constraint, not a merge constraint: do not cut a swift-sdk or kotlin-sdk release from a base that contains this PR but not its Swift/Kotlin counterparts. A mobile host at that base freezes its sync watermark on the first sweep it meets — funds-safe, but a user-visible stall. Between merges on
v4.2-devnothing auto-ships.The pin bump also carries rust-dashcore#981's breaking mnemonic API; the platform-side adaptation is included here and is mechanical.
Checklist:
For repository code-owners and collaborators only