test: parallelize data recovery masternode restarts - #7500
Conversation
|
✅ Final review complete — no blockers (commit c46d48a) |
Changed-test CI timing comparisonTest: Develop baseline:
The changed test was faster in all five applicable CI configurations, saving 4m 17s across them in this pair of runs. These are the functional test runner's per-test durations from the CI logs, not whole-job or whole-suite runtimes. The develop TSAN execution failed once after 45s and then passed in 311s. The table uses the final successful attempt and excludes that extra failed attempt so the comparison is not inflated by a baseline retry. |
WalkthroughThe test framework now centralizes masternode restart selection, stopping, parallel startup, optional block-height waiting, synchronization completion, and controller reconnection. The LLMQ data recovery test delegates to this method and provides per-node arguments for data recovery, quorum-vector synchronization, and reindexing. Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant QuorumDataRecoveryTest
participant DashTestFramework
participant ThreadPoolExecutor
participant Masternodes
QuorumDataRecoveryTest->>DashTestFramework: restart_masternodes(...)
DashTestFramework->>Masternodes: stop selected nodes
DashTestFramework->>ThreadPoolExecutor: start selected nodes in parallel
ThreadPoolExecutor->>Masternodes: start with recovery arguments
DashTestFramework->>Masternodes: wait for block height and finish sync
DashTestFramework->>Masternodes: reconnect to controller
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/functional/test_framework/test_framework.py (1)
1838-1850: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated arg-building logic between
restart_oneandstart_masternode.The
wait_block_countbranch reconstructs the-masternodeblsprivkey=...+self.extra_args[mn.nodeIdx]+extra_argssequence inline instead of reusingstart_masternode(lines 1865-1870). Ifstart_masternode's argument construction ever changes, this inlined copy will silently drift out of sync.Consider extracting a small helper for building the arg list, shared by both
start_masternodeand this branch.♻️ Proposed refactor to share arg-building logic
+ def _masternode_start_args(self, mn: MasternodeInfo, extra_args=None): + args = ['-masternodeblsprivkey=%s' % mn.keyOperator] + self.extra_args[mn.nodeIdx] + if extra_args is not None: + args += extra_args + return args + def start_masternode(self, mninfo: MasternodeInfo, extra_args=None): - args = ['-masternodeblsprivkey=%s' % mninfo.keyOperator] + self.extra_args[mninfo.nodeIdx] - if extra_args is not None: - args += extra_args - self.start_node(mninfo.nodeIdx, extra_args=args) + self.start_node(mninfo.nodeIdx, extra_args=self._masternode_start_args(mninfo, extra_args)) force_finish_mnsync(mninfo.get_node(self))And in
restart_one:- args = ['-masternodeblsprivkey=%s' % mn.keyOperator] + self.extra_args[mn.nodeIdx] - if extra_args is not None: - args += extra_args - self.start_node(mn.nodeIdx, extra_args=args) + self.start_node(mn.nodeIdx, extra_args=self._masternode_start_args(mn, extra_args))🤖 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 `@test/functional/test_framework/test_framework.py` around lines 1838 - 1850, Extract the masternode argument construction from start_masternode into a shared helper, including the operator key, self.extra_args[mn.nodeIdx], and optional extra_args. Update both start_masternode and the wait_block_count branch of restart_one to use this helper so their startup arguments remain synchronized.
🤖 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 `@test/functional/test_framework/test_framework.py`:
- Around line 1838-1850: Extract the masternode argument construction from
start_masternode into a shared helper, including the operator key,
self.extra_args[mn.nodeIdx], and optional extra_args. Update both
start_masternode and the wait_block_count branch of restart_one to use this
helper so their startup arguments remain synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 408dfd03-63fc-411d-b572-e356ab6a2fbf
📒 Files selected for processing (2)
test/functional/feature_llmq_data_recovery.pytest/functional/test_framework/test_framework.py
Add DashTestFramework.restart_masternodes using the existing ThreadPoolExecutor start pattern: stop selected MNs first, start and force_finish_mnsync in parallel, then connect_nodes serially to the controller. Migrate feature_llmq_data_recovery.restart_mns through the helper without changing recovery, reindex, exclude, or post-restart sync semantics. Benchmark: feature_llmq_data_recovery ~38.5s / 22.7% faster (baseline 168.270/171.687s vs candidate 133.473/129.478s).
e28d85d to
c46d48a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/test_framework/test_framework.py (1)
1816-1828: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKey
excludebynodeIdxfor consistency with the dedupe set.Dedupe uses
mn.nodeIdxwhileexcluderelies on object identity (MasternodeInfodefines no__eq__). Current callers pass the same instances fromself.mninfo, so it works, but a caller that reconstructsMasternodeInfofor the same node would silently skip the exclusion.♻️ Optional: index-based exclusion
- if exclude is None: - exclude = [] + excluded_idx = set() if exclude is None else {mn.nodeIdx for mn in exclude} @@ - if mn in exclude or mn.nodeIdx in seen: + if mn.nodeIdx in excluded_idx or mn.nodeIdx in seen:🤖 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 `@test/functional/test_framework/test_framework.py` around lines 1816 - 1828, Update the selection logic around the exclude and seen sets to normalize excluded MasternodeInfo entries by nodeIdx, then compare each candidate’s mn.nodeIdx against that index set. Preserve the existing order, deduplication, and empty-selection behavior in the surrounding selection flow.
🤖 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 `@test/functional/test_framework/test_framework.py`:
- Around line 1830-1863: The reindex restart path in restart_one must pass an
explicit, reasonably bounded timeout to wait_until when waiting for
wait_block_count, rather than relying on the framework default. Preserve the
existing height predicate and force_finish_mnsync flow while choosing a duration
that accommodates slow TSAN/UBSAN runs and still fails deterministically.
---
Nitpick comments:
In `@test/functional/test_framework/test_framework.py`:
- Around line 1816-1828: Update the selection logic around the exclude and seen
sets to normalize excluded MasternodeInfo entries by nodeIdx, then compare each
candidate’s mn.nodeIdx against that index set. Preserve the existing order,
deduplication, and empty-selection behavior in the surrounding selection flow.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3c68ee8-1fed-4a5f-ba80-abc017fdf5a6
📒 Files selected for processing (2)
test/functional/feature_llmq_data_recovery.pytest/functional/test_framework/test_framework.py
| self.log.info("Restarting %d masternodes in parallel", len(selected)) | ||
|
|
||
| # Issue stop RPCs first, then wait — same ordering as stop_nodes() | ||
| for mn in selected: | ||
| self.nodes[mn.nodeIdx].stop_node(wait_until_stopped=False) | ||
| for mn in selected: | ||
| self.nodes[mn.nodeIdx].wait_until_stopped() | ||
|
|
||
| def restart_one(mn: MasternodeInfo): | ||
| extra_args = None if extra_args_cb is None else extra_args_cb(mn) | ||
| if wait_block_count is None: | ||
| self.start_masternode(mn, extra_args=extra_args) | ||
| return | ||
| # With -reindex, wait for height before force_finish_mnsync. | ||
| self.start_node( | ||
| mn.nodeIdx, | ||
| extra_args=self._masternode_start_args(mn, extra_args=extra_args), | ||
| ) | ||
| # Default-arg capture avoids late-binding of mn in the wait predicate. | ||
| self.wait_until(lambda mn=mn: mn.get_node(self).getblockcount() >= wait_block_count) | ||
| force_finish_mnsync(mn.get_node(self)) | ||
|
|
||
| executor = ThreadPoolExecutor(max_workers=20) | ||
| try: | ||
| jobs = [executor.submit(restart_one, mn) for mn in selected] | ||
| for job in jobs: | ||
| job.result() | ||
| finally: | ||
| executor.shutdown(wait=True) | ||
|
|
||
| # Connect serially to the controller only. | ||
| for mn in selected: | ||
| self.connect_nodes(mn.nodeIdx, 0) | ||
| return selected |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect prior restart/reindex wait usage and framework wait_until default
rg -nP -C4 '\bdef wait_until\s*\(' test/functional/test_framework/test_framework.py
git log -p -1 --unified=6 -- test/functional/feature_llmq_data_recovery.py | rg -nP -C4 'wait_until|getblockcount|reindex' | head -60Repository: dashpay/dash
Length of output: 659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== surrounding code =="
sed -n '1800,1875p' test/functional/test_framework/test_framework.py
echo "== timeout_factor option / parser =="
rg -n -C3 'timeout_factor|timeout' test/functional/test_framework/test_framework.py | head -120
echo "== target diff for function rename/move context =="
set +u
git diff --unified=20 -- test/functional/test_framework/test_framework.py | sed -n '1,260p'
set -uRepository: dashpay/dash
Length of output: 9642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== wait_until definition =="
sed -n '895,912p' test/functional/test_framework/test_framework.py
echo "== callers / diffs around start_masternodes/restart_masternodes and wait_block_count =="
rg -n -C4 'restart_masternodes|start_masternodes|wait_block_count|wait_until\(.*getblockcount' test/functional/test_framework/test_framework.py test/functional | head -200
echo "== feature_llmq_data_recovery relevant changes in any available diff/status =="
git diff -- test/functional/feature_llmq_data_recovery.py | sed -n '1,240p' || trueRepository: dashpay/dash
Length of output: 18924
Set an explicit timeout for the reindex height wait.
This path defaults to wait_until’s framework 60s timeout, but the current PR does not show an explicit duration, while full -reindex restarts with TSAN/UBSAN can exceed that. Use an explicit, reasonably bounded timeout here so slow sanitizer configs fail deterministically instead of flapping on the framework default.
🤖 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 `@test/functional/test_framework/test_framework.py` around lines 1830 - 1863,
The reindex restart path in restart_one must pass an explicit, reasonably
bounded timeout to wait_until when waiting for wait_block_count, rather than
relying on the framework default. Preserve the existing height predicate and
force_finish_mnsync flow while choosing a duration that accommodates slow
TSAN/UBSAN runs and still fails deterministically.
There was a problem hiding this comment.
Thanks — leaving as-is for now. This is a minor maintainability nit on a low-risk test helper; happy to clean it up in a follow-up if it becomes load-bearing.
There was a problem hiding this comment.
@thepastaclaw, understood — keeping this scoped as-is is reasonable. The current framework timeout remains a known trade-off if reindex restart duration becomes a source of sanitizer flakiness. 🐇
You are interacting with an AI system.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This is a clean, low-risk test-infrastructure refactor that adds a restart_masternodes() helper mirroring the framework's existing parallel-start / serial-stop patterns, and migrates feature_llmq_data_recovery.py to use it while preserving exclusion, reindex, and mnsync semantics. No blocking or consensus-relevant issues were found; the sole valid finding is a minor nitpick about an equality-check inconsistency between the new dedup and exclude logic, which is theoretical risk only given the single current call site.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
💬 1 nitpick(s)
| if exclude is None: | ||
| exclude = [] | ||
| # Preserve caller order but drop duplicates: some tests pass multiset | ||
| # concatenations of quorum membership lists (e.g. feature_llmq_data_recovery). | ||
| selected = [] | ||
| seen = set() | ||
| for mn in (self.mninfo if mns is None else mns): | ||
| if mn in exclude or mn.nodeIdx in seen: | ||
| continue | ||
| seen.add(mn.nodeIdx) | ||
| selected.append(mn) |
There was a problem hiding this comment.
💬 Nitpick: exclude check uses identity equality while dedup uses nodeIdx
In restart_masternodes(), the new dedup set (seen) is keyed by mn.nodeIdx, but the exclude check (mn in exclude) relies on MasternodeInfo's default identity-based __eq__ — the class has no @dataclass decorator and defines no custom __eq__. This mismatch is newly introduced by this refactor (the prior per-test serial code never needed to dedupe or exclude in one pass). It works correctly today because the only call site (feature_llmq_data_recovery.py) always passes the same MasternodeInfo instances from self.mninfo, so identity equality happens to align with node index. But if a future caller reconstructs an equivalent MasternodeInfo for the same node index and passes it via exclude, the exclusion would silently fail while dedup-by-index would still succeed — a confusing footgun for anyone who copies this pattern. Low risk given the single current call site, but keying both checks on nodeIdx removes the inconsistency for free.
| if exclude is None: | |
| exclude = [] | |
| # Preserve caller order but drop duplicates: some tests pass multiset | |
| # concatenations of quorum membership lists (e.g. feature_llmq_data_recovery). | |
| selected = [] | |
| seen = set() | |
| for mn in (self.mninfo if mns is None else mns): | |
| if mn in exclude or mn.nodeIdx in seen: | |
| continue | |
| seen.add(mn.nodeIdx) | |
| selected.append(mn) | |
| excluded_idx = set() if exclude is None else {mn.nodeIdx for mn in exclude} | |
| selected = [] | |
| seen = set() | |
| for mn in (self.mninfo if mns is None else mns): | |
| if mn.nodeIdx in excluded_idx or mn.nodeIdx in seen: | |
| continue | |
| seen.add(mn.nodeIdx) | |
| selected.append(mn) | |
source: ['claude']
1 participant
Issue being fixed or feature implemented
feature_llmq_data_recovery.pyrepeatedly restarts several masternodes serially. Process startup, reindexing, and mnsync waits dominate this restart-heavy functional test, especially under sanitizers.What was done?
Add a focused
DashTestFramework.restart_masternodes()helper and migrate only the data-recovery test to it.The helper:
ThreadPoolExecutorpattern fromstart_masternodes();connect_nodes()calls;No polling changes or other functional-test callsites are included.
How Has This Been Tested?
python3 -m py_compile test/functional/test_framework/test_framework.py test/functional/feature_llmq_data_recovery.pytest/lint/lint-python.pygit diff --check upstream/develop...HEADdashdbuild, alternating baseline and candidate with unique datadirs and port seeds; all runs passedupstream/developImprovement: 38.503 seconds (22.7%).
Breaking Changes
None.
Checklist: