Skip to content

fix(key-wallet): cover CoinJoin/DashPay accounts for AssetLock routing#867

Merged
xdustinface merged 2 commits into
dashpay:devfrom
bfoss765:fix/assetlock-router-account-coverage
Jul 13, 2026
Merged

fix(key-wallet): cover CoinJoin/DashPay accounts for AssetLock routing#867
xdustinface merged 2 commits into
dashpay:devfrom
bfoss765:fix/assetlock-router-account-coverage

Conversation

@bfoss765

@bfoss765 bfoss765 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

TransactionRouter::get_relevant_account_types(TransactionType::AssetLock) omitted the
CoinJoin and DashPay fund-bearing account types. Because only check_core_transaction
marks a UTXO spent, and it scopes spend detection to the account types this router
returns
, an asset-lock transaction funded from a CoinJoin, DashpayReceivingFunds, or
DashpayExternalAccount UTXO never had its input debited. The spent UTXO kept counting
toward the balance indefinitely, while the (BIP44) change output was credited — so the
reported balance ended up high by exactly the CoinJoin/DashPay amount spent, on both
initial relay and full rescan.

This is the rust-dashcore half of the fix tracked in dashpay/platform#4073 /
dashpay/platform#4074 (Kotlin SDK) and dashpay/dash-wallet#1507 (Android wallet).

The defect

check_core_transaction (key-wallet/src/transaction_checking/wallet_checker.rs) does:

let tx_type = TransactionRouter::classify_transaction(tx);
let relevant_types = TransactionRouter::get_relevant_account_types(&tx_type);
let mut result = self.accounts.check_transaction(tx, &relevant_types);

check_transaction is what matches a transaction's inputs against account UTXOs and marks
them spent. If an account type is not in relevant_types, its UTXOs are never considered
for spend detection for that transaction. The AssetLock arm returned only:

StandardBIP44, StandardBIP32, IdentityRegistration, IdentityTopUp,
IdentityTopUpNotBound, IdentityInvitation, AssetLockAddressTopUp,
AssetLockShieldedAddressTopUp

An asset lock is a normal funded transaction: its inputs can come from any fund-bearing
account. A heavy-mixing wallet naturally funds identity top-ups from mixed (CoinJoin)
coins. When it did, the CoinJoin input outpoint was never matched, total_sent stayed 0
for that input, and the UTXO was left in the CoinJoin account as if unspent.

Field evidence

Android SDK wallet (dashpay/dash-wallet#1507). We reconciled the SDK's tracked UTXO set
against the dashj reference wallet by outpoint diff. The SDK balance was high by exactly
the summed value of CoinJoin outpoints that had been spent by asset-lock (identity
top-up) transactions but were still present in the SDK's CoinJoin account UTXO set. Every
missing debit corresponded to an AssetLock-classified transaction whose input pointed at
a CoinJoin UTXO. The mismatch reproduced on a from-birth rescan, ruling out a transient
relay-ordering artifact — consistent with a routing omission rather than a sync gap.

The fix

Route AssetLock through the full fund-bearing account set before the identity/asset-lock
accounts. On the current dev branch this set is already factored out as
fund_bearing_account_types() (BIP44, BIP32, CoinJoin, DashpayReceivingFunds,
DashpayExternalAccount) and is already used by the Standard | CoinJoin arm, so the fix
reuses it:

TransactionType::AssetLock => {
    let mut accounts = Self::fund_bearing_account_types();
    accounts.extend([
        AccountTypeToCheck::IdentityRegistration,
        AccountTypeToCheck::IdentityTopUp,
        AccountTypeToCheck::IdentityTopUpNotBound,
        AccountTypeToCheck::IdentityInvitation,
        AccountTypeToCheck::AssetLockAddressTopUp,
        AccountTypeToCheck::AssetLockShieldedAddressTopUp,
    ]);
    accounts
}

Discovery is membership-based, exactly like Dash Core's IsMine — an account only matches
when a scriptPubKey or spent-UTXO outpoint actually belongs to it — so consulting the
extra accounts can never produce a false positive. It only closes the case where the input
genuinely does belong to a CoinJoin/DashPay account.

Note on porting: the original change (in the platform-vendored copy pinned at
1860089e) listed the five fund-bearing accounts inline, because that revision predates
the fund_bearing_account_types() helper. dev introduced that helper, so this PR ports
the same change by reusing it. Functionally identical; happy to inline it instead if you
prefer.

Test

Adds an end-to-end regression test,
wallet_checker::tests::test_asset_lock_spending_coinjoin_utxo_is_debited, that:

  1. builds a wallet with a BIP44 account and a CoinJoin account,
  2. funds a real CoinJoin external address (creating a CoinJoin UTXO),
  3. spends that UTXO with an AssetLockPayload transaction that has BIP44 change,
  4. processes it through check_core_transaction, and
  5. asserts the CoinJoin UTXO is debited (total_sent == funding_value) and removed, and
    the BIP44 change is credited.

Verified it fails on the old routing (total_sent is 0, UTXO not removed) and passes
with the fix. The existing routing::test_asset_lock_transaction_routing unit test is
also extended to assert CoinJoin/DashPay coverage.

Verification

  • cargo test -p key-wallet — 548 passed, 0 failed
  • cargo clippy -p key-wallet --all-features --all-targets -- -D warnings — clean
  • cargo fmt -p key-wallet -- --check — clean

cc @HashEngineering @QuantumExplorer — this is the AssetLock-routing PR requested on
dashpay/platform#4074 ("If rust-dashcore changes are required, then submit PRs to that
repo").

Summary by CodeRabbit

  • Bug Fixes
    • Improved routing for asset-lock transactions to include all supported fund-bearing account types (including CoinJoin and DashPay-related accounts), ensuring the correct accounts are considered.
    • Fixed wallet accounting when an asset-lock spends a CoinJoin UTXO, including correct totals and removal of the spent UTXO without inflating balances.
  • Tests
    • Added a regression test covering asset-lock spending of a CoinJoin UTXO and verifying routing and wallet balance behavior.

`TransactionRouter::get_relevant_account_types(AssetLock)` returned only the
BIP44/BIP32 standard accounts plus the identity/asset-lock accounts, omitting
the CoinJoin and DashPay fund-bearing accounts.

Only `check_core_transaction` marks a UTXO spent, and it scopes spend detection
to the account types this router returns. So an asset lock funded from a
CoinJoin or DashPay UTXO never had its input debited: the spent UTXO kept
counting toward the balance indefinitely while the (BIP44) change output was
still credited, inflating the reported balance by exactly the spent amount —
on both relay and rescan.

Field evidence (Android SDK wallet, dashpay/dash-wallet#1507): outpoint-diff
reconciliation against the dashj oracle showed the SDK balance high by exactly
the value of asset-lock-spent CoinJoin inputs that were never debited.

Fix: the AssetLock arm now consults the full `fund_bearing_account_types()` set
(BIP44, BIP32, CoinJoin, DashpayReceivingFunds, DashpayExternalAccount) before
the identity/asset-lock accounts. Discovery is membership-based like Dash
Core's `IsMine`, so consulting extra accounts never yields false positives.

Adds an end-to-end regression test that funds a CoinJoin UTXO, spends it via an
asset-lock transaction through `check_core_transaction`, and asserts the
CoinJoin UTXO is debited and removed (fails on the old routing: total_sent 0).
Also extends the AssetLock routing unit test.

Refs: dashpay/platform#4073, dashpay/platform#4074, dashpay/dash-wallet#1507

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dfb0df9d-eefd-4903-8d83-713fed6cc464

📥 Commits

Reviewing files that changed from the base of the PR and between 6b3ba44 and 34756ff.

📒 Files selected for processing (1)
  • key-wallet/src/transaction_checking/wallet_checker.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • key-wallet/src/transaction_checking/wallet_checker.rs

📝 Walkthrough

Walkthrough

AssetLock routing now includes the complete fund-bearing account set, including CoinJoin and DashPay accounts. A regression test verifies that spending a CoinJoin UTXO debits wallet totals, credits BIP44 change, and removes the spent UTXO.

Changes

AssetLock routing

Layer / File(s) Summary
Expand AssetLock account routing
key-wallet/src/transaction_checking/transaction_router/mod.rs, key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
AssetLock routing derives its account types from fund_bearing_account_types() and asserts coverage of CoinJoin and DashPay accounts.
Validate CoinJoin AssetLock spending
key-wallet/src/transaction_checking/wallet_checker.rs
A regression test verifies transaction classification, accounting, balance totals, and UTXO removal when an AssetLock spends a CoinJoin UTXO and creates BIP44 change.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: llbartekll, xdustinface

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: expanding AssetLock routing to include CoinJoin and DashPay accounts.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
key-wallet/src/transaction_checking/wallet_checker.rs (1)

702-718: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Also assert the aggregate wallet balance.

The regression is balance inflation, but the test currently verifies only per-transaction totals and UTXO removal. Add an assertion against the expected confirmed/spendable wallet balance after the spend to catch future divergence between spend_result and persisted balance state.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@key-wallet/src/transaction_checking/wallet_checker.rs` around lines 702 -
718, Extend the asset-lock spending test after the existing UTXO-removal
assertions to verify the aggregate wallet balance, including the expected
confirmed/spendable value after the spend. Use the wallet’s established balance
accessor and expected value derived from the funding and change amounts, while
preserving the current per-transaction and UTXO assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 702-718: Extend the asset-lock spending test after the existing
UTXO-removal assertions to verify the aggregate wallet balance, including the
expected confirmed/spendable value after the spend. Use the wallet’s established
balance accessor and expected value derived from the funding and change amounts,
while preserving the current per-transaction and UTXO assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c3fa87a1-620d-40ae-b307-ff4fa3154f35

📥 Commits

Reviewing files that changed from the base of the PR and between 845ee9e and 6b3ba44.

📒 Files selected for processing (3)
  • key-wallet/src/transaction_checking/transaction_router/mod.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
  • key-wallet/src/transaction_checking/wallet_checker.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 11, 2026
@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.16667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.98%. Comparing base (845ee9e) to head (34756ff).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
...-wallet/src/transaction_checking/wallet_checker.rs 99.09% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #867      +/-   ##
==========================================
+ Coverage   73.62%   73.98%   +0.36%     
==========================================
  Files         324      324              
  Lines       73374    73545     +171     
==========================================
+ Hits        54018    54409     +391     
+ Misses      19356    19136     -220     
Flag Coverage Δ
core 77.08% <ø> (ø)
ffi 48.03% <ø> (+1.95%) ⬆️
rpc 20.00% <ø> (ø)
spv 91.09% <ø> (+0.14%) ⬆️
wallet 73.39% <99.16%> (+0.27%) ⬆️
Files with missing lines Coverage Δ
...src/transaction_checking/transaction_router/mod.rs 89.14% <100.00%> (+0.08%) ⬆️
...-wallet/src/transaction_checking/wallet_checker.rs 99.24% <99.09%> (-0.02%) ⬇️

... and 25 files with indirect coverage changes

…oinJoin-debit test

Addresses CodeRabbit's nitpick on dashpay#867. The regression the
test guards against is balance inflation, but it previously asserted only the
per-transaction totals and the CoinJoin UTXO removal. Add confirmed/spendable/
total balance assertions after the spend: a non-debited CoinJoin coin would
report funding_value + change_value; a correct debit leaves only the confirmed
BIP44 change (the asset-lock credit output is locked into Platform, never a
spendable wallet UTXO). Existing assertions preserved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the ready-for-review CodeRabbit has approved this PR label Jul 11, 2026
@bfoss765

Copy link
Copy Markdown
Contributor Author

Added the aggregate-balance assertions: after the spend, confirmed/spendable/total all equal change_value — a non-debited CoinJoin coin would inflate them to funding_value + change_value (the asset-lock credit output is locked into Platform, never a spendable wallet UTXO). Existing per-transaction and UTXO-removal assertions kept.

@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label Jul 11, 2026
@xdustinface xdustinface merged commit 93b7477 into dashpay:dev Jul 13, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants