test(dash-spv): add masternode integration tests#740
Open
xdustinface wants to merge 3 commits intov0.42-devfrom
Open
test(dash-spv): add masternode integration tests#740xdustinface wants to merge 3 commits intov0.42-devfrom
xdustinface wants to merge 3 commits intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds marker-based test-data handling and bumps CI TEST_DATA_VERSION to v0.0.4; centralizes test utilities (wallet creation, logging, node RPC helpers); implements MasternodeTestContext for regtest orchestration with DKG/ChainLock helpers; and adds end-to-end masternode-list sync and InstantSend integration tests. ChangesMasternode Integration Test Infrastructure
Sequence Diagram(s)sequenceDiagram
participant DevTest as Test Runner
participant MNCtx as MasternodeTestContext
participant Controller as dashd Controller
participant MN as Masternode Nodes
participant SPV as DashSpvClient
DevTest->>MNCtx: new(controller_only)
MNCtx->>Controller: start controller dashd
MNCtx->>MN: start masternode dashd instances
MNCtx->>Controller: addnode / connect_all_nodes
MNCtx->>Controller: protx update_service (RPC) -> mine block
DevTest->>SPV: create_and_start_client
SPV->>MNCtx: subscribe events / waiters
DevTest->>MNCtx: mine_dkg_cycle
MNCtx->>SPV: emit MasternodeStateUpdated / InstantLockReceived
DevTest->>SPV: wait_for_chainlock_height_at_least
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #740 +/- ##
=============================================
+ Coverage 71.89% 72.30% +0.41%
=============================================
Files 320 320
Lines 69473 69473
=============================================
+ Hits 49947 50235 +288
+ Misses 19526 19238 -288
|
xdustinface
force-pushed
the
test/masternode-integration-tests
branch
from
492b26c to
a944bc7
Compare
xdustinface
force-pushed
the
feat/mn-pipeline-mode-state-machine
branch
from
4d3421b to
5bd2320
Compare
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
test/masternode-integration-tests
branch
from
a944bc7 to
3ceb7d1
Compare
github-actions
Bot
removed
the
merge-conflict
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
dash-spv/src/test_utils/node.rs (3)
67-67: 💤 Low valueAdd doc comments to new public APIs for consistency.
The new
pubitems (extra_argsfield,with_extra_args,get_new_address,get_best_block_hash,datadir,p2p_port,rpc_port) lack the rustdoc comments that all neighboring public items in this struct/impl carry. Adding a one-line///comment to each keeps the test harness API self-documenting.Also applies to: 106-109, 271-273, 459-462, 497-507
🤖 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` at line 67, Public API items in the test harness are missing rustdoc comments; add one-line doc comments (/// ...) to the new public field and methods to match surrounding documentation: place a concise doc comment above the pub extra_args field and above the pub methods/fields named with_extra_args, get_new_address, get_best_block_hash, datadir, p2p_port, and rpc_port describing their purpose/usage in one sentence each so the struct/impl remains self-documenting and consistent with neighboring items.
159-162: 💤 Low valueConsider guarding against
extra_argscolliding with already-set flags.
args_vec.extend(self.config.extra_args.iter().cloned())blindly appends extra args after baseline flags. If a caller passes e.g.-port=...,-wallet=..., or-regtest=0viawith_extra_args, dashd will see the same flag twice. Currently dashd's behavior on duplicate flags is "last wins" for most options, so this is usually fine — but it's worth documenting onwith_extra_args/extra_argsto set caller expectations, or alternatively de-duplicating on a known-conflicting-prefix basis.🤖 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 159 - 162, The code currently appends self.config.extra_args directly to args_vec which can create duplicate flags (e.g., -wallet=, -port=, -regtest=) that collide with the baseline flags set earlier; update the logic that builds args_vec (the place that currently calls args_vec.extend(self.config.extra_args.iter().cloned())) to first filter/de-duplicate extra_args against a small whitelist of known conflicting prefixes (e.g., "-wallet=", "-port=", "-regtest=", "-rpcport=", etc.) so any extra arg whose prefix matches one already set is skipped (or alternatively choose to let extra_args override by removing the baseline flag first), or document the collision behavior on the with_extra_args/extra_args API so callers know "last-wins" semantics — implement the chosen approach by modifying where args_vec is constructed and checking self.config.extra_args prior to extending.
464-468: 💤 Low value
get_block_templatesilently swallows RPC errors.The
let _: Result<Value, _> = ...pattern discards both success and failure outcomes. If the RPC fails (e.g., dashd is not ready, or returns an unexpected error), tests will silently lose the side effect this method exists to trigger, leading to confusing downstream test failures rather than a clear root-cause panic. Consider at minimum logging the error attracing::debug!/tracing::warn!so operators can correlate failures, or if the call is expected to occasionally return errors during DKG (liketry_rpc_callcovers), reuse that helper for consistency.♻️ Suggested change
- /// Call getblocktemplate to trigger CreateNewBlock (includes quorum commitments). - pub fn get_block_template(&self) { - let client = self.rpc_client(); - let _: Result<Value, _> = client.call("getblocktemplate", &[]); - } + /// Call getblocktemplate to trigger CreateNewBlock (includes quorum commitments). + /// + /// Errors are intentionally tolerated because this is invoked during DKG orchestration + /// where transient failures are expected; they are logged for diagnostic purposes. + pub fn get_block_template(&self) { + let client = self.rpc_client(); + if let Err(e) = client.call::<Value>("getblocktemplate", &[]) { + tracing::debug!("getblocktemplate returned error (ignored): {}", e); + } + }🤖 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 464 - 468, get_block_template currently discards RPC results (let _: Result<...>) so RPC failures are silently ignored; update get_block_template (and calls to self.rpc_client()) to surface failures either by reusing the existing try_rpc_call helper for consistent retry/handling or at minimum capture the Result and log errors with tracing::warn! or tracing::debug! including the error details and context ("getblocktemplate" call) so test failures show the real RPC error instead of failing silently.dash-spv/tests/dashd_sync/tests_restart.rs (1)
10-13: 💤 Low valueConsolidate duplicate
dash_spv::test_utilsimports.Lines 10 and 13 both import from the same path; merging them is a trivial readability win.
♻️ Suggested change
-use dash_spv::test_utils::SYNC_TIMEOUT; - -use super::setup::{create_and_start_client, TestContext}; -use dash_spv::test_utils::{create_test_wallet, TestChain}; +use super::setup::{create_and_start_client, TestContext}; +use dash_spv::test_utils::{create_test_wallet, TestChain, SYNC_TIMEOUT};🤖 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/tests/dashd_sync/tests_restart.rs` around lines 10 - 13, Consolidate the duplicate imports from dash_spv::test_utils by merging SYNC_TIMEOUT and create_test_wallet/TestChain into a single use statement; keep the separate use super::setup::{create_and_start_client, TestContext} as-is and replace the two dash_spv::test_utils imports with one use dash_spv::test_utils::{SYNC_TIMEOUT, create_test_wallet, TestChain} to remove redundancy and improve readability.dash-spv/src/test_utils/network.rs (1)
50-50: 💤 Low valueVerify the 100,000 broadcast capacity is necessary.
Bumping capacity by 1000× (from a previous value to
100000) reserves a ring buffer of that manyNetworkEventslots in theMockNetworkManagereven though no test currently subscribes via this channel (it's only exposed throughsubscribe_network_events). If the bump is just defensive against future lag, a lower value (e.g., 10_000, matching the new event-handler capacity) would have a closer-to-zero footprint while still preventing realistic lag. If the higher capacity is actually load-bearing for one of the new tests, a brief comment would help future readers.🤖 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/network.rs` at line 50, The broadcast capacity in MockNetworkManager's network_event_sender is set to 100_000 without justification; either reduce it to a more reasonable default (e.g., 10_000 to match the event-handler capacity) or keep 100_000 but add a concise comment explaining which test(s) require such high capacity. Locate the network_event_sender initialization in MockNetworkManager and either change the Sender::new(100000) to Sender::new(10_000) or add a one-line comment referencing subscribe_network_events and the specific test(s) that need the larger ring buffer so future readers understand the rationale.
🤖 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/masternode_network.rs`:
- Around line 1074-1085: The protx update_service call currently swallows
failures with warn! which allows bootstrap to continue; change this to fail fast
by returning or propagating the error instead: capture the Err(e) from the
client.call("protx", ...) invocation and convert it into an early return (e.g.,
return Err(e.into()) or panic! with context) so the caller sees the failure;
include mn_info.datadir and mn_info.pro_tx_hash in the error context so it's
easy to identify which masternode failed during update_service.
In `@dash-spv/tests/dashd_masternode/helpers.rs`:
- Around line 42-76: The helper wait_for_masternode_sync currently only checks
progress after progress_receiver.changed(), which misses the case where the
receiver is already at SyncState::Synced; update the function
(wait_for_masternode_sync) to first inspect the current value from
progress_receiver (use borrow() or borrow_and_update() and check
progress.masternodes() and mn_progress.state()) and return immediately if
already Synced, before entering the loop/select that awaits
progress_receiver.changed(); keep the existing timeout/select logic for future
updates.
In `@dash-spv/tests/dashd_masternode/tests_instantsend.rs`:
- Around line 114-133: After waiting for the ChainLock
(cl_height/cl_sync_height) add a final assertion loop that verifies every
tracked InstantSend transaction has transitioned to
TransactionContext::InChainLockedBlock(_). Locate where the test stores/tracks
the transactions created earlier (use the client_handle's tracked transactions
or wallet/tx list associated with client_handle), iterate those tx ids, query
their TransactionContext/state from the SPV client (via the same API used
earlier to inspect InstantSend state), and assert each context matches
TransactionContext::InChainLockedBlock(_); place this check just before calling
client_handle.stop().await so the test fails if transactions never reach the
chainlocked state.
In `@dash-spv/tests/dashd_masternode/tests_sync.rs`:
- Around line 25-26: This test suite contains network-dependent, long-running
integration tests; add the #[ignore] attribute to each such async test function
so they are not run by default—specifically add #[ignore] above
test_masternode_list_sync and likewise add #[ignore] above the other
network-dependent test functions referenced in the file (the tests reported at
the other locations) so they run only when invoked with -- --ignored; ensure the
attribute is placed immediately above the #[tokio::test] line for each affected
test.
---
Nitpick comments:
In `@dash-spv/src/test_utils/network.rs`:
- Line 50: The broadcast capacity in MockNetworkManager's network_event_sender
is set to 100_000 without justification; either reduce it to a more reasonable
default (e.g., 10_000 to match the event-handler capacity) or keep 100_000 but
add a concise comment explaining which test(s) require such high capacity.
Locate the network_event_sender initialization in MockNetworkManager and either
change the Sender::new(100000) to Sender::new(10_000) or add a one-line comment
referencing subscribe_network_events and the specific test(s) that need the
larger ring buffer so future readers understand the rationale.
In `@dash-spv/src/test_utils/node.rs`:
- Line 67: Public API items in the test harness are missing rustdoc comments;
add one-line doc comments (/// ...) to the new public field and methods to match
surrounding documentation: place a concise doc comment above the pub extra_args
field and above the pub methods/fields named with_extra_args, get_new_address,
get_best_block_hash, datadir, p2p_port, and rpc_port describing their
purpose/usage in one sentence each so the struct/impl remains self-documenting
and consistent with neighboring items.
- Around line 159-162: The code currently appends self.config.extra_args
directly to args_vec which can create duplicate flags (e.g., -wallet=, -port=,
-regtest=) that collide with the baseline flags set earlier; update the logic
that builds args_vec (the place that currently calls
args_vec.extend(self.config.extra_args.iter().cloned())) to first
filter/de-duplicate extra_args against a small whitelist of known conflicting
prefixes (e.g., "-wallet=", "-port=", "-regtest=", "-rpcport=", etc.) so any
extra arg whose prefix matches one already set is skipped (or alternatively
choose to let extra_args override by removing the baseline flag first), or
document the collision behavior on the with_extra_args/extra_args API so callers
know "last-wins" semantics — implement the chosen approach by modifying where
args_vec is constructed and checking self.config.extra_args prior to extending.
- Around line 464-468: get_block_template currently discards RPC results (let _:
Result<...>) so RPC failures are silently ignored; update get_block_template
(and calls to self.rpc_client()) to surface failures either by reusing the
existing try_rpc_call helper for consistent retry/handling or at minimum capture
the Result and log errors with tracing::warn! or tracing::debug! including the
error details and context ("getblocktemplate" call) so test failures show the
real RPC error instead of failing silently.
In `@dash-spv/tests/dashd_sync/tests_restart.rs`:
- Around line 10-13: Consolidate the duplicate imports from dash_spv::test_utils
by merging SYNC_TIMEOUT and create_test_wallet/TestChain into a single use
statement; keep the separate use super::setup::{create_and_start_client,
TestContext} as-is and replace the two dash_spv::test_utils imports with one use
dash_spv::test_utils::{SYNC_TIMEOUT, create_test_wallet, TestChain} to remove
redundancy and improve readability.
🪄 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: f3a38506-ea92-46ee-9a4d-e0c77e83bf2f
📒 Files selected for processing (18)
.github/workflows/build-and-test.ymlcontrib/setup-dashd.pydash-spv/src/test_utils/event_handler.rsdash-spv/src/test_utils/masternode_network.rsdash-spv/src/test_utils/mod.rsdash-spv/src/test_utils/network.rsdash-spv/src/test_utils/node.rsdash-spv/src/test_utils/wallet.rsdash-spv/tests/dashd_masternode/helpers.rsdash-spv/tests/dashd_masternode/main.rsdash-spv/tests/dashd_masternode/setup.rsdash-spv/tests/dashd_masternode/tests_instantsend.rsdash-spv/tests/dashd_masternode/tests_sync.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/dashd_sync/tests_mempool.rsdash-spv/tests/dashd_sync/tests_multi_wallet.rsdash-spv/tests/dashd_sync/tests_restart.rs
xdustinface
added a commit
that referenced
this pull request
May 8, 2026
…rnode_sync` `watch::Receiver::changed()` only wakes on future updates, so callers entering after masternode sync was already `Synced` would wait until timeout. Inspect the current value via `borrow_and_update()` first and return early when already synced. Addresses CodeRabbit review comment on PR #740 #740 (comment)
xdustinface
added a commit
that referenced
this pull request
May 8, 2026
…otstrap The masternode network setup depends on every node having its real P2P port registered, otherwise dashd can't form quorum connections and downstream DKG/IS sessions fail with hard-to-trace timeouts. Collect any RPC failures during the per-masternode `protx update_service` loop and panic with the affected datadirs once the loop is done so all failures are surfaced together. Addresses CodeRabbit review comment on PR #740 #740 (comment)
xdustinface
added a commit
that referenced
this pull request
May 8, 2026
…rnode_sync` `watch::Receiver::changed()` only wakes on future updates, so callers entering after masternode sync was already `Synced` would wait until timeout. Inspect the current value via `borrow_and_update()` first and return early when already synced. Addresses CodeRabbit review comment on PR #740 #740 (comment)
xdustinface
added a commit
that referenced
this pull request
May 8, 2026
…otstrap The masternode network setup depends on every node having its real P2P port registered, otherwise dashd can't form quorum connections and downstream DKG/IS sessions fail with hard-to-trace timeouts. Collect any RPC failures during the per-masternode `protx update_service` loop and panic with the affected datadirs once the loop is done so all failures are surfaced together. Addresses CodeRabbit review comment on PR #740 #740 (comment)
xdustinface
force-pushed
the
test/masternode-integration-tests
branch
from
9356563 to
d4ca37b
Compare
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
Bot
added
the
merge-conflict
Adds a `dashd_masternode` integration test suite that runs against a real dashd regtest network with multiple masternodes, plus a new `masternode_network` module in `test_utils` that manages multi-node dashd lifecycles, DKG cycles, mocktime advancement, and ChainLock signing. The new suite complements the existing `dashd_sync` tests by exercising masternode-specific behavior. `tests_sync.rs` covers masternode list sync, restart, new-block extension, quorum rotation, and a full end-to-end scenario. `tests_instantsend.rs` covers InstantSend lock formation, behavior across quorum rotation, and the case where the `islock` arrives before the transaction.
…rnode_sync` `watch::Receiver::changed()` only wakes on future updates, so callers entering after masternode sync was already `Synced` would wait until timeout. Inspect the current value via `borrow_and_update()` first and return early when already synced. Addresses CodeRabbit review comment on PR #740 #740 (comment)
…otstrap The masternode network setup depends on every node having its real P2P port registered, otherwise dashd can't form quorum connections and downstream DKG/IS sessions fail with hard-to-trace timeouts. Collect any RPC failures during the per-masternode `protx update_service` loop and panic with the affected datadirs once the loop is done so all failures are surfaced together. Addresses CodeRabbit review comment on PR #740 #740 (comment)
xdustinface
force-pushed
the
test/masternode-integration-tests
branch
from
d4ca37b to
97c1a0c
Compare
github-actions
Bot
removed
ready-for-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/dashd_masternode/tests_instantsend.rs`:
- Around line 35-36: Update the doc comment so it does not claim the test
asserts wallet state transitions to TransactionContext::InChainLockedBlock(_);
instead state that the test only mines until a ChainLock and verifies the
ChainLock sync height check (that cl_sync_height >= cl_height is satisfied) and
that wallet-level InChainLockedBlock assertions are intentionally deferred/not
implemented yet; reference the cl_sync_height and cl_height checks in the test
to make the intended behavior clear.
🪄 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: b9e409d8-a4ff-4e8c-876d-15a57f79cc24
📒 Files selected for processing (18)
.github/workflows/build-and-test.ymlcontrib/setup-dashd.pydash-spv/src/test_utils/event_handler.rsdash-spv/src/test_utils/masternode_network.rsdash-spv/src/test_utils/mod.rsdash-spv/src/test_utils/network.rsdash-spv/src/test_utils/node.rsdash-spv/src/test_utils/wallet.rsdash-spv/tests/dashd_masternode/helpers.rsdash-spv/tests/dashd_masternode/main.rsdash-spv/tests/dashd_masternode/setup.rsdash-spv/tests/dashd_masternode/tests_instantsend.rsdash-spv/tests/dashd_masternode/tests_sync.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/dashd_sync/tests_mempool.rsdash-spv/tests/dashd_sync/tests_multi_wallet.rsdash-spv/tests/dashd_sync/tests_restart.rs
✅ Files skipped from review due to trivial changes (5)
- dash-spv/src/test_utils/event_handler.rs
- .github/workflows/build-and-test.yml
- dash-spv/tests/dashd_sync/tests_basic.rs
- dash-spv/tests/dashd_sync/tests_mempool.rs
- dash-spv/tests/dashd_masternode/main.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- dash-spv/src/test_utils/network.rs
- dash-spv/src/test_utils/wallet.rs
- dash-spv/tests/dashd_sync/tests_restart.rs
- dash-spv/src/test_utils/mod.rs
- contrib/setup-dashd.py
- dash-spv/src/test_utils/node.rs
- dash-spv/tests/dashd_masternode/tests_sync.rs
- dash-spv/tests/dashd_sync/tests_multi_wallet.rs
- dash-spv/tests/dashd_masternode/setup.rs
- dash-spv/tests/dashd_masternode/helpers.rs
- dash-spv/src/test_utils/masternode_network.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
dashd_masternodeintegration test suite that runs against a real dashd regtest network with multiple masternodes, plus a newmasternode_networkmodule intest_utilsthat manages multi-node dashd lifecycles, DKG cycles, mocktime advancement, and ChainLock signing.The new suite complements the existing
dashd_synctests by exercising masternode-specific behavior.tests_sync.rscovers masternode list sync, restart, new-block extension, quorum rotation, and a full end-to-end scenario.tests_instantsend.rscovers InstantSend lock formation, behavior across quorum rotation, and the case where theislockarrives before the transaction.Summary by CodeRabbit
Chores
Tests