fix(platform-wallet): stop a contact's watch-only chain from defining the persisted transaction row - #4363
fix(platform-wallet): stop a contact's watch-only chain from defining the persisted transaction row#4363bfoss765 wants to merge 1 commit into
Conversation
… the persisted transaction row dashpay/rust-dashcore#926 established that a `DashpayExternalAccount` is watch-only by construction — its addresses derive from a contact's xpub, so they are the contact's coins and this wallet only ever pays into them — and removed those accounts from `all_funding_accounts` so they stop counting toward balance and UTXO aggregation. The persistence seam is the same rule's second home, and it was missed. Upstream `check_core_transaction` emits one `TransactionRecord` per matched account, so a payment to a contact produces two records sharing one txid: the funding account's (`Outgoing`, `net = change - spent`) and the external account's (`Incoming`, `net = +paid`). `build_core_changeset` projected both into `CoreChangeSet.records`, and `derive_new_utxos` turned the contact's output into a wallet UTXO. Because the persisted `transactions` row is keyed by txid alone — the `transaction_account_involvements` table is only written for provider-key accounts, so there is no per-account dimension to disambiguate — the watch-only record defined the stored row. Field capture on testnet: a 0.69998912 DASH payment away was persisted as `direction=incoming`, `netAmount=+69998912` instead of `-70000000`, and the paid output sat in `txos` with `isSpent=0` indefinitely, inflating any SQL-sum balance. Records owned by an external account are now excluded from the persist-time projection: no transaction row, no new TXO. The funding account's record — already correct — becomes the row that lands. Everything genuinely ours from the same event is preserved: address-used flips and highest-used watermarks (so contact address rotation keeps working), derived-address rows, and `derive_spent_utxos`, which stays unfiltered so a contact spending an output persisted by a pre-fix build still clears the stale row. Eight regression tests cover the record pair a real contact payment produces, the standalone first-sighting path, the confirmation re-emit, a genuine receive, a DashPay *receival* account receive (the boundary #926 drew, which must stay incoming/positive), and an internal transfer. The four fix-dependent ones were verified to fail without the change.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
|
✅ Final review complete — no blockers (commit 98f171e) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The watch-only filtering fixes the transaction-row and new-UTXO classification, but the retained spent-UTXO cleanup is lost when the changeset crosses the FFI persistence boundary, so existing phantom TXOs do not self-heal on FFI-backed hosts. The tests also do not exercise the explicitly preserved contact address-pool deltas.
Source: codex-general reviewer backend gpt-5.6-sol; codex-rust-quality reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:686-692: Preserve filtered contact spends through the FFI projection
Filtering the watch-only record leaves its spent outpoints in `CoreChangeSet.spent_utxos`, which lets the native SQLite persister mark a stale pre-fix TXO spent. However, `WalletChangeSetFFI::from_changeset` explicitly ignores `CoreChangeSet.spent_utxos` and derives each account's `utxos_spent` only from the records retained in `cs.records` (`packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:249-255, 360-368`). After this filter, a contact-only spend has no retained record, so FFI-backed hosts receive no spent outpoint and leave the historical phantom TXO unspent even after the contact spends it. The standalone `TransactionDetected` filter at lines 626-629 has the same behavior. Add an account-routed spent delta that the FFI conversion consumes independently of persisted transaction records, and cover the complete FFI conversion path with the stale-TXO regression fixture.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:1396-1407: Exercise preserved contact address-pool deltas
This test uses an empty `WalletManager`, and its event carries no `addresses_derived`. Therefore `collect_usage_deltas` takes the unknown-wallet return and the resulting changeset contains none of the `addresses_marked_used`, `account_highest_used`, or `addresses_derived` state that the PR explicitly promises to preserve. The assertions only verify record and UTXO suppression; they do not establish that a real `DashpayExternalAccount` advances its contact address pool or that a watch-only-only event remains persistable after filtering. Build the test around a manager containing a real external account and monitored contact address, then assert the usage and derivation deltas survive while `records` and `new_utxos` remain empty.
| cs.records.extend( | ||
| inserted | ||
| .iter() | ||
| .chain(updated.iter()) | ||
| .chain(matured.iter()) | ||
| .filter(|r| !is_contact_watch_only(r)) | ||
| .cloned(), |
There was a problem hiding this comment.
🟡 Suggestion: Preserve filtered contact spends through the FFI projection
Filtering the watch-only record leaves its spent outpoints in CoreChangeSet.spent_utxos, which lets the native SQLite persister mark a stale pre-fix TXO spent. However, WalletChangeSetFFI::from_changeset explicitly ignores CoreChangeSet.spent_utxos and derives each account's utxos_spent only from the records retained in cs.records (packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:249-255, 360-368). After this filter, a contact-only spend has no retained record, so FFI-backed hosts receive no spent outpoint and leave the historical phantom TXO unspent even after the contact spends it. The standalone TransactionDetected filter at lines 626-629 has the same behavior. Add an account-routed spent delta that the FFI conversion consumes independently of persisted transaction records, and cover the complete FFI conversion path with the stale-TXO regression fixture.
source: ['codex']
| async fn contact_watch_only_detection_persists_no_transaction_row() { | ||
| let (_, _, watch_only) = contact_payment_records(); | ||
| let cs = build_core_changeset(&test_manager(), &transaction_detected(watch_only)).await; | ||
|
|
||
| assert!( | ||
| cs.records.is_empty(), | ||
| "a contact's watch-only chain must not define a wallet transaction row" | ||
| ); | ||
| assert!( | ||
| cs.new_utxos.is_empty(), | ||
| "the contact's output must not become a wallet UTXO" | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Exercise preserved contact address-pool deltas
This test uses an empty WalletManager, and its event carries no addresses_derived. Therefore collect_usage_deltas takes the unknown-wallet return and the resulting changeset contains none of the addresses_marked_used, account_highest_used, or addresses_derived state that the PR explicitly promises to preserve. The assertions only verify record and UTXO suppression; they do not establish that a real DashpayExternalAccount advances its contact address pool or that a watch-only-only event remains persistable after filtering. Build the test around a manager containing a real external account and monitored contact address, then assert the usage and derivation deltas survive while records and new_utxos remain empty.
source: ['codex']
…es (#952) A DashpayExternalAccount derives its addresses from the contact's xpub, so its coins are the contact's, never this wallet's. That policy now has two enforcement sites — balance/UTXO aggregation dropped the accounts from all_funding_accounts (#926), and dashpay/platform#4363 filters the same records out of its persistence projection — but each site hardcodes its own account-type list, which can silently drift when a new contact-owned account type is added. Give the policy one canonical home: AccountType::is_contact_owned(), with an exhaustive match so a new account type cannot compile without deciding whether its coins are the wallet's or a contact's, plus a delegating ManagedAccountType::is_contact_owned(). The #926 funding- scope test now asserts all_funding_accounts agrees with the predicate. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Supersedes #4353 — same change, recreated on a
dashpay/platformbranch per repo policy (no more personal-fork PRs). Commits and authorship unchanged; full review history on #4353.Context: this completes dashpay/rust-dashcore#926
dashpay/rust-dashcore#926 (@romchornyi, merged 2026-08-07) established the policy:
and applied it by dropping
dashpay_external_accountsfromManagedAccountCollection::all_funding_accounts/_mut, which coversbalance,account_balances,utxosandget_spendable_utxosin one place.dashpay_receival_accountswere deliberately kept — those derive from our xpub, so a contact paying into them really is money arriving.That fixed the balance layer. The persist-time projection is the same rule's second home, and it was missed. This PR applies the identical policy there. The
key-walletpin onv4.2-dev(b056d07c) already contains #926, so the two layers currently disagree with each other: the in-memory balance excludes the contact's coins, the persisted store still counts them.The defect
Upstream
check_core_transactionemits oneTransactionRecordper matched account (key_wallet::transaction_checking::wallet_checker). A payment to a contact matches two accounts, producing two records that share one txid:directionnet_amountOutgoingchange - spentDashpayExternalAccountIncoming+paidThe external account's record is not wrong about its own account — that chain did receive an output. It is wrong as a description of the wallet.
build_core_changesetprojected both records intoCoreChangeSet.records, andderive_new_utxosturned the contact's output into a wallet UTXO. The persistedtransactionsrow is keyed by txid alone — there is no per-account dimension to disambiguate it, becausetransaction_account_involvementsis only written for provider-key accounts (see follow-ups below). So whichever record is stored last defines the row, and the watch-only one is emitted last, sinceall_accountsvisits the DashPay accounts after the standard ones.Field-observed on a testnet device store (2026-08-09): every payment to a contact is persisted with
direction=incomingand a positivenet_amount— a 0.69998912 DASH payment away stored as+69998912where the wallet's true net is-70000000. The paid output sits intxoswithisSpent=0indefinitely (only the contact's own spend could ever flip it), so any SQL-sum consumer reads a phantom balance. A tester's mainnet wallet shows the same signature ("+3.83 change instead of −0.1 payment").What changed
packages/rs-platform-wallet/src/changeset/core_bridge.rs— one predicate plus the two projection sites that consume it:is_contact_watch_only(record)— matchesAccountType::DashpayExternalAccount { .. }, carrying the rationale and the feat(dashmate): replace js-drive-abci with rs-drive-abci #926 link so the line does not get "fixed" back.derive_new_utxosreturns nothing for such a record. Direct counterpart of feat(dashmate): replace js-drive-abci with rs-drive-abci #926 dropping those accounts fromutxos()/get_spendable_utxos().build_core_changesetomits them fromCoreChangeSet.recordson both paths: the per-recordTransactionDetectedfirst sighting, and theinserted/updated/maturedlists ofBlockProcessed. The confirmation path matters on its own — re-emitting the watch-only record when the block lands would re-clobber the row exactly as the first sighting did.The funding account's record — already
Outgoingwithnet = -(spent - change), exactly whatbalance()semantics imply — becomes the row that lands. No FFI or storage-schema change;CoreChangeSetand every persistence signature are untouched.What deliberately did not change
Everything from the same event that is genuinely ours to remember is preserved, so the event is never dropped wholesale:
addresses_marked_usedandaccount_highest_used— the contact's address pool must keep advancing or the wallet would pay the same contact address twice.is_empty_no_records()counts these, so a watch-only-only event still round-trips to the persister.addresses_derived— gap-limit extensions on the contact's chain still persist.derive_spent_utxosstays unfiltered — so a contact spending an output that a pre-fix build already persisted still clears that stale row. Covered by a test.all_accounts.One intended behavioural consequence: a third party paying our contact (which we see, because we monitor that chain) no longer produces a wallet
transactionsrow or TXO. Under #926's policy that is correct — those were never our coins — and it removes a second, quieter source of the same phantom balance.Required for correct Android behavior
The Android wallet currently compensates for these records at read time. That correction is a workaround for wrong data on disk, not a fix: every other consumer of the same store reads the rows raw — iOS parity, and any future feature that trusts
direction/net_amount/txos— and each would have to reinvent the same compensation. Fixing it at the point of persistence lets the Android read-time correction be retired.Testing
cargo test -p platform-wallet --lib— 618 passed, 0 failed. No existing test encoded the old projection.cargo test -p platform-wallet-ffi --lib— 261 passed, 0 failed.cargo clippy -p platform-wallet --all-targets -- -D warningsandcargo fmt --checkclean.Eight new tests in
contact_watch_only_projection_tests, built from the record pair a real contact payment produces:contact_directed_payment_persists_as_outgoing_and_negative— both records in oneBlockProcessed; asserts exactly one record reaches the persister,Outgoing,net == -70_000_000, and that only the change output becomes a TXO.contact_watch_only_detection_persists_no_transaction_row— the standaloneTransactionDetectedpath, where there is no sibling record in the batch to fall back on.funding_account_detection_of_the_same_payment_still_persists— the filter is scoped to the account, not the transaction.genuine_receive_still_persists_incoming_and_positive— unchanged: incoming, positive, TXO created.dashpay_receival_account_receive_is_unaffected— the boundary feat(dashmate): replace js-drive-abci with rs-drive-abci #926 drew; receival accounts derive from our xpub and must keep their incoming/positive row.internal_transfer_is_unaffected— all outputs owned, none type-13: direction, net, both TXOs and the spent input all unchanged.confirmation_re_emit_does_not_reintroduce_the_watch_only_row— theupdatedlist.contact_spend_still_clears_a_stale_pre_fix_txo— no transaction row, but the spent-TXO removal still fires.The four fix-dependent tests (1, 2, 7, 8) were verified to fail without the change — reverting
is_contact_watch_onlytofalsereproduces the exact defect (records.len()is 2 where 1 is allowed). Tests 3–6 are guards and pass either way by design.Notes / not in scope
Three adjacent items surfaced while tracing this, deliberately left for follow-up:
direction/net_amount, and their phantomtxosrows persist until the contact spends. Android's read-time correction covers its own surface; a one-time store-rewrite pass would be the general fix and is intentionally out of scope here.transaction_account_involvementsis effectively unpopulated. It is written only when the account is a provider-key account and the transaction is a provider special tx (PlatformWalletPersistenceHandler), so ordinary payments never get a row — which is precisely why the txid-keyedtransactionsrow has no per-account dimension and this collision was possible at all. Populating it generally would give per-account history a real join (theINNER JOIN transaction_account_involvementsquery inTransactionDaoreturns nothing for normal accounts today).txos.isInstantLockedhas no consumer. It is written from the TXO's first sighting and restored on load, but nothing reads it to make a decision — and becauseTransactionInstantLockedcarries no UTXO re-emit, a TXO first seen in mempool never has the flag flipped when its IS lock later arrives. Worth either wiring up or removing.Summary by CodeRabbit