test: use cheaper quorum setup in governance ChainLocks test - #7498
Conversation
|
✅ Final review complete — no blockers (commit de9df58) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
|
The TSAN failure is unrelated pre-existing flakiness in #7498 changes only 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. |
|
CI note: |
Changed-test CI timing comparisonTest: Develop baseline:
The changed test was faster in all five applicable CI configurations. 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 The PR's overall CI run failed in TSAN, but |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe governance functional test now calls Estimated code review effort: 1 (Trivial) | ~2 minutes 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 |
| self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0) | ||
| self.wait_for_sporks_same() | ||
| self.mine_cycle_quorum() | ||
| # ChainLocks use llmq_test on regtest; a rotation (DIP0024) cycle is unnecessary here. |
There was a problem hiding this comment.
nit: We do send txes though (via "gobject prepare") so mine_quorum() is enough here only because of the bump_mocktime(60 * 10 + 1) below which makes collateral txes "safe". Might want to adjust this comment.
There was a problem hiding this comment.
Good catch, thanks. Updated in de9df589df5 to note that the later 10-minute mocktime bump is what keeps the proposal collateral transactions safe with the shorter mine_quorum() setup.
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.
5cdc3b2 to
de9df58
Compare
![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: de9df589df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.mine_cycle_quorum() | ||
| # ChainLocks use llmq_test on regtest, and the 10-minute mocktime bump below keeps | ||
| # proposal collateral transactions safe with this shorter quorum setup. | ||
| self.mine_quorum() |
There was a problem hiding this comment.
With this swap, the test starts the governance flow much earlier: from the framework's post-setup height 8, mine_quorum() leaves the chain around height 43, so the proposal cycle starts at 60 and the exercised superblock is 80, while set_dash_test_params() still activates v20/mn_rr at height 100. The previous mine_cycle_quorum() setup crossed height 100 before proposals, so this now stops covering the post-v20 ChainLocks/governance path that this regression test used to exercise; mine just enough extra blocks or explicitly activate v20 before preparing the proposals.
AGENTS.md reference: AGENTS.md:L165-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is wrong. No non-coinbase txes can be made before maturity height (100) and we do send txes here. Pls re-verify your claims.
3 participants
Issue being fixed or feature implemented
feature_governance_cl.pyonly needs a signing-capable ChainLocks quorum before exercising governance behavior. It currently builds a full DIP0024 rotation cycle even though regtest ChainLocks usellmq_test.What was done?
Replace
mine_cycle_quorum()withmine_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.pygit diff --check upstream/develop...HEADdashdbuild, alternating baseline and candidate with unique datadirs and port seeds; all runs passedupstream/developMedian improvement: 6.105 seconds (18.6%).
Breaking Changes
None.
Checklist: