Skip to content

test: use cheaper quorum setup in governance ChainLocks test - #7498

Open
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:perf/feature-governance-cl-quorum
Open

test: use cheaper quorum setup in governance ChainLocks test#7498
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:perf/feature-governance-cl-quorum

Conversation

@thepastaclaw

Copy link
Copy Markdown

Issue being fixed or feature implemented

feature_governance_cl.py only needs a signing-capable ChainLocks quorum before exercising governance behavior. It currently builds a full DIP0024 rotation cycle even though regtest ChainLocks use llmq_test.

What was done?

Replace mine_cycle_quorum() with mine_quorum() and document why rotation setup is unnecessary. The existing ChainLock wait still proves that the quorum is ready before the governance assertions begin.

How Has This Been Tested?

  • python3 -m py_compile test/functional/feature_governance_cl.py
  • git diff --check upstream/develop...HEAD
  • Exact-head code review: ship, zero findings
  • Six balanced A/B functional runs using the same local dashd build, alternating baseline and candidate with unique datadirs and port seeds; all runs passed
Variant Runs (seconds) Median
upstream/develop 36.570, 32.828, 32.536 32.828s
This PR 30.440, 25.688, 26.723 26.723s

Median improvement: 6.105 seconds (18.6%).

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

feature_governance_cl only needs a signing-capable ChainLocks quorum. On
regtest ChainLocks use llmq_test (LLMQ_TEST), not the DIP0024 rotation type
that mine_cycle_quorum() builds. Prefer the cheaper mine_quorum() path while
preserving the same CL activation and governance-skip coverage.
@thepastaclaw

thepastaclaw commented Jul 30, 2026

Copy link
Copy Markdown
Author

✅ Final review complete — no blockers (commit 5cdc3b2)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Codex + Sonnet

This is a minimal, correct test-only change swapping mine_cycle_quorum() for the cheaper mine_quorum() in feature_governance_cl.py. I confirmed regtest's llmqTypeChainLocks is LLMQ_TEST (chainparams.cpp:926) matching mine_quorum()'s default llmq_type_name="llmq_test"/llmq_type=100, and that the function mines through signing maturity by default, so the subsequent all-node ChainLock wait still genuinely proves readiness. All four reviewer lanes (Codex general/commit-history, Claude/Sonnet-strict general/commit-history) independently found zero issues, and there is no prior review history or CodeRabbit content to reconcile.

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(failed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); 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: claude-sonnet-5 — final-verifier
  • Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — general (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (completed)

Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.

@thepastaclaw

Copy link
Copy Markdown
Author

The TSAN failure is unrelated pre-existing flakiness in feature_protx_version.py, so this PR is proper as-is and the branch has not been changed.

#7498 changes only test/functional/feature_governance_cl.py. The failing test exhausted all three retries and ultimately timed out at feature_protx_version.py:241, waiting 240 seconds for a revoked masternode's getconnectioncount() to reach zero. The immediately preceding develop base SHA (2e52d33fa3bde2e4bf70152df8e126f0db11d989) failed its own TSAN run with the exact same test, line, predicate, and timeout: https://github.com/dashpay/dash/actions/runs/30483914944/job/90688072625

Tracked as the renamed test's latest occurrence in #6702. Maintainers can rerun CI if desired; no commit, push, rebase, or retrigger was made here.

@thepastaclaw

Copy link
Copy Markdown
Author

CI note: linux64_tsan-test failed on an unrelated pre-existing flake in feature_protx_version.py (test_revoke_protx waiting for getconnectioncount() == 0 timed out after 240s). This PR only changes feature_governance_cl.py quorum setup and does not touch ProTx revoke/connection drain paths. I do not have rights to rerun failed jobs here; a maintainer rerun of the failed TSAN job should be enough if this remains the only red check.

@thepastaclaw

thepastaclaw commented Jul 30, 2026

Copy link
Copy Markdown
Author

Changed-test CI timing comparison

Test: feature_governance_cl.py

Develop baseline: d41edd90 (successful develop CI push run 30402917810; exact base 2e52d33f run 30483914944 failed)
PR run: 5cdc3b29 (run 30515379393)

CI configuration develop baseline PR delta change
linux64 53s 50s -3s -5.7%
linux64_ubsan 54s 50s -4s -7.4%
linux64_tsan 104s 100s -4s -3.8%
linux64_sqlite 32s 28s -4s -12.5%
linux64_multiprocess 86s 78s -8s -9.3%
Total across applicable configurations 329s 306s -23s -7.0%

The changed test was faster in all five applicable CI configurations. linux64_nowallet is excluded because the test is skipped when wallet support is not compiled.

These are the functional test runner's per-test durations from the CI logs, not whole-job or whole-suite runtimes. For configurations that run both wallet variants, the --legacy-wallet and --descriptors durations are summed; SQLite runs descriptors only.

The PR's overall CI run failed in TSAN, but feature_governance_cl.py passed in both wallet variants there; that job failure occurred elsewhere and does not affect these test timings.

@PastaPastaPasta
PastaPastaPasta marked this pull request as ready for review July 30, 2026 16:05
@PastaPastaPasta
PastaPastaPasta requested review from UdjinM6 and knst July 30, 2026 16:05
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 59e8ff9c-204c-4267-93ae-95d3a7219f52

📥 Commits

Reviewing files that changed from the base of the PR and between 2e52d33 and 5cdc3b2.

📒 Files selected for processing (1)
  • test/functional/feature_governance_cl.py

Walkthrough

The governance functional test now calls mine_quorum() instead of mine_cycle_quorum() when activating ChainLocks. The accompanying comment states that DIP0024 rotation cycles are unnecessary on regtest because ChainLocks use llmq_test.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: pastapastapasta

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: switching governance ChainLocks test setup to a cheaper quorum path.
Description check ✅ Passed The description matches the changeset and explains the quorum setup simplification and its test impact.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Fix failing CI checks
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant