Conversation
The SQLite persister replayed its stored transaction records through the wallet checker inside `load()`, so only SQLite rebuilt the in-memory spend guards (`spent_outpoints`, `observed_spent_outpoints`, IS-lock upgrades) that stop a redelivered funding transaction from resurrecting a spent output. Any other persister got no guard for confirmed spends. Move the replay (`replay_order`, lock matching, the persisted-UTXO-set retain filter) into `platform_wallet::manager::history_replay` and run it from `load_from_persistor` for every persister. Persisters now hand their stored history over as `ClientWalletStartState::recorded_history` (`RecordedHistory` / `StoredTransaction`); SQLite supplies its records and locks instead of replaying them itself. The replay runs at the async boundary and awaits the checker, so the first-poll `poll_ready` shim and its suspension rollback are gone. Records an account already holds (a persister's own raw restores) are skipped, and an unconfirmed outgoing send present in the history is only re-dispatched, not accounted twice. BREAKING CHANGE: `ClientWalletStartState` gains the public field `recorded_history`; struct-literal constructors must set it (`Default::default()` keeps the old behaviour). `SqlitePersister::load` no longer returns a replayed projection: callers that bypass `load_from_persistor` must run `replay_recorded_history` themselves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bit 13 attests that a persister's load hands back the wallet's complete stored transaction history, so the shared load replay rebuilds the spend guards for confirmed spends too. The SQLite persister attests it; the FFI mirrors the bit value as a C constant (host wiring follows). Diagnostic only: `load_from_persistor` warns once per load when a persister restores wallets without it, and no operation is gated on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The FFI load path handed Rust only the host's unspent UTXO rows plus its unconfirmed outgoing sends, so confirmed spends were never replayed: after restart a redelivered funding transaction (rescan, gap-limit rediscovery) re-credited an output a confirmed spend had already consumed. Hosts can now hand back every stored transaction through `WalletRestoreEntryFFI::recorded_transactions` (`RecordedTransactionRestoreFFI`: txid, bytes, context, block fields, stored net amount and direction). `build_wallet_start_state` decodes them into `ClientWalletStartState::recorded_history` for the shared load replay. A row whose bytes do not hash to its txid, that does not decode, or that carries an unknown context is dropped with a counted warning; InstantSend rows replay as mempool because the host keeps no lock bytes. The FFI persister attests `CORE_HISTORY_RESTORE` when the host declares it and wires the wallet-list load pair. Android does not supply history yet (null/0, TODO in the JNI bridge). BREAKING CHANGE: `WalletRestoreEntryFFI` gains two trailing fields and has no size field, so the host and the library must be rebuilt together (the same lockstep contract as `unconfirmed_outgoing_tx_records`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`loadWalletList` now hands every wallet-owned `PersistentTransaction` back to Rust as `WalletRestoreEntryFFI.recorded_transactions` (txid, bytes, context, block fields, stored net amount and direction), so the shared load replay rebuilds the spend guards of confirmed spends and a funding transaction redelivered after restart cannot resurrect a spent output. The handler declares `CORE_HISTORY_RESTORE`. A failed history fetch rejects the snapshot (`errored`), like the unspent TXO fetch: an empty history would claim there is nothing to guard. Rows without a 32-byte txid, without bytes, or confirmed without a 32-byte block hash are skipped with one logged count. Not compiled locally (no Swift toolchain on the authoring host). 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 ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced 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 |
|
⛔ Final review complete — 3 blocking finding(s) (commit 04ed0f6) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
History replay is correctly centralized, and the normal FFI path test demonstrates that recorded confirmed spends guard redelivered funding outputs. However, the FFI also raw-inserts unresolved asset-lock records before shared replay, while the new held-txid skip suppresses replay of the same confirmed funding and spender rows; their spend guards therefore remain absent and a redelivered funding transaction can resurrect an already-spent output. This is a blocking correctness issue.
🔴 1 blocking | 🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: 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-language persistence and wallet-replay change that directly alters funds movement and spent-output accounting in platform-wallet load paths, including FFI deserialization and storage restoration. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - 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— 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/manager/history_replay.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/history_replay.rs:93-95: Do not skip history merely because a raw-restored transaction record exists
The `held` set only proves that an account contains a transaction-map entry; it does not prove that the checker restored the transaction's spend effects. The FFI load path raw-inserts unresolved asset-lock records, including confirmed spenders of asset-lock inputs, through `transactions_mut().insert(...)` before `recorded_history` is replayed. Swift supplies those same rows in both the unresolved-asset-lock array and the recorded-history array. Because their txids are already held, this branch skips them, and the raw synthetic records do not populate `spent_outpoints` or `observed_spent_outpoints`. A later redelivery of the funding transaction can therefore credit an output already consumed by the confirmed spender. The existing FFI regression does not include the raw unresolved-record array, so it does not exercise this overlap. Stage retention-only records after shared replay, avoid raw-inserting records that will be checker-replayed, or otherwise rebuild the spend guards independently; add a regression with a raw-restored confirmed spender and funding redelivery.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/history_replay.rs:63-72: Build the held transaction set once instead of scanning all accounts per record
The `held` filter reconstructs `all_accounts()` and scans every account for every stored transaction. Since load replays the full persisted history on each startup, this adds repeated account-collection work proportional to history size. Build an owned txid set from all account transaction maps once before filtering; this also avoids retaining references into `wallet_info` across the subsequent mutable replay.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:7140-7149: Prefetch transaction relationships during history restore
The new history fetch retrieves every `PersistentTransaction`, then evaluates `walletOwnsTransaction` for each restorable wallet. That predicate touches `involvedAccounts`, `outputs`, `inputs`, and `pendingInputs`, but this descriptor does not prefetch any of them. On a cold launch, relationship faults can therefore be triggered repeatedly while filtering the history for each wallet. Prefetch these relationships, or bucket ownership in one pass, to keep history restoration from scaling poorly as wallets and transaction history grow.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:5245-5257: Reject trailing bytes in recorded transaction rows
`consensus_decode(&mut &bytes[..])` accepts a valid transaction prefix without requiring the input cursor to be exhausted. Consequently, a row containing a valid transaction followed by arbitrary bytes passes the txid check and is replayed, despite the decoder's fail-closed contract and the surrounding use of full `deserialize` for other restore paths. Require complete cursor consumption before accepting the row; apply the same exact-consumption rule to the unconfirmed decoder for consistency.
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.
- Define expiry for persisted unconfirmed spend reservations — The shared replay now also replays host-supplied FFI mempool rows, so a never-mined or invalid observed spender can reserve an outpoint across every restart until a conflicting confirmed or InstantSend transaction is observed. This is an availability concern, but the same policy already exists on the SQLite path and is explicitly tracked by the moved expiry TODO rather than being introduced as an implementation mistake in this change.
- Follow-up: Track a separate policy change defining how persisted unconfirmed reservations are revalidated or expired, including safe release of dependent transactions.
Replay raw restored history records through the checker while retaining proof lookup records and user metadata. Sweep fallback records against final transactions, prefetch Swift history relationships, and reject trailing transaction bytes at both restore boundaries. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The shared history replay fixes the confirmed-spend reconstruction gap and the four prior findings are resolved. Two replay-order defects remain: load-time chain-lock finalization can erase asset-lock conflict evidence before it is seeded, and final conflict reconciliation is skipped when the checker regenerates a conflicting raw record. Both can leave incorrect asset-lock wait behavior or wallet balance/spend state after restore.
🔴 2 blocking
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); 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:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate cross-language change that directly alters wallet transaction-history replay, spent-output reconstruction, and spend reservations in persistence and loading paths, affecting funds movement and coin-selection state across SQLite, FFI, Swift, and Rust. - 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 below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— 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/manager/history_replay.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/history_replay.rs:168-170: Preserve asset-lock conflict evidence before load-time finalization
The replay finalizes the restored chain lock after rebuilding history, but `load_from_persistor` seeds `observed_input_conflicts` only afterward. With the default `keep-finalized-transactions = OFF`, `apply_chain_lock` evicts a confirmed spender record once its block is buried, leaving only its finalized txid. The subsequent seeder reads `transaction_history()` and the cache, so a restored confirmed spender that was needed to identify an `AssetLockInputContested` conflict disappears before it can be recorded. This regresses the bounded proof-wait behavior for a Built/Broadcast asset lock whose input was spent by a restored transaction. Capture conflict evidence before applying the restored chain lock, or seed the cache from the replayed records before finalization while retaining the finalization needed to rebuild spend guards.
- [BLOCKING] packages/rs-platform-wallet/src/manager/history_replay.rs:162-166: Reconcile final conflicts after raw-history restoration
`sweep_conflicts` is invoked only when `restored_fallback` becomes true. If a final winner is replayed before a conflicting mempool loser, all raw records have already been detached, so the winner's initial sweep cannot see the loser. The checker can then regenerate the loser because its wallet-owned change output is attributable, causing metadata restoration to take the existing-record branch at lines 152–155 and leaving `restored_fallback` false. For an InstantSend winner, the checker also does not populate `observed_spent_outpoints`, so the loser can recreate change and reserve additional inputs. Because the persisted projection filter preserves that change when it was present in the restored UTXO set, both conflicting state and an inflated balance can survive load. Reconcile all final transactions after the complete replay and metadata-restoration pass, independently of whether fallback records were reinserted; add a regression with an attributable loser and an InstantSend winner replayed first.
Out-of-scope follow-up suggestions (2)
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.
- Revalidate indefinitely retained unconfirmed spend reservations — Stored incoming or outgoing mempool records are replayed across restarts without expiry or chain revalidation. A never-mined transaction can therefore keep a wallet outpoint reserved indefinitely. This behavior predates the shared replay and is explicitly tracked by
TODO(expire-unconfirmed-spend-reservations), so it should not be fixed in this PR.- Follow-up: Track a separate change for safe admission and reservation revalidation that does not release inputs belonging to genuinely broadcast transactions merely because peers become silent.
- Bound startup history replay without discarding spend guards — A peer can accumulate many wallet-relevant unconfirmed records, increasing retained history and startup replay work on every launch. The SQLite path already replayed complete history, and this PR moves that behavior into the shared implementation; truncating history here would reopen spent-output resurrection. This is explicitly tracked by
TODO(bound-load-history-replay)and is outside this fix.- Follow-up: Design a separate bounded-history mechanism backed by durable spend-guard or finality checkpoints, with adversarial history-size coverage.
| // Finalize replayed records before a sync checkpoint can prune their spend guards. | ||
| if let Some(chain_lock) = wallet_info.metadata.last_applied_chain_lock.clone() { | ||
| wallet_info.apply_chain_lock(chain_lock); |
There was a problem hiding this comment.
🔴 Blocking: Preserve asset-lock conflict evidence before load-time finalization
The replay finalizes the restored chain lock after rebuilding history, but load_from_persistor seeds observed_input_conflicts only afterward. With the default keep-finalized-transactions = OFF, apply_chain_lock evicts a confirmed spender record once its block is buried, leaving only its finalized txid. The subsequent seeder reads transaction_history() and the cache, so a restored confirmed spender that was needed to identify an AssetLockInputContested conflict disappears before it can be recorded. This regresses the bounded proof-wait behavior for a Built/Broadcast asset lock whose input was spent by a restored transaction. Capture conflict evidence before applying the restored chain lock, or seed the cache from the replayed records before finalization while retaining the finalization needed to rebuild spend guards.
source: gpt-6.1-sol (phase2-reviewer: general)
| if restored_fallback { | ||
| // Unattributable raw records were absent from the checker's conflict sweeps. | ||
| for (transaction, context) in final_transactions { | ||
| wallet_info.sweep_conflicts(&transaction, &context); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Reconcile final conflicts after raw-history restoration
sweep_conflicts is invoked only when restored_fallback becomes true. If a final winner is replayed before a conflicting mempool loser, all raw records have already been detached, so the winner's initial sweep cannot see the loser. The checker can then regenerate the loser because its wallet-owned change output is attributable, causing metadata restoration to take the existing-record branch at lines 152–155 and leaving restored_fallback false. For an InstantSend winner, the checker also does not populate observed_spent_outpoints, so the loser can recreate change and reserve additional inputs. Because the persisted projection filter preserves that change when it was present in the restored UTXO set, both conflicting state and an inflated balance can survive load. Reconcile all final transactions after the complete replay and metadata-restoration pass, independently of whether fallback records were reinserted; add a regression with an attributable loser and an InstantSend winner replayed first.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
Keep this branch's removal of the SQLite-only replay from rehydrate.rs and port the two restart-replay fixes that landed there into the shared history_replay, which runs for every persister at load: - Height-only funding: StoredTransaction gains `owned_inputs`, filled from TransactionRecord::input_details. Before replay, every owned input whose funding no replayed record credits is staged, so the spender rebuilds its account spent mark; the retention pass drops leftovers. The FFI decode supplies none yet (TODO(ffi-recorded-input-details)), so the guard is not rebuilt on the SwiftData path until the host sends per-input ownership. - InstantSend sibling order: records replayed as InstantSend re-run their conflict sweep after the full replay, and the swept txids join the set that keeps held raw records from returning as fallbacks. Staging makes more losers attributable, so a sweep can remove a parent before the raw-record fallback restore, leaving its unattributable child unreachable by the final sweeps. Held unconfirmed, unlocked descendants of swept transactions are now dropped transitively, matching the checker's own descendant rule. The storage integration tests merged from fix/pr-5126 await the async replaying load helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification confirms two prior blockers remain: block-confirmed conflicts can retain persisted losing change, and load-time finalization can erase asset-lock conflict evidence before recovery caches it. The new descendant cleanup also fails to remove already-reconstructed descendants and introduces avoidable quadratic startup work; four prior findings are fixed. No builds or tests were run; the supplied CI snapshot confirms Kotlin validation but contains no Rust or native Swift validation results.
🔴 3 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); 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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate cross-persister replay in packages/rs-platform-wallet/src/manager/history_replay.rs::replay_recorded_history changes spend reservations, conflict resolution, and spendable UTXO reconstruction, directly affecting funds availability and coin selection after wallet reload. - 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 below 15% reserve: weekly 39% left, 5h 11% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (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/manager/history_replay.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/history_replay.rs:157-163: Remove reconstructed descendants when their parent was already swept
drop_swept_descendants only extends the swept set, and this continue only skips restoration of the held original. Neither operation removes a live descendant already reconstructed by the checker. The permitted replay order loser → InstantSend winner → mempool child exposes this: the winner removes the parent while the child is detached, then a child with a wallet-owned output is reconstructed and its persisted output survives retention. The post-replay InstantSend sweep cannot discover that child because the conflicting parent record is already absent; the pinned conflict walker returns when it finds no direct loser. This helper identifies the child through held history, but skipping its original leaves the live record and selectable output intact. Remove reconstructed records and UTXOs for the swept descendant closure, with appropriate spend-mark cleanup, or prevent those descendants from being applied. The existing orphan regression uses an unattributable child and does not exercise this case.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/history_replay.rs:202-226: Index held descendants instead of repeatedly scanning them
This closure repeatedly scans every followable held record until no new txid is added. Held records are collected in account/transaction-map order rather than dependency order, so a deep descendant chain can require one pass per generation, producing O(depth × held history) startup work. The pinned checker's conflict walker already avoids this scaling problem with a parent-to-children index. Build that index once and traverse newly swept txids with a worklist, preserving the existing confirmed/locked exclusions while making traversal linear in records and input edges.
- [BLOCKING] packages/rs-platform-wallet/src/manager/history_replay.rs:175-179: Reconcile final conflicts after raw-history restoration
(existing thread: https://github.com/dashpay/platform/pull/5220#discussion_r4153965344)
The final reconciliation runs only when an unattributable held record was reinserted. A block-confirmed winner replays before its conflicting mempool loser while the held loser is detached, so the initial sweep cannot remove it. If the loser pays a wallet-owned address, the checker subsequently reconstructs its record. The pinned checker's observed-spend guard returns before modifying UTXOs, which prevents new credits but leaves the loser's already-restored change untouched. That change passes the projection retention filter, and restoring the held record takes the metadata-only branch, leaving restored_fallback false. The unconditional sweep at lines 122–123 covers only InstantSend winners, so neither the reconstructed loser nor its selectable change is removed. Reconcile block-confirmed winners after complete replay and raw restoration independently of fallback insertion, and cover an attributable loser whose change is present in the restored UTXO projection.
- [BLOCKING] packages/rs-platform-wallet/src/manager/history_replay.rs:181-184: Preserve asset-lock conflict evidence before load-time finalization
(existing thread: https://github.com/dashpay/platform/pull/5220#discussion_r4153965337)
Replay applies the restored chain lock before load_from_persistor constructs PlatformWalletInfo and calls seed_observed_input_conflicts at load.rs:283. With keep-finalized-transactions disabled, as it is by default, the pinned key-wallet implementation promotes and evicts covered InBlock spender records, retaining only their txids. Swift explicitly supplies confirmed spenders of unresolved asset-lock inputs through its unresolved-record buffer, but restoring those records before this finalization does not preserve their input-to-spender association afterward. The seeder reads only surviving transaction_history(), and the new conflict cache starts empty, so a Built/Broadcast asset lock loses its conflict-capped proof wait and contested-input verdict. Already-chainlocked history can also be evicted during checker replay when no held fallback preserves it. Capture the relevant input, spender, and height observations before either eviction point and carry them into PlatformWalletInfo while retaining core finalization. Cover manager load with a competing confirmed spender and a higher persisted chain lock under the default retention configuration.
| drop_swept_descendants(&mut swept, &held, &instant_locks); | ||
| let mut restored_fallback = false; | ||
| for mut account in wallet_info.accounts.all_accounts_mut() { | ||
| let owner = account.managed_account_type().to_account_type(); | ||
| for original in held.remove(&owner).into_iter().flatten() { | ||
| if swept.contains(&original.txid) { | ||
| continue; |
There was a problem hiding this comment.
🔴 Blocking: Remove reconstructed descendants when their parent was already swept
drop_swept_descendants only extends the swept set, and this continue only skips restoration of the held original. Neither operation removes a live descendant already reconstructed by the checker. The permitted replay order loser → InstantSend winner → mempool child exposes this: the winner removes the parent while the child is detached, then a child with a wallet-owned output is reconstructed and its persisted output survives retention. The post-replay InstantSend sweep cannot discover that child because the conflicting parent record is already absent; the pinned conflict walker returns when it finds no direct loser. This helper identifies the child through held history, but skipping its original leaves the live record and selectable output intact. Remove reconstructed records and UTXOs for the swept descendant closure, with appropriate spend-mark cleanup, or prevent those descendants from being applied. The existing orphan regression uses an unattributable child and does not exercise this case.
source: gpt-6.1-sol (phase2-reviewer: general)
| let followable: Vec<&TransactionRecord> = held | ||
| .values() | ||
| .flatten() | ||
| .filter(|record| { | ||
| matches!(record.context, TransactionContext::Mempool) | ||
| && !instant_locks.contains_key(&record.txid) | ||
| }) | ||
| .collect(); | ||
| loop { | ||
| let before = swept.len(); | ||
| for record in &followable { | ||
| if !swept.contains(&record.txid) | ||
| && record | ||
| .transaction | ||
| .input | ||
| .iter() | ||
| .any(|input| swept.contains(&input.previous_output.txid)) | ||
| { | ||
| swept.insert(record.txid); | ||
| } | ||
| } | ||
| if swept.len() == before { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Index held descendants instead of repeatedly scanning them
This closure repeatedly scans every followable held record until no new txid is added. Held records are collected in account/transaction-map order rather than dependency order, so a deep descendant chain can require one pass per generation, producing O(depth × held history) startup work. The pinned checker's conflict walker already avoids this scaling problem with a parent-to-children index. Build that index once and traverse newly swept txids with a worklist, preserving the existing confirmed/locked exclusions while making traversal linear in records and input edges.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
This branch has not been deployed
2 participants
Issue being fixed or feature implemented
Stacked on #5150 (
fix/pr-5126). Addresses the #5150 review thread "Confirmed-history replay is implemented only for SQLite".After a restart the wallet must know which outputs its confirmed transactions already spent (
spent_outpoints/observed_spent_outpoints). Without that, a redelivered funding transaction (rescan, gap-limit rediscovery) credits an already-spent output again. #5150 rebuilt this state only inside the SQLite persister. The FFI persister (iOS / SwiftData) replayed only unconfirmed outgoing sends, so confirmed spends were never replayed on that path.What was done?
platform-wallet-storage(sqlite/rehydrate.rs) intors-platform-wallet(manager/history_replay.rs).load_from_persistorruns it for every persister before the balance is mirrored into the wallet. Persisters only supply stored records through the newClientWalletStartState::recorded_history(RecordedHistory { transactions, instant_locks }, types inchangeset/recorded_history.rs).#[repr(C)] RecordedTransactionRestoreFFI, appended toWalletRestoreEntryFFIasrecorded_transactions/recorded_transactions_count.loadWalletListfetches the stored history once and hands each restorable wallet its own records. The history fetch prefetches the relationships used to determine wallet ownership. A fetch error rejects the snapshot rather than claiming the wallet has no history.CORE_HISTORY_RESTORE(bit 13). SQLite and Swift declare it; the FFI attests it only when the wallet-list load callbacks are wired. It gates nothing:load_from_persistorlogs a warning when a persister restores wallets without it.load_from_persistorand awaits the checker, so the first-poll shim (poll_ready) and its rollback were removed.net_amount/direction. They are unused here, reserved for the follow-up that makes Rust the single source of transaction accounting (replacing the Swift re-derivation).fix/pr-5126brought in its two restart-replay fixes. Both are ported into the sharedhistory_replay:StoredTransaction::owned_inputs) are staged before replay. A spend whose funding survives only as a height row therefore rebuilds its spent mark.Known limitations:
TODO(android-core-history-restore): the Kotlin host does not supply history yet. Its JNI fields are null/0, and Android does not attest the capability.TODO(bound-load-history-replay)andTODO(expire-unconfirmed-spend-reservations)moved with the replay. The latter now also covers stored incoming mempool records on the FFI path.TODO(ffi-recorded-input-details): the FFI path supplies no owned inputs yet. On SwiftData, the spent-mark guard for height-only funding is rebuilt only once the host provides per-input ownership.How Has This Been Tested?
fix/pr-5126merge,nextestfor-p platform-wallet -p platform-wallet-storage -p platform-wallet-ffipassed 2660 tests, and Clippy--all-targets --all-features -D warningsis clean for those crates. The ported fixes are covered byshould_sweep_conflicting_spend_for_either_sibling_replay_order,should_rebuild_spent_mark_for_height_only_funding_from_owned_inputs,should_drop_orphaned_raw_descendant_of_swept_conflict_in_either_order, and the two height-only-funding storage tests. Each fails with its fix disabled.platform-walletandplatform-wallet-ffiwith--all-targets --all-features --locked -- --no-deps -D warnings.swiftnorxcodebuild.Breaking Changes
WalletRestoreEntryFFIgains two trailing fields (recorded_transactions,recorded_transactions_count), plus the new element structRecordedTransactionRestoreFFI. The entry has no size field, so the host and the Rust library must be built together (same contract asunconfirmed_outgoing_tx_records).ClientWalletStartStategains the public fieldrecorded_history. Struct-literal constructors must set it;Default::default()gives the previous behaviour.SqlitePersister::load()no longer returns a replayed projection. Callers that bypassPlatformWalletManager::load_from_persistormust callplatform_wallet::manager::history_replay::replay_recorded_historythemselves.CORE_HISTORY_RESTORE(bit 13) is additive.This PR conflicts with #5210 in
sqlite/rehydrate.rs(the replay region moves out) and slightly inpersister.rs::load_one_wallet. Resolve by keeping the move and porting #5210's replay changes intomanager/history_replay.rs.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent