test: reduce nodes in LLMQ connections test - #7499
Conversation
|
✅ Final review complete — no blockers (commit ccdc763) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR makes a single, well-justified one-line change reducing feature_llmq_connections.py from 15/14 to 11/10 nodes/masternodes, with commit message reasoning that checks out against the quorum sizing math (llmq_test size 5, two llmq_test_dip0024 quorums at size 4 with signingActiveQuorumCount=2 needing 8 concurrent members, leaving 2 spares out of 10 masternodes). Both verified findings are legitimate but non-blocking observations about shrinking safety margins in two probabilistic assertions (the aggregate connection-count bound and the rotation-churn detection loop); neither breaks correctness, and the second is meaningfully mitigated by the existing precedent of feature_llmq_rotation.py running with even fewer spares (8 MNs, 0 spare). No blocking issues found; recommend COMMENT-level scrutiny on CI stability post-merge.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); 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 (completed),claude-sonnet-5— dash-core-commit-history (failed),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 `test/functional/feature_llmq_connections.py`:
- [SUGGESTION] test/functional/feature_llmq_connections.py:44: Aggregate connection-count margin shrinks from 25 to 5 with 10 masternodes
`get_mn_connection_count` counts verified MN-to-MN peer connections (any masternode, not just quorum members), so the worst-case full-mesh value for the 5 inspected quorum members is `5 * (num_masternodes - 1)`. With the old 14-masternode topology that ceiling was 5*13=65 against a threshold of 40 — a 25-unit detection margin for catching accidental all-to-all connectivity before SPORK_21 is enabled. With the new 10-masternode topology the ceiling drops to 5*9=45, leaving only a 5-unit margin above the same `< 40` threshold. The assertion still technically holds (45 > 40), but a partial-connectivity bug that previously landed safely in the 41-64 range now has much less room to be distinguished from normal test noise, and legitimate variance creeping into the high 30s/low 40s becomes more likely to produce flaky failures. Tightening the threshold to scale with the new node count (e.g. `< 25`) would restore the original detection margin.
Changed-test CI timing comparisonTest: Develop baseline:
The changed test was faster in four of five applicable CI configurations. The 58-second UBSAN regression is an outlier that nearly cancels the aggregate reduction in this single pair of CI runs. These are the functional test runner's per-test durations from the CI logs, not whole-job or whole-suite runtimes. The develop SQLite execution failed once after 131s and then passed in 111s. The table uses the final successful attempt and excludes that extra failed attempt so the comparison is not inflated by a baseline retry. |
Cut the functional test from 15 nodes/14 masternodes to 11 nodes/10 masternodes to lower process overhead while preserving coverage: - llmq_test remains size 5 / threshold 3, so probe counts (==4), wait_for_mnauth(4), and reconnect expectations stay logically intact. - Tighten the pre-SPORK aggregate verified-MN connection bound from 40 to 25 so accidental broad connectivity remains distinguishable with the smaller masternode set. - DIP0024 (llmq_test_dip0024 size 4, signingActiveQuorumCount 2) needs 8 concurrent members for two cycle quorums; 10 MNs leave 2 non-members for inter-quorum watch-connection coverage, matching the stronger known-good floor used by feature_llmq_rotation (8 MNs).
f994728 to
ccdc763
Compare
|
Addressed the aggregate connection-count margin finding in amended commit Validation: |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The delta at ccdc763 tightens the pre-SPORK aggregate verified-MN connection assertion from < 40 to < 25 in the same commit that reduces feature_llmq_connections.py to 11 nodes/10 masternodes. This restores a ~20-connection detection margin against the 45-connection all-to-all worst case, resolving the prior aggregate-margin finding. The other carried-forward prior finding (reduced DIP0024 rotation spare pool) is refuted by first-principles inspection of NetQuorum::CheckQuorumConnections and feature_llmq_rotation.py's own precedent of two fully disjoint size-4 quorums covering 8 total masternodes with zero spares, so 10 masternodes (2 spares) is strictly more slack than an already-proven-good configuration. No in-scope blocking or suggestion-level issues remain at the exact head.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); 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 (completed),claude-sonnet-5— dash-core-commit-history (completed)
Note: GitHub does not allow PastaClaw to approve or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
Issue being fixed or feature implemented
feature_llmq_connections.pystarts 14 masternodes although its quorum and non-member coverage only requires 10. The additional processes increase setup, synchronization, connection, and teardown time without strengthening the assertions.What was done?
Reduce the topology from 15 nodes / 14 masternodes to 11 nodes / 10 masternodes. No assertion or quorum parameter changes are included.
Coverage remains intact:
< 40connection bound still detects all-to-all connectivity, whose maximum with 10 masternodes is 45.How Has This Been Tested?
python3 -m py_compile test/functional/feature_llmq_connections.pygit diff --check upstream/develop...HEADdashdbuild, alternating baseline and candidate with unique datadirs and port seeds; all runs passedupstream/developMedian improvement: 12.769 seconds (13.1%).
Breaking Changes
None.
Checklist: