test: don't expect zero connections after protx operator-key change/revoke - #7506
Conversation
|
✅ Final review complete — no blockers (commit dc27835) |
|
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 ProTx version tests now capture existing peer IDs before migration and revocation changes. They wait for those peers to disconnect instead of waiting for zero total connections. The tests then reconnect and synchronize the masternode. Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/feature_protx_version.py`:
- Around line 61-68: Update wait_for_peers_disconnected so it validates the
initial peer snapshot before polling: obtain the current peer IDs and fail if
the target node has no peers, then continue checking that every requested peer
ID disappears. Preserve the existing wait_until behavior for non-empty
snapshots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8942155b-2ddb-4b73-8353-a5a6948da9f5
📒 Files selected for processing (1)
test/functional/feature_protx_version.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The test-only change correctly replaces the racy zero-connection assertion with tracking of the exact peer IDs present before each operator-key change or revocation. Peer IDs are monotonically assigned for each node process, so the new predicate confirms that all pre-existing connections were dropped while allowing legitimate concurrent quorum reconnections; no in-scope issues were found.
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)
|
pls see d93818b |
|
Thanks — the vacuous-pass concern is valid. I applied the exact follow-up, squashed it into the original PR commit, and preserved attribution in |
…evoke feature_protx_version.py intermittently timed out (dash#6702) waiting for getconnectioncount() == 0 on the masternode whose operator key was changed or revoked. The disconnects it waits for do happen: every peer that verified the masternode's MNAUTH drops the connection once it processes the trigger block (CMNAuth::NotifyMasternodeListChanged). But the masternode remains a member of previously formed quorums, so ThreadOpenMasternodeConnections on both sides concurrently re-establishes intra-quorum connections. Under parallel-test load these replacements land before the teardown finishes and the connection count never observably reaches zero, so the wait can only pass by timing luck. Instead of expecting an instantaneous zero, snapshot the node's peer ids right before mining the trigger transaction and wait until none of them remain: peer ids are monotonic, so this deterministically detects that the key change/revocation dropped every pre-existing connection while tolerating concurrent reconnects. The snapshot is asserted non-empty, since waiting for an empty set to disappear would succeed immediately and the test would silently stop asserting anything. The wait is expressed as set disjointness and the "why not zero connections" rationale lives in one place instead of being repeated at both call sites. Reproduced with 15 parallel copies via test_runner.py at a61ade2: 6/15 + 5/15 + 9/15 runs failed before the change (every failure at the line 202/241 waits), 45/45 passed after it. Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
5e17e12 to
dc27835
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried-forward prior findings: none — the previous final review at 5e17e12 had zero findings and nothing remains unresolved from it. New latest-delta findings: none. The delta (5e17e12..dc27835) is a single, purely mechanical amendment to the already-approved wait_for_peers_disconnected helper in feature_protx_version.py: it replaces an equivalent all() loop with peer_ids.isdisjoint(...), adds a defensive assert peer_ids guard against a trivially-true wait on an empty snapshot, and consolidates the rationale comment into a docstring instead of duplicating it at both call sites. Verified directly against the diff: both call sites still pass non-empty sets from get_peer_ids, the new predicate is behaviorally identical to the old one, and no logic, correctness, or coverage issues are introduced.
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)
3 participants
Issue being fixed or feature implemented
Fixes the currently worst-offending functional-test flake, #6702 (
feature_protx_version.py, formerlyfeature_dip3_v19.py). The two most recent CI occurrences (2026-07-30, one under TSAN and one under multiprocess, both exhausting all three retries) time out at the same predicate:at line 202 (operator-key change / v1→v3 migration path) and line 241 (
test_revoke_protx).The root cause is a race in the test, not an implementation bug. Two correct behaviors compose into a predicate that is not guaranteed to ever be observable:
CMNAuth::NotifyMasternodeListChanged). This part works: in every captured failure, all pre-existing peers of the affected node are disconnected within ~1 second.CConnman::ThreadOpenMasternodeConnectionson both sides legitimately re-establishes intra-quorum connections on its 100 ms–1 s retry loop, and the affected node also keeps accepting inbound quorum connections. These replacement connections are not dropped again, because the node stops sending MNAUTH once its configured operator key no longer matches the list.Under load the replacement connections land before the teardown finishes, so
getconnectioncount()never transiently reaches zero and the wait can only pass by timing luck. The zero-wait was itself introduced as the fix for the previous incarnation of this flake (#5863), whereconnect_nodesraced the disconnect storm — it traded one race for another.The other
getconnectioncount() == 0waits in the tree (feature_llmq_signing.py,feature_llmq_simplepose.py) are not affected: they callsetnetworkactive(false)first, which also blocks reconnections.What was done?
Test-only change. Instead of expecting an instantaneous zero, the test now snapshots the affected node's peer ids immediately before mining the trigger transaction and waits until none of those ids remain in
getpeerinfo(). Peer ids are monotonically increasing per node, so this deterministically detects that the key change/revocation dropped every pre-existing connection while tolerating concurrent (legitimate) quorum reconnections. Both call sites use a shared helper.The alternative of quiescing the network via
setnetworkactive(false)was deliberately not used: it would drop all connections regardless of whether the ban-disconnect logic works, gutting the behavioral assertion the wait provides.How Has This Been Tested?
Reproduced and verified on macOS (M-series, 14 cores) at develop
a61ade2ef84with 15 concurrent copies of the test:python3 test/functional/test_runner.py --jobs=15 --attempts=1 $(for _ in $(seq 15); do echo feature_protx_version.py; done)TEST_RUNNER_TIMEOUT_FACTOR=4(ci/test/00_setup_env.sh); same predicate, same bug.Breaking Changes
n/a
Checklist:
🤖 Generated with Claude Code