fix: intermittent failure in feature_dip3_v19.py - #5863
Merged
Conversation
knst
approved these changes
Feb 8, 2024
knst
left a comment
Collaborator
There was a problem hiding this comment.
LGTM
Though, I can't reproduce failure on feature_dip3_v19.py whatever with or without your patch, with -j20 or with -j4. That successes 100% on my localhost
PastaPastaPasta
approved these changes
Feb 9, 2024
PastaPastaPasta
left a comment
Member
There was a problem hiding this comment.
utACK for squash merge
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 2, 2026
…-key change/revoke dc27835 test: don't expect zero connections after protx operator-key change/revoke (pasta) Pull request description: ## Issue being fixed or feature implemented Fixes the currently worst-offending functional-test flake, #6702 (`feature_protx_version.py`, formerly `feature_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: ```text self.wait_until(lambda: self.nodes[node_idx].getconnectioncount() == 0) AssertionError: ... not true after 240.0 seconds ``` 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: 1. Changing a masternode's operator key (or revoking it) makes every peer that verified its MNAUTH drop the connection once it processes the trigger block (`CMNAuth::NotifyMasternodeListChanged`). This part works: in every captured failure, all pre-existing peers of the affected node are disconnected within ~1 second. 2. The masternode remains a member of previously formed quorums (membership is snapshotted at DKG time), so `CConnman::ThreadOpenMasternodeConnections` on *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), where `connect_nodes` raced the disconnect storm — it traded one race for another. The other `getconnectioncount() == 0` waits in the tree (`feature_llmq_signing.py`, `feature_llmq_simplepose.py`) are not affected: they call `setnetworkactive(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 `a61ade2ef84` with 15 concurrent copies of the test: ```bash python3 test/functional/test_runner.py --jobs=15 --attempts=1 $(for _ in $(seq 15); do echo feature_protx_version.py; done) ``` - Before the change: 6/15, 5/15 and 9/15 runs failed across three batches — every single failure at the line 202/241 zero-wait (no unrelated contention failures). - After the change: 45/45 runs passed (three batches of 15). - Re-checked out the pre-fix state afterwards and re-ran one batch: 9/15 failures returned, all matching the signature. - The 60 s local timeout vs. 240 s in CI is `TEST_RUNNER_TIMEOUT_FACTOR=4` (`ci/test/00_setup_env.sh`); same predicate, same bug. - Sequential run of the fixed test passes in ~150 s, matching CI's passing runtime. ## Breaking Changes n/a ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 7c740ddeb2541741cf858cfd0094809ea64ddca4f150bb2f08a75fc59df475c56f20eb039649d508d3d86517bb7c2834c8cafe927e7c6cd4b669c5f9874bdff5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Fix failures like https://gitlab.com/dashpay/dash/-/jobs/6120923632
What was done?
Handle disconnects and reconnection of the revoked MN in the right place.
How Has This Been Tested?
Run multiple
feature_dip3_v19.pyin parallel a few timesBreaking Changes
n/a
Checklist: