fix(platform-wallet): stop a contact's watch-only chain from defining the persisted transaction row - #4363
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe wallet bridge now excludes contact watch-only records from persisted transactions and newly derived UTXOs. It retains usage, address, spend, and cleanup updates. Regression tests cover transaction and block-processing scenarios. ChangesContact watch-only filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🕓 Ready for review — next in queue (commit 6b802c9) |
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>
… 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.
…y-wallet rust-dashcore#952 (merged as 9c0e8742) gave the #926 policy a canonical home: AccountType::is_contact_owned(), with an exhaustive match so any future account type must declare whether its coins are the wallet's or a contact's. Bump the workspace pin to the dev tip (37b1a361, which also brings dash-spv sync-reliability fixes #941/#943/#949/#953) and make is_contact_watch_only delegate to the upstream predicate instead of matching DashpayExternalAccount locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
98f171e to
6b802c9
Compare
|
Updated now that dashpay/rust-dashcore#952 merged:
Verified: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4363 +/- ##
=========================================
Coverage 87.63% 87.63%
=========================================
Files 2670 2670
Lines 339447 339447
=========================================
+ Hits 297464 297465 +1
+ Misses 41983 41982 -1
🚀 New features to boost your workflow:
|
3 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