fix(dash-spv): harden Windows dashd_sync startup wallet load#904
fix(dash-spv): harden Windows dashd_sync startup wallet load#904thepastaclaw wants to merge 7 commits into
Conversation
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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDashd test utilities now clear stale runtime locks, retain failed startup datadirs conditionally, provide configurable readiness diagnostics, distinguish wallet RPC states, and reuse wallet-scoped transaction helpers. ChangesDashd test startup reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DashdTestContext
participant DashCoreNode
participant dashd
participant RPC
DashdTestContext->>DashCoreNode: start node
DashCoreNode->>dashd: clear locks and spawn
DashCoreNode->>RPC: probe P2P, cookie, and RPC readiness
RPC-->>DashCoreNode: readiness result or diagnostic error
DashCoreNode-->>DashdTestContext: ready or startup failure
DashdTestContext->>RPC: load default mining wallet
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #904 +/- ##
==========================================
- Coverage 74.54% 74.18% -0.37%
==========================================
Files 327 327
Lines 75032 75032
==========================================
- Hits 55936 55659 -277
- Misses 19096 19373 +277
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
dash-spv/src/test_utils/node.rs (2)
887-915: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the newly added filesystem and size-limit branches.
Move the lock-cleanup test into
fs_helpers.rsand coverregtest/wallets/<name>/.walletlock. Also add a log larger than 64 KiB to verify the capped-tail path and partial-line removal.As per coding guidelines, individual components require comprehensive in-module unit tests.
🤖 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 887 - 915, Expand the in-module tests for the filesystem helpers: move clear_stale_runtime_locks_removes_fixture_locks into fs_helpers.rs and include a regtest/wallets/<name>/.walletlock fixture. Update read_log_tail_returns_last_lines to create a log exceeding 64 KiB, assert the returned tail respects the cap, and verify any incomplete leading line is removed.Source: Coding guidelines
269-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove loopback addresses into node configuration.
The changed readiness and RPC paths hardcode
127.0.0.1. Derive both socket addresses and URLs from configured network parameters instead.As per coding guidelines, network parameters and addresses must never be hardcoded.
Also applies to: 339-347, 354-360
🤖 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 269 - 270, The readiness and RPC paths currently hardcode the loopback address; update the node configuration used by the relevant connection checks and URL construction around the shown TcpStream readiness logic and the additional RPC paths to provide the configured bind/host address. Derive both SocketAddr values and RPC URLs from those configuration parameters, preserving the existing ports and behavior without any literal loopback addresses.Source: Coding guidelines
🤖 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/fs_helpers.rs`:
- Around line 27-53: Update clear_stale_runtime_locks and remove_if_exists to
return io::Result<()> and propagate filesystem and directory-read errors instead
of logging and continuing. Adjust DashCoreNode::start to handle the cleanup
result and fail immediately when stale-lock removal is incomplete, while
preserving successful cleanup behavior for missing files.
In `@dash-spv/src/test_utils/node.rs`:
- Around line 746-755: Update wallet_does_not_exist to require code ==
RPC_WALLET_NOT_FOUND alongside the existing wallet-related and missing-message
checks; remove the broad nonzero-code condition. Add a test covering the
wallet-not-found RPC code and ensure unrelated nonzero errors such as -32601 do
not authorize createwallet.
- Around line 233-246: Ensure startup-failure retention waits for dashd to fully
exit before reading or copying the datadir: update fail_startup in
dash-spv/src/test_utils/node.rs (lines 233-246) to use a blocking child wait
after initiating termination. In dash-spv/src/test_utils/context.rs (lines
67-73), ensure node shutdown occurs before RetainOnPanic is dropped and performs
retention. dash-spv/src/test_utils/fs_helpers.rs (lines 98-120) requires no
direct change; its synchronous retention path must be invoked only after the
process has exited.
- Around line 262-298: Update the startup readiness loop around
try_rpc_client_base and get_blockchain_info so the synchronous RPC probe runs
via spawn_blocking or an async RPC client and cannot stall the timeout future.
Remove the early P2P readiness continue, ensuring every loop independently
samples P2P connectivity, RPC/cookie state, and blockchain readiness while
preserving the existing diagnostic state updates.
---
Nitpick comments:
In `@dash-spv/src/test_utils/node.rs`:
- Around line 887-915: Expand the in-module tests for the filesystem helpers:
move clear_stale_runtime_locks_removes_fixture_locks into fs_helpers.rs and
include a regtest/wallets/<name>/.walletlock fixture. Update
read_log_tail_returns_last_lines to create a log exceeding 64 KiB, assert the
returned tail respects the cap, and verify any incomplete leading line is
removed.
- Around line 269-270: The readiness and RPC paths currently hardcode the
loopback address; update the node configuration used by the relevant connection
checks and URL construction around the shown TcpStream readiness logic and the
additional RPC paths to provide the configured bind/host address. Derive both
SocketAddr values and RPC URLs from those configuration parameters, preserving
the existing ports and behavior without any literal loopback addresses.
🪄 Autofix (Beta)
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
Run ID: 7242bcd0-e56b-4107-951f-147b9c728a6c
📒 Files selected for processing (3)
dash-spv/src/test_utils/context.rsdash-spv/src/test_utils/fs_helpers.rsdash-spv/src/test_utils/node.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
1 participant
Summary
Fixes intermittent Windows
dashd_syncfailures tracked in #903:failed to create wallet 'default' ... Database already existsdashd failed to start within timeoutRoot cause
The regtest fixtures already ship both
walletanddefaultunderregtest/.DashdTestContextstarts dashd with-wallet=wallet, then needs the miningdefaultwallet loaded.The old
ensure_wallettreated anyloadwalleterror as permission to callcreatewallet. When the fixture wallet was present but load failed for a non-missing reason, create raced into Core's-4 Database already existspanic — observed on Windows under parallel suite load.Startup failures were also hard to diagnose: readiness only returned a boolean, retained no datadir on mid-setup panics, and did not surface process exit / cookie / last RPC error context.
Fix
load_walletfor fixture wallets that must never be recreated; context uses it for miningdefault.ensure_walletfor create-if-missing cases, but only create when Core reports a wallet-related missing error and no on-disk DB exists.not found,already loaded,database already exists) with message+code guards that reject unrelated "not found" responses..lock/.walletlockfiles before start (including legacyregtest/.walletlock).p2p_ready/cookie_seen/last RPC error on timeout, dumpdebug.logtail, retain datadir once for CI artifacts.DASHD_STARTUP_TIMEOUT_SECS. Capacity allowance for parallel dashd starts, not a blind sleep.Validation
cargo test -p dash-spv --features test-utils --lib test_utils::node::tests(6 passed) covering wallet error classification (including Windows CI "Database already exists" message and "Method not found" false-positive rejection), fixture lock cleanup, log tailing, and database-path detection.cargo fmt -p dash-spvcargo clippy -p dash-spv --features test-utils --all-targets -- -D warnings-version(unsigned binary / host policy). Windows validation is deferred to CI on this PR.Test plan
Windows / spvgreen on this head (primary signal for CI: Windows dashd_sync tests intermittently fail during node startup #903)spvstill greenDASHD_TEST_RETAIN_DIRshould be presentFixes #903
Summary by CodeRabbit
Bug Fixes
Tests