fix(key-wallet): discover Coinbase/AssetUnlock outputs for all fund-bearing accounts by thepastaclaw · Pull Request #901 · dashpay/rust-dashcore

thepastaclaw · 2026-07-16T13:35:10Z

Summary

TransactionRouter::get_relevant_account_types returned only StandardBIP44 and StandardBIP32 for Coinbase and AssetUnlock, while fund_bearing_account_types() is {StandardBIP44, StandardBIP32, CoinJoin, DashpayReceivingFunds, DashpayExternalAccount}. An output of a coinbase (masternode payout / mining reward) or an asset-unlock (Platform credit withdrawal) that pays a CoinJoin or DashPay address was therefore never matched, so the coin was never credited and the balance undercounted.

This is the credit-side mirror of the debit-side routing bug fixed in #867 for AssetLock. The same membership-based reasoning applies: discovery is like Dash Core's IsMine, so consulting the full fund-bearing set cannot yield a false positive. Ownership still requires a scriptPubKey or spent UTXO that actually belongs to the account.

Note the filter layer already does the right thing — the wallet queries all of its scripts, so the block is downloaded on a match. The output was then dropped purely by the account-type narrowing (wasted download + lost coin).

Closes #900

The fix

Replace the hand-written type lists in the Coinbase and AssetUnlock arms with fund_bearing_account_types(), mirroring #867. Roughly -8/+4 in production code; no signature changes.

Also corrected the stale Coinbase comment that said "Check all account types" while listing only two.

Membership safety (verified against source)

check_core_transaction scopes ownership detection to get_relevant_account_types.
An account only matches when a scriptPubKey / UTXO is actually in its pools — extra account types never invent credits.
fund_bearing_account_types() is the same set used for Standard / CoinJoin discovery and for the post-fix(key-wallet): cover CoinJoin/DashPay accounts for AssetLock routing #867 AssetLock debit path.
Unifying the three hand-written fund-bearing lists (fund_bearing_account_types, all_funding_accounts, update_utxos match) remains a separate refactor, as noted in key-wallet: coinbase and asset-unlock outputs paying CoinJoin/DashPay addresses are never discovered #900.

Tests

Router unit tests for Coinbase and AssetUnlock assert equality with fund_bearing_account_types() (single source of truth).
Behavioral regressions:
- test_coinbase_paying_coinjoin_address_is_credited — coinbase to CoinJoin creates UTXO + immature/total balance credit.
- test_asset_unlock_paying_coinjoin_address_is_credited — AssetUnlock to CoinJoin creates UTXO + confirmed/total balance credit.
Shared wallet_with_coinjoin_address() fixture for the credit tests.

Validation

cargo test -p key-wallet --lib transaction_checking   # 107 passed
cargo clippy -p key-wallet --all-targets -- -D warnings
cargo fmt -p key-wallet -- --check

Coinbase and AssetUnlock outputs can pay any user-chosen address, including CoinJoin and DashPay. get_relevant_account_types previously returned only StandardBIP44/BIP32 for those classifications, so a match on those addresses was dropped after the block download — the credit-side mirror of the dashpay#867 AssetLock debit bug. Use fund_bearing_account_types() for both arms (membership-based discovery, like Dash Core's IsMine). Update routing unit tests to expect all five fund-bearing types. Closes dashpay#900

End-to-end regressions for dashpay#900: a coinbase mining reward and an AssetUnlock Platform withdrawal that pay a CoinJoin address must be discovered, create a UTXO on that account, and credit the wallet balance. Mirrors the dashpay#867 AssetLock debit regression shape; fails on pre-fix routing where only StandardBIP44/BIP32 were consulted for those classifications.

…serts Extract wallet_with_coinjoin_address() for the dashpay#900 credit regressions and assert Coinbase/AssetUnlock routing against fund_bearing_account_types() instead of re-listing the five types. Keeps the production list as the single source of truth for those unit tests.

coderabbitai · 2026-07-16T13:35:17Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 16d09170-d470-40e7-a1f5-ec4351210aea

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ 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.}

codecov · 2026-07-16T13:47:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.79%. Comparing base (19690d3) to head (528c52c).
⚠️ Report is 7 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #901      +/-   ##
==========================================
+ Coverage   74.54%   74.79%   +0.24%     
==========================================
  Files         327      328       +1     
  Lines       75032    76722    +1690     
==========================================
+ Hits        55936    57382    +1446     
- Misses      19096    19340     +244

Flag	Coverage Δ
core	`77.29% <ø> (ø)`
ffi	`49.32% <ø> (-1.63%)`	⬇️
rpc	`20.00% <ø> (ø)`
spv	`91.10% <ø> (-0.11%)`	⬇️
wallet	`75.84% <100.00%> (+1.43%)`	⬆️

Files with missing lines	Coverage Δ
...src/transaction_checking/transaction_router/mod.rs	`88.88% <100.00%> (-0.26%)`	⬇️
...-wallet/src/transaction_checking/wallet_checker.rs	`99.31% <100.00%> (+0.06%)`	⬆️

... and 53 files with indirect coverage changes

thepastaclaw · 2026-07-16T14:10:37Z

The failed Windows / spv job is unrelated to this PR. This branch changes only key-wallet transaction routing and focused wallet tests; it does not touch dash-spv, dash-spv/src/test_utils/node.rs, or tests/dashd_sync.

The job failed in the shared Windows dashd integration harness while starting nodes: two tests timed out waiting for dashd, and another hit create_wallet("default") after the database path already existed. The same Windows-only failure signature occurred on merged PR #891, whose diff was likewise confined to key-wallet/key-wallet-manager, but with a different set of dashd_sync tests failing. Ubuntu, Ubuntu ARM, and macOS SPV all pass here.

The PR is proper as-is; no branch change or CI retrigger is warranted. I filed #903 to track the pre-existing Windows integration-test flake with both run links and investigation notes.

Stop treating every loadwallet failure as permission to createwallet. The regtest fixtures already ship a `default` wallet, so the old fallback raced into "Database already exists" under parallel Windows CI. Clear stale fixture lock files, classify wallet RPC errors, surface readiness failures with debug.log context, and retain datadirs on startup panics so CI artifacts are useful. Fixes: dashpay#903

Remove needless returns in wallet availability confirmation and apply rustfmt to the readiness timeout helper.

Share cookie-based Client construction across wallet and base RPC paths, and clear stale fixture locks only in DashCoreNode::start so all callers get the same policy without a double walk from DashdTestContext.

Install RetainOnPanic only after start succeeds so startup failures retain once, share cookie-based soft/hard RPC client construction, and thin-wrap primary wallet send helpers. Fixture mining wallet now uses load_wallet.

Require a wallet-related message before treating a loadwallet failure as permission to createwallet, and cover the "Method not found" false positive in unit tests.

Stop the node before reading/copying debug.log so Windows retain does not hit sharing violations on open handles, and only load the last 64KiB of debug.log when dumping a readiness failure.

thepastaclaw · 2026-08-03T02:38:17Z

CI recovery: Windows / spv dashd startup

Pushed the #903 harness hardening (from #904, CI-green there including Windows / spv) onto this branch so PR CI can re-run against the real failure mode.

Root cause (unchanged product code): parallel Windows dashd_sync startups hit (1) readiness timeouts with opaque panics and (2) ensure_wallet("default") falling through to createwallet when the fixture DB already existed.

Harness fix (dash-spv test_utils only):

load_wallet for fixture mining wallet (never create)
stricter wallet-error classification + on-disk DB guards in ensure_wallet
clear stale fixture .lock/.walletlock before start
Windows readiness default 90s (DASHD_STARTUP_TIMEOUT_SECS override)
process-aware readiness + debug.log tail + retain on startup failure

Head: 528c52cc (7 commits cherry-picked from fix-windows-dashd-sync-startup-flake atop prior key-wallet work).

Local validation: test_utils unit tests 11/11, key-wallet transaction_checking 107/107, fmt/clippy -D warnings clean for dash-spv (test-utils) and key-wallet.

thepastaclaw added 4 commits July 16, 2026 08:30

style(key-wallet): rustfmt wallet_checker CoinJoin credit tests

8d09ce1

thepastaclaw mentioned this pull request Jul 16, 2026

CI: Windows dashd_sync tests intermittently fail during node startup #903

Open

thepastaclaw added 7 commits August 2, 2026 21:34

style(dash-spv): satisfy clippy and rustfmt on test harness

2428c52

Remove needless returns in wallet availability confirmation and apply rustfmt to the readiness timeout helper.

refactor(dash-spv): dedupe dashd test RPC client and lock cleanup

89495c2

Share cookie-based Client construction across wallet and base RPC paths, and clear stale fixture locks only in DashCoreNode::start so all callers get the same policy without a double walk from DashdTestContext.

fix(dash-spv): tighten wallet-not-found classification

fbb9d7b

Require a wallet-related message before treating a loadwallet failure as permission to createwallet, and cover the "Method not found" false positive in unit tests.

fix(dash-spv): kill dashd before retaining failed startup datadir

245e6ff

Stop the node before reading/copying debug.log so Windows retain does not hit sharing violations on open handles, and only load the last 64KiB of debug.log when dumping a readiness failure.

fix(dash-spv): address startup review findings

528c52c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(key-wallet): discover Coinbase/AssetUnlock outputs for all fund-bearing accounts - #901

fix(key-wallet): discover Coinbase/AssetUnlock outputs for all fund-bearing accounts#901
thepastaclaw wants to merge 11 commits into
dashpay:devfrom
thepastaclaw:tracker-2100

thepastaclaw commented Jul 16, 2026

Uh oh!

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading

Review skipped

Uh oh!

codecov Bot commented Jul 16, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Jul 16, 2026

Uh oh!

thepastaclaw commented Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

thepastaclaw commented Jul 16, 2026

Summary

The fix

Membership safety (verified against source)

Tests

Validation

Related

Uh oh!

coderabbitai Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

codecov Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thepastaclaw commented Jul 16, 2026

Uh oh!

thepastaclaw commented Aug 3, 2026

CI recovery: Windows / spv dashd startup

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading

codecov Bot commented Jul 16, 2026 •

edited

Loading