Repository navigation
test: deduplicate the mnlistdiff P2P helper and drop dead helpers in Dash functional tests - #7877
Conversation
feature_dip4_coinbasemerkleroots, feature_llmq_evo, feature_llmq_rotation and feature_protx_version each defined the same TestP2PConn class (one copy carried a TODO asking for a new home). Move it to test_framework/p2p.py as P2PMnListDiffStore, next to the other P2PInterface subclasses. wait_for_mnlistdiff() was only called from getmnlistdiff(), so it is inlined there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- feature_mnehf: ensure_tx_is_not_mined() is never called, and no caller of send_tx() passes expected_error or reason, so its error-handling branch is dead. - interface_zmq_dash: generate_blocks() is never called. - DashTestFramework: dynamically_start_masternode() is never called. - Drop three commented-out code lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Final review complete — no blockers (commit 3d945ca) · triage: low |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughFunctional tests now use a shared Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main changes: deduplicating the masternode-list diff P2P helper and removing unused helpers from functional tests. Description check The description directly explains the helper deduplication, dead-code removal, affected files, testing performed, and lack of breaking changes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The exact reviewed range is a clean, behavior-preserving test-only cleanup. The shared P2PMnListDiffStore preserves the prior mnlistdiff request, state-reset, response, and timeout behavior, and the removed helpers have no remaining callers; no actionable in-scope defects were found. Lint and ClangFormat CI checks passed, while six platform cache checks were queued in the supplied snapshot; this static review did not run builds or tests.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — This is a small, contained functional-test refactor that deduplicates helpers and removes confirmed dead code without changing production behavior or critical surfaces. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 97% left, weekly 95% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
thepastaclaw
removed
the
pastaclaw:approved
2 participants
Issue being fixed or feature implemented
Several Dash functional tests carry copy-pasted or dead helper code. One copy of the mnlistdiff P2P helper even has a
TODO: this helper used in many tests, find a new home for it.What was done?
feature_dip4_coinbasemerkleroots,feature_llmq_evo,feature_llmq_rotationandfeature_protx_versioneach defined the sameTestP2PConnclass. It now lives once intest_framework/p2p.pyasP2PMnListDiffStore, next to the otherP2PInterfacesubclasses.wait_for_mnlistdiff()was only called fromgetmnlistdiff(), so it is inlined.feature_mnehf:ensure_tx_is_not_mined()is never called, and no caller ofsend_tx()passesexpected_error/reason, so its error-handling branch was dead.send_tx()is now a one-liner.interface_zmq_dash:generate_blocks()is never called.DashTestFramework.dynamically_start_masternode()is never called.Net: -105 lines.
How Has This Been Tested?
Ran every touched test locally on Linux x86_64 (debug build):
feature_dip4_coinbasemerkleroots,feature_llmq_evo,feature_llmq_rotation,feature_protx_version,feature_mnehf,feature_governance,feature_llmq_chainlocksandinterface_zmq_dashall pass. On a heavily loaded box,interface_zmq_dashtimed out the first two times inmine_cycle_quorum(generatetoaddresstook >30s) and passed on the next run. The change to that file only removes an unused method and a comment.lint-python.pyreports nothing new.Breaking Changes
None.
Checklist:
🤖 Generated with Claude Code