Conversation
Build on rust-dashcore PR #979 and account-local correction/event fixes at ed4c02e119898f1bb510cf79abc8a125a9bc9bea. Cargo metadata --locked validates the pin; wallet integration checks follow with the storage changes.
Repair fully resolved history from durable TXOs at round commit and wallet load, preserve funded asset-lock accounting during context-only recovery, and scope history presentation to the selected wallet. Swift tests require macOS tooling and were not executable on this host. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Use one ownership predicate for persisted inputs, outputs, and scoped history amounts, including legacy watch-only TXOs. Preserve accountless rows with known wallet ownership. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Index historical inputs to repair late funding and existing SQLite history, preserve known ownership across partial replay, and project corrected bridge topology without double-counting account snapshots. Keep unknown credit verdicts from inventing spends or resurrecting coins. Co-Authored-By: Codex GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Replace the PR 979-based dependency with the isolated accounting correction backported onto the original e4208c90 base. The identical upstream patch is available on current rust-dashcore dev without SPV or address-pool changes. Validated 24 Core storage and 132 changeset tests with the new pin; scoped Clippy with CI flags and formatting checks pass. Co-Authored-By: Codex <noreply@openai.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 — 1 blocking finding(s) (commit d82b65e) · triage: critical |
…sync Replay persisted confirmed transactions through the wallet checker on load, retain only persisted unspent outputs not consumed by confirmed history, and restore finality before sync checkpoint pruning. Use dash-async for synchronous persistence loading. Cover restart, stale projections, funding redelivery, finality, same-block spends, reserved inputs, and synchronous/current-thread/multi-thread callers. Co-authored-by: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…count The load projection parks an unattributable UTXO in the first funds account; replaying confirmed history then credits the same outpoint to its real owner. The post-replay filter only checked a wallet-wide unspent set, so both copies survived and update_balance double counted. Drop the load-time fallback copy whenever replay credits the outpoint to a different funds account, while still refusing to re-credit anything persistence excluded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dable Pre-fix builds persisted DashPay contact (watch-only) outputs as unspent core_utxos rows. V019 re-labels their history as Sent but left the rows unspent, so load routed them to the first funds account: the balance included coins the wallet cannot sign and coin selection could pick one. load_state now skips unspent contact-only rows. The rows are kept in SQLite (no destructive migration step), so the exclusion is reversible. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eset loadWalletList saved or rolled back the background context unconditionally after reconciling transaction accounting. Inside an open begin/endChangeset round that committed half of the round or silently discarded its staged writes while the round still reported success. Guard the pass with !inChangeset like every other load-time writer; the round reconciles its own dirty rows and the next load runs the full pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Phase 1 blocker gate
The SQLite repair logic addresses the stated restart and historical-accounting failures, but the confirmed-spend replay is implemented only in the SQLite persister while the FFI/SwiftData restore path does not rebuild the corresponding spend guards. Several additional correctness and scalability issues remain in the new history-repair and Swift reconciliation code.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 1 blocking | 🟡 7 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: History replay holds the SQLite connection lock for the full confirmed history
packages/rs-platform-wallet-storage/src/sqlite/persister.rs
load_one_wallet invokes restore_confirmed_transactions while load holds the connection mutex. The replay performs an async block_on round trip and runs check_core_transaction once per confirmed record, even though the replay itself does not access SQLite. Large histories therefore block concurrent store and flush operations for the duration of the replay. Snapshot the required records and release the connection lock before replaying, or document and bound this startup write-stall behavior.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, rust-quality)
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: rust-quality); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff combines intricate persisted-accounting repair and a new SQLite migration with changes to restore_confirmed_transactions in packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs that directly determine spendable UTXOs through transaction replay, spent-output exclusion, account deduplication, and finality 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— 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) - Fresh verifier:
gpt-6-astra— verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:423-505: Confirmed-history replay is implemented only for SQLite
The restart invariant is that every persister must hand WalletManager a spend projection containing confirmed spent outpoints. This change enforces that invariant only in SQLite by replaying persisted records through `check_core_transaction` inside `restore_confirmed_transactions`. The FFI restore path still reconstructs spendability from persisted TXO rows, and the Swift reconciliation pass only rewrites `netAmount` and `direction`; it does not rebuild Rust's `spent_outpoints` or remove confirmed inputs from the in-memory UTXO set. Consequently, a wallet restored through the FFI/SwiftData path can still rehydrate a confirmed-spent output as selectable, while SQLite behaves differently. Move the confirmed replay to the shared async `load_from_persistor` boundary, or provide equivalent replay semantics for every persister before the manager consumes the start state.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:6811-6863: SwiftData independently re-derives Rust-owned accounting
The SwiftData repair path recomputes ownership, amounts, and direction from decoded prevouts, while SQLite's `repair_record` derives the same values from persisted UTXO state and the Rust wallet projection. These implementations already have materially different rules: Swift requires every input prevout to resolve, passes `allOutputsOwned = false` to its helper, and re-encodes contact exclusion using a raw account-type tag, whereas the Rust repair can recover ownership from the UTXO table and distinguishes internal transactions from external outputs. Future checker or contact-ownership changes can therefore make the two persistence backends repair the same transaction differently. Centralize this accounting projection in Rust and have Swift persist the returned verdict rather than reimplementing it.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:6915-6934: Load-time repair scans the entire store and fails all wallets on one bad row
`loadWalletList` fetches every `PersistentTransaction` and every `PersistentTxo`, then filters transactions in memory for the wallets being loaded. This makes startup work proportional to the entire SwiftData store even when loading one wallet. The same `do` block also treats any reconciliation or fetch error as a failure for the complete wallet list, so one unreadable transaction or TXO hides otherwise healthy wallets. Scope both fetches to the loaded wallet IDs, or batch per wallet and isolate per-wallet reconciliation failures.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:32-36: Same-txid redelivery with different witness bytes aborts persistence
`preserve_known_details` rejects a persisted transaction whenever `previous.transaction != incoming.transaction`. A transaction's txid excludes witness data, so legitimate observations of the same txid can differ only in witness bytes. That causes the entire changeset to fail even though the row is already keyed by txid and the ownership details below could be merged. Accept same-txid redeliveries whose non-witness transaction identity is unchanged, or compare an appropriate txid-level identity rather than full transaction equality. If a mismatch must remain fatal, include the txid in the typed error so the failure is diagnosable.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:150-157: Contact filtering hardcodes a database label
`contact_only_script` embeds `account_type != 'dashpay_external'` directly in SQL, while the canonical mapping is `accounts::account_type_db_label`. If the persisted label changes, the writer and this repair query can silently diverge and contact-only outputs can be treated as wallet-owned during repair. Use a shared constant or construct the query with the canonical label and add a test tying the SQL value to `account_type_db_label`.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_history.rs:292-313: Migration performs redundant decoding and wallet lookups per record
During migration, each row is decoded by `get_tx_record`, then `repair_record` calls `get_tx_record` again for the same txid. The loop also calls `network(tx, &wallet_id)` once per transaction, which repeats the wallets-table lookup for every record. Because this runs inside the migration transaction, large histories hold the migration lock while doing twice the decoding and redundant queries. Cache the network per wallet and pass the already-decoded record into the repair function.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:628-643: Account coalescing mixes finality from one observation with body data from another
When a later record has lower context rank, the code keeps the later record's amounts, inputs, and outputs but replaces only its context with the earlier record's more-confirmed context. That produces a synthetic record combining the stale observation's body with finality it did not carry. If corrected-then-stale ordering is reachable, the higher-ranked observation should win wholesale; if it is impossible, this branch is dead and should be removed. Add a regression test for corrected-then-stale ordering and replace the record wholesale based on context rank.
In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs: History replay holds the SQLite connection lock for the full confirmed history
`load_one_wallet` invokes `restore_confirmed_transactions` while `load` holds the connection mutex. The replay performs an async `block_on` round trip and runs `check_core_transaction` once per confirmed record, even though the replay itself does not access SQLite. Large histories therefore block concurrent store and flush operations for the duration of the replay. Snapshot the required records and release the connection lock before replaying, or document and bound this startup write-stall behavior.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…PV15 #5057 folded DRIVE_ABCI_QUERY_VERSIONS_V3 into V2 (identical values) and deleted v3.rs; PV14 was updated but PV15, introduced on v4.3-dev, still imported V3, so the v4.2-dev -> v4.3-dev merge stopped compiling. Pure rename; the selected query versions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v4.2-dev -> v4.3-dev merge (704307e) left an unused import and stray blank lines in dpns.rs, failing clippy -D warnings and rustfmt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The structure generator describes PlatformVersion::latest(), which is 15 once PLATFORM_V15 compiles. The committed JSON and a hard-coded "@14" assertion were stale. Regenerated the JSON (version labels only) and derive the expected fixture origin from PlatformVersion::latest(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌳 GroveDB structure No node of the described GroveDB structure was added, changed or removed. Open the structure viewer on this pull request's version. Compared |
Pin `direction(for:)` and `netAmount(for:)` for an A→B transfer between two local wallets, and when "Amount unavailable" is expected for rows without linked TXOs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
… a V019 rescan V019's corruption recovery subtracted one from the raw stored `birth_height`, so `i64::MIN` panicked with subtract-overflow inside `migrations::run`. Gate it through `i64_to_u32` (typed IntegerOverflow, the whole upgrade rolls back) and use `saturating_sub(1)`. Out-of-range heights are no longer silently clamped to a rescan from genesis. The runtime repair in `schema::core_history` never reads `birth_height`, so it has no equivalent to fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
Upstream key-wallet classifies each account on its own. For an asset
lock with no change output, the funding account says `Outgoing`, because
the OP_RETURN burn is not an owned output. The keys account says
`Internal`. The live fold kept the funding verdict, while the SQLite
repair, the frozen V019 migration and Swift all write `Internal`. As a
result, the same row flipped whenever storage repaired it.
The fold also summed the keys account's `+credit` marker net into the
wallet net. Live asset-lock rows therefore read `-fee`, while every
repair path wrote `-(credit + fee)`, with or without change (ARCH-002).
A test reproduced this before the fix (folded net -1000 = -fee).
Changes:
- Add one wallet-level accounting rule in
`changeset::wallet_accounting`:
- `wallet_direction` (public) is the rule the repair already used.
- `apply_wallet_accounting` sets net to owned outputs minus owned
inputs, and sets direction via `wallet_direction`.
- `fold_same_txid_records` now applies that rule to every record,
folded or single, after the per-txid merge. A funding slice that never
meets its keys marker is normalised too.
- Records with no details (keys markers only) keep upstream's net and
direction, mirroring repair's "empty metadata is not evidence".
- The storage runtime repair calls `platform_wallet::changeset::wallet_direction`
instead of its own copy. The Swift-shared case table stays in storage
under the same name.
- The frozen V019 logic is untouched.
- `test_support::fold_wallet_records` exposes the live fold, so storage
can pin repair parity against it.
No existing test pinned the old live behaviour, so none needed updating.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
…ansactions The wallet-level accounting pass runs on every projected record, including singles, so this regression table shows that the live projection's net and direction are unchanged for every other transaction shape. The table covers 14 cases: - plain receive - send with and without change - self-transfer within one account and between two accounts - CoinJoin - provider registration, with an own and with an external collateral - asset unlock - coinbase - payment to a contact, with the watch-only slice present - payment from a contact into DashPay receiving funds - a single keys marker - a keys-only group The expected values were derived by running the table against the pre-pass fold (a70ee60); all 14 pass there. One case changes on purpose. A group made only of keys-account markers, with no details, used to re-derive `Incoming` from its empty details. It now keeps upstream's `Internal`, which matches a single marker and the SQLite repair (that repair skips detail-less records). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
The Rust repair's local `repaired_direction` was replaced by the shared `platform_wallet::changeset::wallet_direction`, which both the live projection and the SQLite repair use. The comment is the only change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Preliminary review — Phase 1 blocker gate
The SQLite replay correctly restores confirmed spend guards, but the same replay is absent from the FFI/SwiftData restore path. Because this PR adds backend-specific replay behavior while its stated goal is restoring Core spending state, the divergence can resurrect confirmed-spent outputs for FFI-restored wallets and remains blocking. Other carried concerns are explicitly tracked as deferred follow-ups in the PR and are not actionable in this change.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); 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); final verifier: gpt-6.1-sol (agent: sol-gate-verifier, role: verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that modifies wallet funds accounting and coin-selection state across Rust and Swift implementations, introduces a SQLite storage migration, and changes persistence/rehydration logic in files such as core_history.rs, core_state.rs, and legacy_v019.rs. - 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— platform-versioning (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— verifier; agentsol-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:433-482: Confirmed-history replay is implemented only for SQLite
(existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4137483872)
This PR calls `restore_recorded_transactions` only from the SQLite `load_one_wallet` path. The FFI restore ABI still carries only unconfirmed outgoing records, and no shared load boundary replays confirmed `TransactionRecord`s before the wallet manager consumes the restored state. As a result, redelivered funding data can remain selectable after a confirmed spend on FFI/SwiftData restores, while SQLite suppresses it through replay. Move the confirmed-history replay to the shared wallet load boundary, or provide equivalent replay inputs and behavior for every persister before restored state is consumed.
|
@thepastaclaw Re: the blocker "Confirmed-history replay is implemented only for SQLite". This is handled in #5220, stacked on this PR:
It is split out on purpose: it changes the FFI ABI and the Swift load path, and reviewing that separately keeps this PR focused on the storage fix. It is not a regression introduced here: before this PR, no persister replayed confirmed history. This PR fixes SQLite and leaves the FFI path unchanged, and #5220 closes the gap. #5220 is meant to land directly after this PR, and this PR is merge-gated on dashpay/rust-dashcore#1082 anyway. |
…r a known txid The body-conflict check compared whole `Transaction` values, including `TxIn::witness`, which the txid does not commit to. dashcore still decodes BIP144-style witnesses, so a peer could serve a known txid with an extra witness and the resulting non-transient conflict wiped the whole flush buffer. Compare the bodies' computed txids instead and keep the stored body, so a witness-only variant neither fails the flush nor replaces it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
…te repair The storage repair re-implemented the owned-output net, the has_ours and has_external scans and the Received|Change ownership predicate that the live fold already applies. Expose `wallet_accounting` (net as i128 plus direction) and `is_owned` from `platform_wallet::changeset` and call them from both paths; each keeps its own i64 overflow policy (live saturates, repair errors). The module doc now states what is shared and that the frozen V019 migration keeps its own copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
…ounting reconcile The reconcile pass read `account.wallet` directly in the network fallback, the unlinked-output owner check and the load-time filter. That relationship is fault-loaded, and the rest of the handler reads it through an Optional cast so that a store inconsistency cannot trap. The load filter runs before wallets restore, where a trap would get past the surrounding do/catch. Read it through the cast everywhere, and build the load filter from `walletOwnsTransaction` so there is a single ownership predicate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
… protocol 15 The v5.1-dev merge brought the team-action layers recorded at protocol 14. The base relabel to 15 did not cover them, so the structure check failed. The tree layout is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Five prior findings are fixed and five are intentionally deferred, but the incoming-payment blocker remains for transactions received by multiple local wallets. The shared Rust accounting helper also introduces quadratic output scanning. Ten focused Rust tests and all 22 existing Swift accounting tests passed; re-executing the additional shared-receipt regression reproduced both missing-amount failures.
🔴 1 blocking | 🟡 1 suggestion(s)
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: 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); 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 wallet funds movement and coin-selection state across Core accounting and Swift persistence, while also introducing a SQLite schema migration and repair logic in files such aspackages/rs-platform-wallet/src/changeset/core_bridge.rsandpackages/rs-platform-wallet-storage/migrations/V019__core_transaction_accounting.rs. - 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— 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/changeset/wallet_accounting.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/wallet_accounting.rs:96-101: Avoid quadratic output lookups in shared wallet accounting
For each non-OP_RETURN transaction output, this predicate restarts a linear search through `output_details`. A transaction with N owned outputs and matching details therefore requires approximately N(N+1)/2 detail comparisons. The previous SQLite repair used indexed `BTreeMap::get` lookups; the new shared helper performs this scan for both live projection and runtime SQLite repair, adding quadratic work to large self-splits and extending the repair transaction's connection-lock duration. Build an index of owned/unspendable output indices once, or merge the ordered output and detail sequences in linear time.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift:250-252: Pending external inputs hide ordinary incoming payment amounts
(existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4145159068)
The single-wallet shortcut fixes ordinary receipts recorded by one wallet, but the remaining guard still mistakes recording-wallet membership for input ownership. `upsertTransaction` passes every raw input to `resolveInputOutpoint`, which creates a pending row tagged with each recording wallet whenever the prevout is absent. An external payment with a 40-duff output for local wallet A and a 60-duff output for local wallet B therefore has foreign pending rows tagged with both wallets. Its participant set contains A and B, bypassing the shortcut, and this guard returns nil for both amounts indefinitely. I re-executed the SwiftData regression representing that persisted state: both incoming directions passed, but the amount assertions returned nil instead of 40 and 60. Preserve authoritative per-wallet accounting or distinguish unresolved owned inputs from foreign linkage misses, and cover this shared external receipt alongside the existing resolved A-to-B transfer test.
Out-of-scope follow-up suggestions (3)
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.
- Release unconfirmed and reorg-invalidated spend reservations — The replay still restores mempool observations with state updates enabled and no provenance or expiry check. Runtime repair also retains attributable spent claims without a corresponding reorg-release path. These are concrete wallet-availability concerns, but the PR explicitly separates them under
expire-unconfirmed-spend-reservationsandrelease-repair-spends-after-reorg; that deferral is not a technical mitigation.- Follow-up: Complete #5225 and the paired reorg-release work, clearing invalid reservations in both persisted and in-memory state while preserving valid confirmed and InstantSend-protected spends.
- Complete confirmed-history replay for non-SQLite persisters — Full recorded-history replay remains in SQLite's
load_one_wallet. The shared manager replays onlyunconfirmed_outgoing_txs, so FFI/SwiftData has not gained equivalent confirmed-spend reconstruction on this head. The base had the same non-SQLite limitation, and the PR explicitly leaves the ABI and shared-load expansion to stacked #5220.- Follow-up: Land the separately scoped #5220 work so every persister supplies recorded history for replay before restored spendability is exposed.
- Enforce the documented rust-dashcore dependency merge gate — The workspace dependency family remains pinned to
18f7f3e695e770ea5d2820aa85597d45160d1b8e. The PR identifies this as a compatibility-branch commit and explicitly prohibits merging until rust-dashcore #1082 lands and the pin moves to a merged commit. This is a publication gate rather than another local implementation defect.- Follow-up: Before publication, update Cargo.toml and Cargo.lock to the merged dependency commit and rerun the targeted wallet and storage validation.
The external-output check rescanned output_details for every transaction output, so large self-splits cost O(N^2) on both the live fold and the SQLite runtime repair. Collect the owned/unspendable output indices into a set first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
|
Thanks for clarifying the split with #5220. The latest completed review (#5150 (review), on 628efc3) now explicitly classifies non-SQLite confirmed-history replay as an out-of-scope follow-up, not a blocker for #5150. That supersedes the earlier blocking classification quoted here; it does not claim that the FFI gap is fixed in this PR. That completed review still identified a separate shared/multi-wallet incoming-amount blocker. The new head f78bccb, including your output-lookup change, is already under review in Reviewsys. No duplicate review or implementation is needed from this mention; the current-head verdict is still pending. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Two blocking restart-replay correctness defects remain: SQLite does not restore ownership evidence when a funding transaction is represented only by a height row, and replay order can credit outputs from a losing mempool transaction after an InstantSend winner. A shared-wallet Swift accounting edge case and an oversized-script migration read remain actionable suggestions; the supplied CI snapshot reports Rust workspace and Swift success, with unrelated Dashmate and Kotlin failures.
🔴 2 blocking | 🟡 2 suggestion(s)
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: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff directly changes storage migrations in sqlite/migrations/legacy_v019.rs and spendable-output restoration in sqlite/rehydrate.rs and sqlite/schema/core_state.rs, where errors could corrupt persisted accounting or make already-spent coins selectable. - 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 85% left, 5h 7% 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 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— 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
🤖 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-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:479-481: Restore owned-input evidence when the funding body is absent
`restore_recorded_transactions` replays only `record.transaction`, so it discards the stored `input_details` that may be the only surviving ownership evidence. `load_state` does not emit height-only funding rows as records and excludes spent TXOs from the restored UTXO set. Therefore, a confirmed spend that pays entirely externally can be classified without an owned input and fail to rebuild `spent_before_funded` or an equivalent durable spend guard. The temporary observed-spend entry is later removed by `apply_chain_lock`, after which redelivery of the funding transaction can recreate the consumed output as selectable; an unconfirmed spend has no such temporary guard. Restore spend evidence from persisted spent-TXO rows or the stored spender details before replay, and add a height-only-funding restart regression covering redelivery after finality pruning.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:461-481: InstantSend conflict replay can retain the losing transaction's spendable change
`replay_order` orders dependency records but does not settle conflicting siblings before replaying them. If the InstantSend winner is processed before the conflicting mempool loser, `check_core_transaction` cannot sweep the loser because that record has not yet been installed. The winner may be irrelevant and therefore contributes neither an observed block spend nor a conflict marker; the loser is then replayed and its wallet-owned change is credited. The final retention pass filters only `observed_spent_outpoints` and misplaced outputs, so the loser's persisted change remains selectable. Reconcile all conflicting records after the complete replay, or otherwise retain winner claims independently of replay order, and test both sibling orders with a persisted wallet-owned loser output.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift:248-252: Shared-wallet receipts remain unavailable when foreign inputs are pending
For a transaction paying two local wallets, `participatingWalletIds` contains both recording accounts, so the single-wallet fast path at line 249 is skipped. The FFI callback passes every raw input to each matched wallet, and `resolveInputOutpoint` creates a pending row tagged with that recording wallet whenever the sender's foreign prevout is absent. Both wallet-specific calls therefore hit the pending-input guard and return `nil`, even though Rust has already emitted a positive wallet-level amount for each wallet. The list and detail views consequently show “Amount unavailable” for valid shared-wallet receipts. Retain the Rust wallet-scoped accounting verdict through the persistence callback, or distinguish unresolved owned inputs from foreign pending links, and add a two-wallet receive regression.
In `packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs:168-181: Validate the stored script length before selecting its payload
`owned_output` selects `length(script)` and `script` in the same SQLite statement, then checks the length only after `rows.next()` has stepped the result. An oversized historical script can therefore be materialized by SQLite before `BlobTooLarge` is returned, defeating the migration's intended allocation guard; the runtime `core_history` reader has the same ordering. Use a length-only query first and fetch the script in a second query after the size check, with oversized-script coverage for both migration and runtime repair.
| let hasUnownedTxos = (inputs + outputs).contains { !PlatformWalletPersistenceHandler.isWalletOwnedTxo($0) } | ||
| if participatingWalletIds == [walletId], !hasUnownedTxos { return netAmount } | ||
| // Computed from TXOs alone: a pending input this wallet recorded may be | ||
| // one of its own still-unlinked coins, so the sum is only provisional. | ||
| guard !pendingInputs.contains(where: { $0.walletId == walletId }) else { return nil } |
There was a problem hiding this comment.
🟡 Suggestion: Shared-wallet receipts remain unavailable when foreign inputs are pending
For a transaction paying two local wallets, participatingWalletIds contains both recording accounts, so the single-wallet fast path at line 249 is skipped. The FFI callback passes every raw input to each matched wallet, and resolveInputOutpoint creates a pending row tagged with that recording wallet whenever the sender's foreign prevout is absent. Both wallet-specific calls therefore hit the pending-input guard and return nil, even though Rust has already emitted a positive wallet-level amount for each wallet. The list and detail views consequently show “Amount unavailable” for valid shared-wallet receipts. Retain the Rust wallet-scoped accounting verdict through the persistence callback, or distinguish unresolved owned inputs from foreign pending links, and add a two-wallet receive regression.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
There was a problem hiding this comment.
Confirmed, deferred to #5226. Swift cannot tell a foreign pending input from an unlinked own one: the FFI carries only input outpoints, no per-input ownership, and testShouldNotComputeSharedAmountWhileOwnInputsArePending deliberately keeps the amount unavailable rather than risk an understated one. The fix is to store Rust's wallet-scoped net amount and direction, which is exactly #5226's scope (Rust as the single source of accounting, removing this Swift re-derivation). Marked with TODO(wallet-scoped-accounting-from-rust) in d82b65e.
There was a problem hiding this comment.
Deferred (re-reviewed at d82b65ec): Your explanation matches the current implementation: shared-wallet rows still encounter wallet-tagged pending inputs without enough information to distinguish foreign prevouts from unresolved owned inputs. The #5226 TODO explicitly defers retaining Rust's wallet-scoped accounting; the unavailable receipt amount remains unresolved at this head.
…col' into fix/pr-5126
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR materially improves SQLite history repair and shared accounting, but restart replay still has two correctness blockers: InstantSend conflict resolution is order-dependent, and persisted owned-input evidence is discarded when funding history is incomplete. The shared-wallet Swift accessor and both script-size readers also retain narrower issues. This was a static review; the supplied CI snapshot does not establish Rust wallet or Swift test coverage.
🔴 2 blocking | 🟡 2 suggestion(s)
4 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: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: 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 diff introduces an intricate persisted-accounting migration in migrations/legacy_v019.rs and substantially changes SQLite rehydration in src/sqlite/rehydrate.rs, repairing spent-output state and determining which restored outputs remain selectable for spending. - 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 54% left, 5h 6% 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— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:461-481: InstantSend conflict replay can retain the losing transaction's spendable change
(existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4160993680)
Replay orders independent sibling records by txid, not by finality precedence. If the InstantSend winner sorts first, its conflict sweep runs before the mempool loser has been restored. The pinned checker rejects later mempool credits only for block-observed spends, not for InstantSend claims, so the loser can then recreate a wallet-owned change output. The final retention pass keeps any output present in the persisted unspent projection unless it is an observed spent input or misplaced, so that losing change can remain selectable. The existing regression deliberately makes the winner sort after the loser and therefore does not cover the reverse order. Resolve InstantSend losers and their descendants independently of replay order, or perform a final conflict reconciliation that removes outputs created by swept transactions; test both sibling orders with persisted wallet-owned loser change.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:479-481: Restore owned-input evidence when the funding body is absent
(existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4160993674)
The load path restores only unspent TXOs and passes only each stored transaction body and context to `check_core_transaction`; it discards the record's persisted `input_details`. When a confirmed outgoing record has known wallet-owned inputs but its funding transaction is absent or only represented by a height-only row, the checker cannot resolve that input and a no-change spender can be classified as irrelevant. Its durable account-local spend guard is therefore not rebuilt, and the temporary observed-spend guard can later be pruned by the restored chainlock/checkpoint. Redelivering the funding transaction can then make the already-spent output appear selectable. Carry persisted ownership evidence through restoration using a non-spendable history channel owned by the wallet checker, and cover an external-only confirmed spend with missing funding body plus later funding redelivery.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift:248-252: Shared-wallet receipts remain unavailable when foreign inputs are pending
(existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4160993685)
The sole-participant fast path now preserves ordinary incoming amounts, but the shared-wallet branch still returns `nil` whenever a pending row is tagged with the queried wallet. `resolveInputOutpoint` creates such a row for every missing prevout and tags it with the recording wallet, including foreign sender inputs. A payment to two local wallets can therefore create foreign pending rows for both wallets while each wallet's received output and Rust-derived amount are already known. Pending-row presence does not prove that the unresolved input belongs to the queried wallet. Preserve wallet-scoped Rust accounting or carry explicit input-ownership/completeness evidence across the persistence boundary instead of using the recording-wallet tag as the veto.
In `packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/migrations/legacy_v019.rs:168-181: Validate the stored script length before selecting its payload
(existing thread: https://github.com/dashpay/platform/pull/5150#discussion_r4160993691)
`owned_output` selects `script` in the same statement as `length(script)` and checks the length only after `rows.next()` returns. SQLite evaluates the result row before the Rust size check, so an oversized historical script can be materialized before the allocation guard rejects it. The migration's `read_record` helper already uses a separate length-only query for this reason, while `owned_output` and the equivalent runtime reader in `schema/core_history.rs` retain the unsafe query shape. Preflight the script length in a separate query, then fetch the payload only after it passes the bound.
Out-of-scope follow-up suggestions (3)
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.
- Complete confirmed-history replay across all persistence backends — This head implements recorded-history replay in the SQLite persister, while the shared manager and FFI/SwiftData load path remain unchanged. The PR explicitly separates the ABI-changing shared restoration work into stacked #5220, so this is a concrete follow-up rather than a regression introduced by the SQLite implementation.
- Follow-up: Land and independently validate #5220, including the FFI restart regression, before treating restored spending-state invariants as complete across persisters.
- Make Rust the sole SwiftData accounting authority — Swift still independently derives ownership, net amount, and direction even though Rust now owns the live and SQLite accounting rules. The PR explicitly defers removal of this duplicate authority to #5226 because it requires cross-wallet row-precedence and persistence-boundary decisions.
- Follow-up: Complete #5226 by persisting wallet-scoped Rust accounting and removing Swift's semantic re-derivation.
- Add lifecycle management for unconfirmed spend reservations — Replay intentionally restores persisted mempool observations without provenance validation or expiry, and the code marks this with
TODO(expire-unconfirmed-spend-reservations). This is a known security and correctness follow-up explicitly assigned to #5225, not a completed mitigation in this PR.- Follow-up: Implement #5225 with coordinated removal from persisted and in-memory state while preserving confirmed and InstantSend-locked claims.
…ding on load A spender whose funding output persists only as a height-only row replayed with no owned input: load excludes spent outputs, so the checker saw an irrelevant transaction and rebuilt no account spent mark. Once a chain lock pruned the observed spend (or for any unconfirmed spend), a redelivered funding transaction re-credited the consumed output as selectable. Before replay, stage every owned input named by a stored record's input_details whose funding no replayed record credits. Replaying the spender then removes it and records the spent mark; the retention pass drops anything left, since staged coins are never part of the persisted unspent set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
… of replay order Unconfirmed siblings replay by txid, so an InstantSend winner replayed before its conflicting mempool loser swept nothing: the loser was not installed yet, and on its own replay its wallet-owned change was credited and survived the retention pass as selectable. The opposite order swept it. Collect every record replayed in InstantSend context and run its conflict sweep once more after the full replay, so the outcome no longer depends on sibling order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
…reading it `owned_output` in the live history repair and the frozen V019 migration selected `length(script)` and `script` in one statement. SQLite loads every result column before returning the row, so an oversize script was materialised before `check_size` ran; past the connection length cap the read failed as a raw SQLite TooBig instead of the typed BlobTooLarge. Read the length in its own statement and fetch the script only once it passes the gate. V019 migrated data is unchanged (pinned by its existing repair test); only the error for an oversize script differs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
…nting A foreign payment to two local wallets shows "Amount unavailable" because Swift cannot tell a foreign pending input from an unlinked own one. Rust's wallet-scoped accounting (#5226) replaces this re-derivation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LN7YPS4eaKdQv5XFUBPLH
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
One restart-safety blocker remains: replay can retain persisted change from a mempool transaction that conflicts with a confirmed winner. The previous fixes are present, and the remaining documented deferrals have been distinguished from completed fixes across all 16 prior findings. This was a static review; the supplied exact-head CI snapshot has Kotlin build/tests pending, provides no Rust test results, and does not establish Swift execution.
🔴 1 blocking
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: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes critical storage migration and coin-selection state in sqlite/migrations/legacy_v019.rs and sqlite/rehydrate.rs, repairing persisted accounting and reconstructing spend marks and spendable outputs across confirmed, mempool, and InstantSend replay. - 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 40% left, 5h 19% left - 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— 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-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:484-486: Sweep confirmed winners after replaying their mempool competitors
The post-replay conflict sweep collects only InstantSend winners, leaving block-confirmed winners vulnerable to stale persisted loser outputs. A legacy database can contain a confirmed spend and a conflicting mempool record whose change remains unspent: the base SQLite writer unconditionally persisted new outputs as unspent, and V019 repairs accounting and spent inputs without removing conflicting outputs. Loading installs that change before replay. `replay_order` normally processes the confirmed winner first, so its initial sweep cannot see the competitor's record yet. When the competitor replays, the pinned key-wallet implementation inserts its record but returns from `update_utxos` through `doomed_by_a_settled_spend` before removing or modifying the already-restored change. The final retention predicate keeps that output because it belongs to `placed` and its own outpoint is not an observed spent input. The wallet therefore exposes nonexistent change to coin selection after upgrading and restarting. Include confirmed winners in post-replay conflict reconciliation using the upstream sweep's ChainLock/InstantSend precedence, and extend the conflicting-sibling regression with a block-confirmed winner and persisted unspent loser change.
| if matches!(context, TransactionContext::InstantSend(_)) { | ||
| instant_send_winners.push((record.transaction, context)); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Sweep confirmed winners after replaying their mempool competitors
The post-replay conflict sweep collects only InstantSend winners, leaving block-confirmed winners vulnerable to stale persisted loser outputs. A legacy database can contain a confirmed spend and a conflicting mempool record whose change remains unspent: the base SQLite writer unconditionally persisted new outputs as unspent, and V019 repairs accounting and spent inputs without removing conflicting outputs. Loading installs that change before replay. replay_order normally processes the confirmed winner first, so its initial sweep cannot see the competitor's record yet. When the competitor replays, the pinned key-wallet implementation inserts its record but returns from update_utxos through doomed_by_a_settled_spend before removing or modifying the already-restored change. The final retention predicate keeps that output because it belongs to placed and its own outpoint is not an observed spent input. The wallet therefore exposes nonexistent change to coin selection after upgrading and restarting. Include confirmed winners in post-replay conflict reconciliation using the upstream sweep's ChainLock/InstantSend precedence, and extend the conflicting-sibling regression with a block-confirmed winner and persisted unspent loser change.
source: gpt-6.1-sol (phase2-reviewer: general)
TL;DR: Repair Core transaction history that omits spent inputs and prevent already-spent outputs from becoming selectable after a SQLite wallet restart.
Issue being fixed or feature implemented
Fixes #5126.
As a wallet user, I want history to reflect what each transaction actually spent and received. When funding is discovered late or an asset lock is restored after restarting, history can overstate incoming funds or replace the debit with zero even though spent-output links are correct.
What was done?
dash-asyncdependency.input_detailsbefore replay.The pin
18f7f3e695e770ea5d2820aa85597d45160d1b8eis a compatibility backport of the initial accounting fix onto the original dependency base; it does not include #1082's subsequent finalized-retention follow-up. Current rust-dashcore dev has unrelated incompatible APIs. This avoids importing those changes or the broader work in rust-dashcore #979; it also leaves Platform #4777's snapshot persistence work separate.Base fixes
This PR is based on #5154 (
fix/structure-tests-latest-protocol), which regenerates the GroveDB structure snapshot for protocol version 15; without itv5.1-devfails the drive structure tests. Retarget tov5.1-devonce #5154 is merged. This PR carries no base fixes of its own.Review follow-ups (squash-merged from #5208)
InstantSend, but only when the persisted lock and the transaction's own txid both match the record.core_transaction_record_originals, recordsspent_in_txidfor every spent flag the repair sets, and keeps its repair logic inmigrations/legacy_v019.rs, with a V018→V019 pinning test. V019 checks the stored blob length with a separate query before it reads the payload.synced_heightto force a rescan. An undecodable unconfirmed record still fails the migration atomically.WalletStorageErroris#[non_exhaustive]and gainsTransactionBodyConflict,UnknownWalletNetworkandNetAmountOverflow.platform_wallet::changesetexposes the ownership predicate (is_owned), the direction rule (wallet_direction) and the net formula (wallet_accounting). The live fold and the SQLite runtime repair both call them and differ only on ani64overflow: live saturates, repair returnsNetAmountOverflow. The frozen V019 keeps its own copy. Asset locks areInternalwith net-(credit + fee)on both paths. Swift mirrors the rule through the same case table.netAmount(for:)and address-matched outputs counted only for funding wallets;CoreDirectionCodeand one direction style for the list and detail views;SCHEMA.mddocuments V019, its two tables and all writers ofspent_in_txid.Deferred (marked with
TODO(...)in code)expire-unconfirmed-spend-reservations: a never-mined (or forged) mempool spend keeps its input reserved across load and rescan with no expiry. Pairs with the reorg release below. Tracked in platform-wallet: unconfirmed spend reservations from unauthenticated mempool transactions never expire #5225.release-repair-spends-after-reorg: release rows whosespent_in_txidspender left the chain.bound-load-history-replay: replay only records above the last chain lock (performance).unify-record-coalescing: this PR addscoalesce_account_recordsnext to the existingcoalesce_newest_wins; the two have differentMergesemantics, and unifying them is deferred.test-round-accounting-failure: Swift fault-injection test for a round-end reconcile failure.persist-accounting-backfill-marker: the Swift full-history pass runs on every launch. Superseded by platform-wallet: make Rust the single source of Core transaction accounting (drop Swift re-derivation) #5226.wallet-scoped-accounting-from-rust: a foreign payment to two local wallets shows "Amount unavailable" in Swift, which cannot tell a foreign pending input from an unlinked own one. Fixed by platform-wallet: make Rust the single source of Core transaction accounting (drop Swift re-derivation) #5226.netAmount/directionand removes the Swift re-derivation.Related pull requests
fix/sqlite-asset-lock-reconciliation): lets SQLite asset-lock reconciliation run withoutWALLET_RESTORE.feat/sqlite-wallet-restore, on fix(wallet): require only atomic tracked-lock writes for reconciliation #5207): restores complete Core wallet snapshots from SQLite.fix/wallet-load-history-replay): replays recorded history on load for every persister, including the FFI/SwiftData path.Merge gate: the rust-dashcore pin
18f7f3elives only on rust-dashcore'sfix/5126-accounting-compatbranch. This PR will not be merged before dashpay/rust-dashcore#1082 is merged and the pin moves to a merged commit.Swift backfill conservatively retains authoritative accounting when previous-output ownership cannot be fully resolved. Core spending does not imply Platform asset-lock consumption.
How Has This Been Tested?
sqlite_spent_rehydration.rscover funding redelivery, stale unspent projections, finality pruning, persisted finality, same-block spends, unconfirmed reservations, synchronous/current-thread/multi-thread loading, and single-account ownership of replayed outputs. The follow-ups add replay tests for IS-lock restoration and pairing, dependency order and same-block order, plus V019 tests for corrupt records, oversize blobs and pinning. Together with the load reconstruction and error classification tests, these targeted suites passed. Final storage Clippy (--all-targets --all-features --locked -- --no-deps -D warnings), formatting and whitespace checks passed.cargo test -p platform-wallet-storage --all-features --locked --lib sqlite::schema::core_state::tests(24 passed), andcargo test -p platform-wallet --all-features --locked --lib changeset::(132 passed).--all-targets --all-features --locked -- --no-deps -D warnings, formatting and whitespace checks passed.should_keep_replayed_output_only_in_its_owning_account,should_not_load_contact_only_outputs_as_spendable,should_keep_contact_only_rows_but_exclude_them_after_history_migration, SwifttestShouldNotCommitOrDropOpenRoundWhenLoadRunsMidChangeset,should_keep_spent_output_excluded_after_finality_pruning_with_height_only_funding,should_keep_unconfirmed_spend_reservation_with_height_only_funding,should_sweep_conflicting_spend_for_either_sibling_replay_order, andshould_reject_oversize_owned_output_script_before_reading_itfor both readers); the last four groups failed before their fixes. The targeted storage suites passed again on this head, and storage Clippy--all-targets -D warningsis clean.2383a0aa2/ Platformc0425f7bdc/ rust-dashcore18f7f3e6: the standalone Core payment round trip passed; the full network-dependent backend E2E run finished with 65 passed and 10 failed (75 executed, 18 non-network tests filtered out). Seven failures require the unsetE2E_MN_PAYOUT_KEY; the other failures were DashPay identity funding (AssetLockInsufficientFunds), shielded withdrawal (balance mismatch), and asset-lock address funding (AssetLockAddressNotFound). Core payment round trip and cold-process wallet migration/balance recovery passed. NoWalletConfirmedInputConflictoccurred in this run. The suite is not green; the three other failures have not been root-caused.Breaking Changes
WalletStorageError(public,platform-wallet-storage) changes:#[non_exhaustive];TransactionBodyConflict,UnknownWalletNetworkandNetAmountOverflow.Downstream exhaustive matches, such as DET's, need a wildcard arm. No method signatures or FFI ABI change.
SQLite adds migration V019. It is unreleased but changed during review, so a development database that ran an earlier build of this branch fails refinery's divergent-migration check and must be recreated. No consensus-versioned behaviour changes.
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