Skip to content

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

Draft
thepastaclaw wants to merge 11 commits into
dashpay:devfrom
thepastaclaw:tracker-2100
Draft

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

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

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)

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

Related

Summary by CodeRabbit

  • Bug Fixes

    • Coinbase and asset unlock transactions now correctly recognize all fund-bearing wallet account types, including CoinJoin and DashPay accounts.
    • Payments to CoinJoin addresses are now detected and recorded reliably.
    • CoinJoin-related UTXOs and wallet balances now update correctly for both immature and confirmed transactions.
  • Reliability

    • Improved transaction routing coverage helps ensure wallet balances remain accurate across supported account types.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR improves Dash Core test-node startup, diagnostics, wallet lifecycle, and cleanup. It also updates Coinbase and AssetUnlock routing to include all fund-bearing account types and adds CoinJoin regression coverage.

Changes

Dash SPV test runtime

Layer / File(s) Summary
Runtime lock cleanup and panic retention
dash-spv/src/test_utils/fs_helpers.rs, dash-spv/src/test_utils/context.rs
Stale runtime locks are removed. Test directories are retained during setup failures and panic unwinding.
Node startup and readiness diagnostics
dash-spv/src/test_utils/node.rs
Startup uses configurable timeouts, concurrent readiness checks, process-exit detection, shutdown handling, and bounded diagnostic log tails.
Wallet and context lifecycle
dash-spv/src/test_utils/node.rs, dash-spv/src/test_utils/context.rs
Wallet loading and creation classify RPC errors. Sending uses wallet-specific helpers. Shutdown waits for process termination. Context initialization loads the fixture wallet and defuses panic retention after success.

Fund-bearing transaction routing

Layer / File(s) Summary
Fund-bearing account routing and assertions
key-wallet/src/transaction_checking/transaction_router/mod.rs, key-wallet/src/transaction_checking/transaction_router/tests/*
Coinbase and AssetUnlock transactions route through every fund-bearing account type. Router tests assert the complete account set.
CoinJoin discovery and balance regressions
key-wallet/src/transaction_checking/wallet_checker.rs
Regression tests verify CoinJoin outputs from Coinbase and AssetUnlock transactions create UTXOs and update balances.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DashCoreNode
  participant DashdProcess
  participant RuntimeFilesystem
  participant RPC
  DashCoreNode->>RuntimeFilesystem: Clear stale runtime locks
  DashCoreNode->>DashdProcess: Start dashd
  DashCoreNode->>RPC: Probe readiness
  DashCoreNode->>DashdProcess: Wait for shutdown
  DashCoreNode->>RuntimeFilesystem: Retain diagnostics when setup fails
Loading

Possibly related PRs

Suggested reviewers: xdustinface, quantumexplorer

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The dash-spv test-harness hardening changes are unrelated to the linked key-wallet issue [#900]. Move the dash-spv harness changes to a separate pull request linked to the issue that tracks the Windows test-harness failures.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The key-wallet changes satisfy issue [#900] by routing both transaction types through all fund-bearing accounts and adding routing and CoinJoin regression tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary key-wallet change: discovering Coinbase and AssetUnlock outputs for all fund-bearing accounts.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.09%. Comparing base (dca5b05) to head (3c04c73).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #901      +/-   ##
==========================================
+ Coverage   75.07%   75.09%   +0.01%     
==========================================
  Files         328      328              
  Lines       77587    77716     +129     
==========================================
+ Hits        58245    58357     +112     
- Misses      19342    19359      +17     
Flag Coverage Δ
core 77.29% <ø> (ø)
ffi 49.85% <ø> (ø)
rpc 20.00% <ø> (ø)
spv 91.30% <ø> (-0.10%) ⬇️
wallet 76.16% <100.00%> (+0.13%) ⬆️
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 6 files with indirect coverage changes

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

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.

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

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.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

@github-actions github-actions Bot added the merge-conflict The PR conflicts with the target branch. label Aug 6, 2026
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.
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

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@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.

Actionable comments posted: 2

🧹 Nitpick comments (5)
key-wallet/src/transaction_checking/transaction_router/mod.rs (1)

161-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Split the SPV harness changes into a separate PR.

PR #901 combines the #900 key-wallet routing fix with the unrelated #903 dash-spv startup hardening. Move the three dash-spv/src/test_utils changes to a separate PR. Keep the existing fix(key-wallet): title for the routing PR.

🤖 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/transaction_router/mod.rs` around lines
161 - 170, Separate the unrelated dash-spv startup-hardening changes under
dash-spv/src/test_utils into another PR, leaving this PR focused on the
key-wallet routing update in the TransactionType::AssetUnlock |
TransactionType::Coinbase branch and Self::fund_bearing_account_types. Preserve
the existing fix(key-wallet): PR title.

Source: Path instructions

dash-spv/src/test_utils/node.rs (3)

872-881: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Require a wallet database file, not just a directory name.

The last two candidates match any directory named wallet_name under regtest or regtest/wallets. A wallet name that collides with an existing datadir entry, for example wallets, reports a database that does not exist. ensure_wallet then panics instead of creating the wallet. Restrict the bare-directory candidates to entries that contain a wallet database file.

🤖 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 `@dash-spv/src/test_utils/node.rs` around lines 872 - 881, Update
wallet_database_exists to ensure the bare-directory candidates under regtest and
regtest/wallets only match when they contain a wallet database file, rather than
accepting the directory itself. Preserve the existing wallet.dat candidate
checks and ensure names such as “wallets” do not report an existing wallet
without a database file.

12-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider splitting the harness hardening from the routing fix.

This PR contains two unrelated concerns. The first is the key-wallet routing change for Coinbase and AssetUnlock fund-bearing account types. The second is the Windows dashd harness hardening tracked by issue #903, which covers dash-spv/src/test_utils/node.rs, context.rs, and fs_helpers.rs. Separate PRs would keep each change reviewable and revertable on its own. If you keep them together, use a title prefix that describes the dominant change, because the harness work is test/ci scoped while the routing change is a fix.

As per path instructions: "If a PR mixes unrelated concerns (e.g., a bug fix bundled with a refactor or new feature), suggest splitting it into separate focused PRs."

🤖 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 `@dash-spv/src/test_utils/node.rs` around lines 12 - 48, Separate the unrelated
key-wallet routing changes from the Windows dashd harness hardening across
node.rs, context.rs, and fs_helpers.rs into focused changes or PRs. If they must
remain together, use a title prefix describing the dominant routing fix while
identifying the harness work as test/CI scoped.

Source: Path instructions


903-912: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Read the log tail as bytes to survive a split UTF-8 sequence.

After a mid-file seek, the first bytes can be the tail of a multi-byte character. read_to_string then fails and read_log_tail returns "failed to read" instead of the log tail. This removes the diagnostics that the startup panic depends on. Read bytes and convert with from_utf8_lossy.

♻️ Proposed refactor
-    let mut contents = String::new();
-    if let Err(e) = file.read_to_string(&mut contents) {
+    let mut raw = Vec::new();
+    if let Err(e) = file.read_to_end(&mut raw) {
         return format!("  <failed to read {}: {}>", path.display(), e);
     }
+    let contents = String::from_utf8_lossy(&raw).into_owned();

Note that use std::io::Read; at Line 12 stays required for read_to_end.

🤖 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 `@dash-spv/src/test_utils/node.rs` around lines 903 - 912, Update read_log_tail
to read the file contents into a byte buffer with read_to_end instead of
read_to_string, then convert the bytes using String::from_utf8_lossy so a seek
beginning within a UTF-8 character still produces the available log tail.
Preserve the existing partial-first-line removal and failure behavior, and
retain the std::io::Read import.
dash-spv/src/test_utils/fs_helpers.rs (1)

129-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a disarm flag instead of std::mem::forget.

defuse leaks the PathBuf and String allocations. A boolean flag skips the retain without leaking and states the intent directly.

♻️ Proposed refactor
 pub(super) struct RetainOnPanic {
     path: PathBuf,
     label: String,
+    armed: bool,
 }
 
 impl RetainOnPanic {
     pub(super) fn new(path: impl Into<PathBuf>, label: impl Into<String>) -> Self {
         Self {
             path: path.into(),
             label: label.into(),
+            armed: true,
         }
     }
 
-    pub(super) fn defuse(self) {
-        std::mem::forget(self);
+    pub(super) fn defuse(mut self) {
+        self.armed = false;
     }
 }
 
 impl Drop for RetainOnPanic {
     fn drop(&mut self) {
-        if std::thread::panicking() {
+        if self.armed && std::thread::panicking() {
             // Already know we are panicking; skip retain_test_dir's re-check.
             retain_test_dir_now(&self.path, &self.label);
         }
     }
 }
🤖 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 `@dash-spv/src/test_utils/fs_helpers.rs` around lines 129 - 131, Update the
RAII guard containing defuse so it uses a boolean disarm flag instead of
std::mem::forget(self). Have defuse mark the guard as disarmed, and make its
Drop implementation skip cleanup when that flag is set while retaining normal
cleanup otherwise.
🤖 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.

Inline comments:
In `@dash-spv/src/test_utils/node.rs`:
- Around line 739-763: Update DashCoreNode::stop_and_wait to enforce a finite
deadline while polling process.try_wait, preserving the existing exit and
wait-error handling. When the deadline expires before the dashd child exits, log
a warning with sufficient process context and stop waiting so callers such as
Drop cannot hang indefinitely.

In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 293-315: Update wallet_with_coinjoin_address to accept a Network
parameter instead of hardcoding Network::Testnet, and pass that parameter to
Wallet::new_random. Update both new credit regression tests that use this
fixture to run for Network::Mainnet and Network::Testnet.

---

Nitpick comments:
In `@dash-spv/src/test_utils/fs_helpers.rs`:
- Around line 129-131: Update the RAII guard containing defuse so it uses a
boolean disarm flag instead of std::mem::forget(self). Have defuse mark the
guard as disarmed, and make its Drop implementation skip cleanup when that flag
is set while retaining normal cleanup otherwise.

In `@dash-spv/src/test_utils/node.rs`:
- Around line 872-881: Update wallet_database_exists to ensure the
bare-directory candidates under regtest and regtest/wallets only match when they
contain a wallet database file, rather than accepting the directory itself.
Preserve the existing wallet.dat candidate checks and ensure names such as
“wallets” do not report an existing wallet without a database file.
- Around line 12-48: Separate the unrelated key-wallet routing changes from the
Windows dashd harness hardening across node.rs, context.rs, and fs_helpers.rs
into focused changes or PRs. If they must remain together, use a title prefix
describing the dominant routing fix while identifying the harness work as
test/CI scoped.
- Around line 903-912: Update read_log_tail to read the file contents into a
byte buffer with read_to_end instead of read_to_string, then convert the bytes
using String::from_utf8_lossy so a seek beginning within a UTF-8 character still
produces the available log tail. Preserve the existing partial-first-line
removal and failure behavior, and retain the std::io::Read import.

In `@key-wallet/src/transaction_checking/transaction_router/mod.rs`:
- Around line 161-170: Separate the unrelated dash-spv startup-hardening changes
under dash-spv/src/test_utils into another PR, leaving this PR focused on the
key-wallet routing update in the TransactionType::AssetUnlock |
TransactionType::Coinbase branch and Self::fund_bearing_account_types. Preserve
the existing fix(key-wallet): PR title.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: eb0e36ec-124f-4077-807d-6f398a23c449

📥 Commits

Reviewing files that changed from the base of the PR and between dca5b05 and 3c04c73.

📒 Files selected for processing (7)
  • dash-spv/src/test_utils/context.rs
  • dash-spv/src/test_utils/fs_helpers.rs
  • dash-spv/src/test_utils/node.rs
  • key-wallet/src/transaction_checking/transaction_router/mod.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs
  • key-wallet/src/transaction_checking/wallet_checker.rs

Comment on lines +739 to +763
pub(super) fn stop_and_wait(&mut self) {
let Some(mut process) = self.process.take() else {
return;
};

tracing::info!("Stopping dashd process...");
if let Err(e) = process.start_kill() {
tracing::warn!("Failed to request dashd shutdown: {}", e);
}
loop {
match process.try_wait() {
Ok(Some(status)) => {
tracing::info!("dashd process exited with {}", status);
break;
}
Ok(None) => {
std::thread::sleep(Duration::from_millis(50));
}
Err(e) => {
tracing::warn!("Failed to wait for dashd process exit: {}", e);
break;
}
}
}
}

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound the wait loop in stop_and_wait.

The loop has no deadline. If the child process never reports an exit status, stop_and_wait spins forever with 50 ms sleeps and no diagnostic output. Drop for DashCoreNode and DashdTestContext::drop both call it, so a stuck process converts a test failure into a CI hang with no message. Add a deadline and log a warning when it expires.

🛡️ Proposed fix
         tracing::info!("Stopping dashd process...");
         if let Err(e) = process.start_kill() {
             tracing::warn!("Failed to request dashd shutdown: {}", e);
         }
-        loop {
+        let deadline = std::time::Instant::now() + Duration::from_secs(30);
+        loop {
             match process.try_wait() {
                 Ok(Some(status)) => {
                     tracing::info!("dashd process exited with {}", status);
                     break;
                 }
                 Ok(None) => {
+                    if std::time::Instant::now() >= deadline {
+                        tracing::warn!("dashd process did not exit within 30s; giving up");
+                        break;
+                    }
                     std::thread::sleep(Duration::from_millis(50));
                 }
                 Err(e) => {
                     tracing::warn!("Failed to wait for dashd process exit: {}", e);
                     break;
                 }
             }
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub(super) fn stop_and_wait(&mut self) {
let Some(mut process) = self.process.take() else {
return;
};
tracing::info!("Stopping dashd process...");
if let Err(e) = process.start_kill() {
tracing::warn!("Failed to request dashd shutdown: {}", e);
}
loop {
match process.try_wait() {
Ok(Some(status)) => {
tracing::info!("dashd process exited with {}", status);
break;
}
Ok(None) => {
std::thread::sleep(Duration::from_millis(50));
}
Err(e) => {
tracing::warn!("Failed to wait for dashd process exit: {}", e);
break;
}
}
}
}
pub(super) fn stop_and_wait(&mut self) {
let Some(mut process) = self.process.take() else {
return;
};
tracing::info!("Stopping dashd process...");
if let Err(e) = process.start_kill() {
tracing::warn!("Failed to request dashd shutdown: {}", e);
}
let deadline = std::time::Instant::now() + Duration::from_secs(30);
loop {
match process.try_wait() {
Ok(Some(status)) => {
tracing::info!("dashd process exited with {}", status);
break;
}
Ok(None) => {
if std::time::Instant::now() >= deadline {
tracing::warn!("dashd process did not exit within 30s; giving up");
break;
}
std::thread::sleep(Duration::from_millis(50));
}
Err(e) => {
tracing::warn!("Failed to wait for dashd process exit: {}", e);
break;
}
}
}
}
🤖 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 `@dash-spv/src/test_utils/node.rs` around lines 739 - 763, Update
DashCoreNode::stop_and_wait to enforce a finite deadline while polling
process.try_wait, preserving the existing exit and wait-error handling. When the
deadline expires before the dashd child exits, log a warning with sufficient
process context and stop waiting so callers such as Drop cannot hang
indefinitely.

Comment on lines +293 to +315
fn wallet_with_coinjoin_address() -> (Wallet, ManagedWalletInfo, Address) {
let network = Network::Testnet;
let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None)
.expect("Should create wallet");
wallet
.add_account(
AccountType::CoinJoin {
index: 0,
},
None,
)
.expect("Should add CoinJoin account");
let mut managed_wallet =
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
let coinjoin_xpub =
wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub;
let coinjoin_address = managed_wallet
.first_coinjoin_managed_account_mut()
.expect("managed coinjoin")
.next_address(Some(&coinjoin_xpub), true)
.expect("coinjoin address");
(wallet, managed_wallet, coinjoin_address)
}

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Parameterize the fixture over Network.

Line 294 hardcodes Network::Testnet. The Rust guidelines prohibit hardcoded network parameters. Accept Network as a fixture parameter. Run both new credit regressions with Network::Mainnet and Network::Testnet.

Proposed fixture change
-fn wallet_with_coinjoin_address() -> (Wallet, ManagedWalletInfo, Address) {
-    let network = Network::Testnet;
+fn wallet_with_coinjoin_address(network: Network) -> (Wallet, ManagedWalletInfo, Address) {

As per coding guidelines, “Never hardcode network parameters, addresses, or keys.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn wallet_with_coinjoin_address() -> (Wallet, ManagedWalletInfo, Address) {
let network = Network::Testnet;
let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None)
.expect("Should create wallet");
wallet
.add_account(
AccountType::CoinJoin {
index: 0,
},
None,
)
.expect("Should add CoinJoin account");
let mut managed_wallet =
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
let coinjoin_xpub =
wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub;
let coinjoin_address = managed_wallet
.first_coinjoin_managed_account_mut()
.expect("managed coinjoin")
.next_address(Some(&coinjoin_xpub), true)
.expect("coinjoin address");
(wallet, managed_wallet, coinjoin_address)
}
fn wallet_with_coinjoin_address(
network: Network,
) -> (Wallet, ManagedWalletInfo, Address) {
let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::None)
.expect("Should create wallet");
wallet
.add_account(
AccountType::CoinJoin {
index: 0,
},
None,
)
.expect("Should add CoinJoin account");
let mut managed_wallet =
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
let coinjoin_xpub =
wallet.accounts.coinjoin_accounts.get(&0).expect("coinjoin account").account_xpub;
let coinjoin_address = managed_wallet
.first_coinjoin_managed_account_mut()
.expect("managed coinjoin")
.next_address(Some(&coinjoin_xpub), true)
.expect("coinjoin address");
(wallet, managed_wallet, coinjoin_address)
}
🤖 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 293 -
315, Update wallet_with_coinjoin_address to accept a Network parameter instead
of hardcoding Network::Testnet, and pass that parameter to Wallet::new_random.
Update both new credit regression tests that use this fixture to run for
Network::Mainnet and Network::Testnet.

Source: Coding guidelines

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

@github-actions github-actions Bot removed the merge-conflict The PR conflicts with the target branch. label Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

key-wallet: coinbase and asset-unlock outputs paying CoinJoin/DashPay addresses are never discovered

1 participant