test: segregate wallet-mode coverage and deduplicate functional test runs - #7497
Conversation
… paths Dash-specific RPCs touch the wallet in a small number of places: gobject prepare funds the proposal collateral from the wallet, gobject vote-many/vote-alias look up voting keys via CheckWalletOwnsKey/IsMine and sign via CWallet::SignGovernanceVote -> SignMessage (SPKM dispatch), and the protx register/update family funds, signs and submits special transactions through the wallet, including the external-collateral register_prepare + signmessage + register_submit path. Cover exactly this surface in a dedicated test that runs in both --legacy-wallet and --descriptors modes using a small 2-masternode topology with no quorums and no superblock cycles (~13s per mode locally). This will allow the consensus-heavy tests that currently run twice only to keep a single wallet mode without losing wallet coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feature_governance.py and feature_governance_cl.py were run in both --legacy-wallet and --descriptors modes because they were the regression tests for descriptor-wallet governance vote signing (dash#6003, dash#6094). Their coverage is almost entirely consensus-side and wallet-agnostic (superblock budgets, trigger creation and voting, ChainLock interaction); the wallet-mode-divergent surface (gobject prepare collateral funding, vote-many/vote-alias key lookup and vote signing) is now covered in both wallet modes by wallet_dash_rpcs.py, so run the governance tests in descriptors mode only. Saves roughly 110 seconds of serial functional-test time per CI job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feature_dip3_deterministicmns.py was run in both wallet modes for the same historical reason as the governance tests: it was part of the descriptor-wallet support regression suite (dash#6003, dash#6094). The test exercises consensus-side deterministic masternode list behavior (registration heights, collateral spends, reorgs, payment enforcement) which is wallet-mode-agnostic. The wallet-facing protx paths it goes through (register_fund, register, update_service, update_registrar and external-collateral signing) are covered in both wallet modes by wallet_dash_rpcs.py, so run it in descriptors mode only. Saves roughly 100 seconds of serial functional-test time per CI job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feature_llmq_signing.py ran twice, once with spork21 off and once with --spork21, duplicating the whole signing-session flow (quorum mining, sign/verify RPCs, recovered-sig expiration) which does not depend on the spork. Run the shared flow once with spork21 off, then enable SPORK_21_QUORUM_ALL_CONNECTED mid-test, mine one more quorum and run the spork21-only sections on it: all-connected topology and symmetric QSENDRECSIGS checks, the submit=false RPC parameter with QSIGSHARE P2P share submission to the recovery member, and the recovery-member isolation scenario. mine_quorum() reads spork state at call time, so expected connection counts adjust automatically. The only permutation lost is spork21 being active from the very first DKG, which is already covered by feature_llmq_connections.py (it mines quorums right after activating spork21 on a fresh quorum-less chain and verifies member connections and probes). Saves roughly 60 seconds of serial functional-test time per CI job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…riant feature_llmq_simplepose.py runs twice, with spork23 enabled and disabled. The first section (test_banning via isolate_mn) exercises the DKG contribution-miss ban path (MarkBadMember -> PoSePunish), which is not gated on spork23: spork23 only gates VerifyConnectionAndMinProtoVersions and connection probes. Running that section in the --disable-spork23 variant repeated identical coverage, so skip it there together with its repair_masternodes call. Instead, mine a single quorum in normal conditions so that the spork23-specific sections below (no banning for unreachable or outdated nodes when PoSe checks are off) start from the same state as in the spork23-enabled run: an existing quorum and all masternodes healthy. Those sections are otherwise unchanged and the skipped section does not feed any state into them: masternodes are unbanned either way once repair_masternodes has run and expected contributor counts are derived from mninfo at call time. Also move the --disable-spork23 entry further down the runner list to match its reduced runtime. Saves roughly 20-30 seconds of serial functional-test time per CI job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit 850aeba) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95996baf4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # mined after that. spork21 being active from the very first DKG is | ||
| # covered by feature_llmq_connections.py. |
There was a problem hiding this comment.
Keep a test where SPORK_21 is active before the first DKG: the cited replacement does not cover that scenario, because feature_llmq_connections.py mines three llmq_test quorums at lines 36, 58, and 70 before enabling SPORK_21 at lines 74-76, then proceeds with a rotating quorum. Consequently, regressions specific to initializing or forming the first quorum with the spork already active are no longer detected after removing the --spork21 variant.
AGENTS.md reference: AGENTS.md:L164-L164
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right that feature_llmq_connections.py was the wrong citation — it mines three llmq_test quorums (lines 36, 58, 70) before enabling SPORK_21 at line 75, so it does not cover "spork21 active before the first DKG" for that quorum type. The PR description has been corrected.
However, the permutation itself is still covered in every CI job: feature_llmq_data_recovery.py enables SPORK_21_QUORUM_ALL_CONNECTED at the very top of run_test (line 164), on a fresh chain before its first mine_quorum() (line 173), and then forms both llmq_test and llmq_test_v17 quorums every DKG cycle with the spork active from before the first DKG ever. That exercises first-quorum formation under all-connected topology at least as thoroughly as the removed --spork21 variant did (two quorum types concurrently vs one). Additionally, feature_llmq_connections.py does cover the chain's first rotation (dip0024) DKGs under spork21 via mine_cycle_quorum() after the spork is enabled.
The only delta vs the removed variant is quorum size (5 members there vs 4/3 in data_recovery); the spork21 path (EnsureQuorumConnections all-connected + probes) has no first-DKG-specific branching that depends on member count, so we are not restoring the variant.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR updates functional coverage for LLMQ signing and recovery, Simple PoSe behavior under SPORK23 modes, and wallet-dependent governance and protx RPC paths. It adds Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletDashRPCsTest
participant WalletRPC
participant Blockchain
participant ProtxState
WalletDashRPCsTest->>WalletRPC: prepare and sign governance or protx operation
WalletRPC->>Blockchain: submit transaction
Blockchain->>WalletDashRPCsTest: confirm mined transaction
WalletDashRPCsTest->>ProtxState: query resulting state
ProtxState->>WalletDashRPCsTest: return updated state
Possibly related PRs
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The test-only refactor preserves the intended wallet-mode, SPORK21, and SPORK23 coverage; in particular, first-DKG SPORK21 coverage remains in feature_llmq_data_recovery.py. One documentation-only issue remains: the signing test comment and corresponding commit message cite feature_llmq_connections.py instead of the test that actually provides that coverage.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); claude/general=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
💬 1 nitpick(s)
| # mined after that. spork21 being active from the very first DKG is | ||
| # covered by feature_llmq_connections.py. |
There was a problem hiding this comment.
💬 Nitpick: Correct the stale first-DKG coverage citation
feature_llmq_connections.py mines three llmq_test quorums at lines 36, 58, and 70 before enabling SPORK_21_QUORUM_ALL_CONNECTED at lines 74-76, so it does not cover SPORK21 being active for the first DKG. The actual replacement coverage is in feature_llmq_data_recovery.py, which enables SPORK21 at line 164 before its first mine_quorum() at line 173. Update this comment and amend the matching rationale in commit 436bcfaf339 so the source and commit history accurately explain why removing the original permutation is safe; the functional coverage itself remains intact.
| # mined after that. spork21 being active from the very first DKG is | |
| # covered by feature_llmq_connections.py. | |
| # mined after that. spork21 being active from the very first DKG is | |
| # covered by feature_llmq_data_recovery.py. |
source: ['codex']
There was a problem hiding this comment.
Resolved in 850aeba — Correct the stale first-DKG coverage citation no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Affected-test CI timing comparisonDevelop baseline: The table sums only the runner entries affected by this PR:
Affected-entry serial time was lower in all five applicable CI configurations. Breakdown across those five configurations:
These are the functional test runner's per-test durations from the CI logs. They are serial sums, not elapsed job time; the test runner executes entries in parallel. The tables use final successful test attempts and exclude retry overhead: the baseline linux64 simplepose variant and PR SQLite simplepose variant each failed once before passing. Full functional-test step comparisonThis table uses the successful TSAN rerun instead of the earlier failed TSAN attempt. The other five configurations are unchanged from the original run.
Across all six configurations, the complete |
- feature_llmq_signing.py: the comment justifying the removal of the --spork21 variant named feature_llmq_connections.py, which mines three quorums before activating spork21 and therefore does not cover the first-DKG case. feature_llmq_data_recovery.py does, so name that one. - feature_llmq_signing.py: the intra-quorum connections here predate the spork21 activation, unlike on a chain that runs with it from the first DKG, so bump mocktime while waiting for the all-connected topology to settle instead of polling a fixed connection count without bumping. - wallet_dash_rpcs.py: cover "protx register" too. It proves collateral ownership via CWallet::SignMessage (rpc/evo.cpp), a wallet-mode-divergent path that feature_dip3_deterministicmns.py exercised in legacy mode before this series dropped its --legacy-wallet run. - wallet_dash_rpcs.py: assert on the per-masternode result of vote-alias and vote-many instead of inferring the rate-limited re-vote from the aggregate counts alone. - wallet_dash_rpcs.py: drop pubKeyOperator="" and rewards_address="" from the update_registrar call. Both are falsy and fall back to the masternode's existing values, so they read as "clear this field" while meaning "keep it", making the payoutAddress assertion trivially true. - wallet_dash_rpcs.py: pass the registered masternode between the protx subtests explicitly rather than via an implicit self.fund_mn attribute. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
pls see 850aeba |
|
Thanks — I reviewed and validated |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 850aeba56f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not self.options.disable_spork23: | ||
| # Lets isolate MNs one by one and verify that punishment/banning happens | ||
| self.test_banning(self.isolate_mn, 2) |
There was a problem hiding this comment.
When SPORK_23_QUORUM_POSE is disabled, this branch now skips the only contribution-miss banning check, so the suite no longer proves that ordinary DKG misses remain punishable despite the spork being off. The enabled run cannot catch a regression that accidentally gates MarkBadMember/PoSePunish on SPORK_23, while the remaining disabled-spork cases assert the opposite behavior for connection and protocol failures; retain at least a small disabled-spork contribution-miss case.
AGENTS.md reference: AGENTS.md:L164-L175
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The latest wallet and LLMQ test changes are functionally correct, with no blocking defects confirmed. Carried-forward prior findings: one remains valid only in commit history because the source citation was fixed but commit 436bcfa still contains the stale rationale. New latest-delta findings: one commit-hygiene suggestion remains because 850aeba combines fixes for separate earlier commits.
Source: reviewers codex/general=gpt-5.6-sol, codex/dash-core-commit-history=gpt-5.6-sol, claude/general=claude-sonnet-5, claude/dash-core-commit-history=claude-sonnet-5; verifier=codex/final-verifier=gpt-5.6-sol (fallback_for_sonnet_verifier=true); coordinator=openclaw-agent (orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:850aeba56fd>`:
- [SUGGESTION] <commit:850aeba56fd>:1: Squash the review-feedback commit into the changes it repairs
Commit 850aeba56fd combines corrections to two separate changes already introduced in this unmerged stack. Its feature_llmq_signing.py hunks repair 436bcfaf339, while its wallet_dash_rpcs.py hunks repair and complete the test introduced by b052e852af0, including the direct `protx register` coverage that 698079525d7 already cites as replacement coverage. This leaves intermediate commits with incomplete or inaccurate rationales and creates a generic cross-area review-fix commit instead of preserving atomic history. Fold the signing hunks into 436bcfaf339 and the wallet hunks into b052e852af0, preserving the relevant authorship and co-author trailers.
3 participants
Issue being fixed or feature implemented
Functional tests account for roughly 79 minutes of serial runtime per CI job, and several whole tests are run twice with configurations whose divergent surface is only a small fraction of the test. The duplicated runs cost roughly 450 seconds of serial time per job:
feature_governance.py,feature_governance_cl.pyandfeature_dip3_deterministicmns.pyrun in both--legacy-walletand--descriptorsmodes. This dates back to the descriptor-wallet support series (feat: support rpc protx-register for descriptor wallets - part VI #6003, feat: support descriptor wallets for RPC governance votemany, votealias #6094): these tests were the regression tests for descriptor vote signing and protx funding, so both modes were wired up (c72ec70 addedfeature_governance.py --descriptorsin the same commit that implemented descriptor vote signing). The tests themselves are overwhelmingly consensus-side (superblock budgets, trigger creation, DMN list updates, reorgs, payment enforcement) and wallet-mode-agnostic; only the RPC entry points that fund, look up keys and sign through the wallet differ per wallet mode.feature_llmq_signing.pyruns twice (default and--spork21), duplicating the entire signing-session flow which does not depend on the spork.feature_llmq_simplepose.py --disable-spork23re-runs the contribution-miss banning section even though that ban path is not gated on spork23.What was done?
Wallet coverage is segregated from consensus coverage, and per-test duplication is removed where the second run added no coverage:
wallet_dash_rpcs.py, run in both--legacy-walletand--descriptorsmodes (~11-15s per mode). It covers exactly the wallet-mode-divergent Dash RPC surface:gobject prepare(wallet collateral funding) ->list-prepared->submit;gobject vote-many/vote-alias(CheckWalletOwnsKey/IsMinelookup andCWallet::SignGovernanceVote->SignMessageSPKM dispatch);protx register_fund; the external-collateral pathprotx register_prepare+signmessage+register_submit;protx update_service;protx update_registrar. Small 2-masternode topology, no quorums, no superblock cycles.feature_governance.pyandfeature_governance_cl.pyrun in--descriptorsmode only. The legacy-wallet coverage of the governance wallet surface is carried bywallet_dash_rpcs.py.feature_dip3_deterministicmns.pyruns in--descriptorsmode only, same justification for the protx wallet surface.feature_llmq_signing.pymerged into a single run. The shared flow runs with spork21 off; SPORK_21_QUORUM_ALL_CONNECTED is then enabled mid-test, one more quorum is mined, and the spork21-only sections run against it: all-connected topology plus symmetric QSENDRECSIGS checks, thesubmit=falseRPC parameter with QSIGSHARE P2P share submission to the recovery member, and the recovery-member isolation scenario.mine_quorum()reads spork state at call time, so expected connection counts adjust automatically.feature_llmq_simplepose.py --disable-spork23skips the contribution-miss banning section (MarkBadMember->PoSePunishis not spork23-gated; spork23 only gatesVerifyConnectionAndMinProtoVersionsand probes, so that section was identical in both variants). A single quorum is mined in normal conditions instead so the spork23-specific sections start from the same state as in the spork23-enabled run.Deliberately lost permutations, called out explicitly:
feature_llmq_signing.py --spork21): covered byfeature_llmq_data_recovery.py, which enables SPORK_21_QUORUM_ALL_CONNECTED at the top ofrun_teston a fresh chain, before the first DKG, and then forms bothllmq_testandllmq_test_v17quorums every cycle under it. (feature_llmq_connections.pyadditionally covers the chain's first rotation (dip0024) DKGs under spork21, though it mines three non-rotation quorums before enabling the spork.) The one delta is quorum size: the removed variant used 5-member quorums whilefeature_llmq_data_recovery.pyuses 4- and 3-member ones; the spork21 connection path has no first-DKG-specific branching that depends on member count.wallet_dash_rpcs.py.Changed entries were re-slotted in
BASE_SCRIPTSaccording to their new runtimes.How Has This Been Tested?
Local machine (Apple Silicon, 14 cores), all timings from
test/functional/test_runner.py -j3runs before and after the changes:feature_governance.py --legacy-walletfeature_governance.py --descriptorsfeature_governance_cl.py --legacy-walletfeature_governance_cl.py --descriptorsfeature_dip3_deterministicmns.py --legacy-walletfeature_dip3_deterministicmns.py --descriptorsfeature_llmq_signing.pyfeature_llmq_signing.py --spork21feature_llmq_simplepose.py --disable-spork23wallet_dash_rpcs.py --legacy-walletwallet_dash_rpcs.py --descriptorsThat is a ~270 s serial saving per CI job on this machine; CI runners are slower, so the absolute saving there is expected to be larger (~380 s estimated from recent CI run timings).
Additional verification:
wallet_dash_rpcs.pypasses in both wallet modes.feature_llmq_signing.pypasses standalone and under parallel load.feature_llmq_simplepose.pyvariants pass; the--disable-spork23variant was additionally stress-tested with multiple concurrent instances.test/lint/all-lint.pypasses for the touched files (the only failure is pre-existinglint-cppcheck-dashwarnings in unrelated C++ files untouched by this PR).Breaking Changes
None. Test-only changes.
Checklist:
🤖 Generated with Claude Code