Fixes #25031
The feature_index_prune.py test seems to be racy because connections are reestablished after restarts and the blocks are synced via the sync_blocks function. The sync_blocks function has a sanity check at the beginning to check that all nodes in the set have at least one established connection and that is not always the case.
As a solution nodes are not connected via the -connect parameter on start but instead via the connect_nodes helper.
Concept ACK
Is there an explanation why this race is only (or at least, much more likely, it seems) happening on Windows? If someone knows how to reproduce the fixed failure behaviour on a Linux/BSD system, would be nice to know.
Concept ACK
Is there an explanation why this race is only (or at least, much more likely, it seems) happening on Windows? If someone knows how to reproduce the fixed failure behaviour on a Linux/BSD system, would be nice to know.
Agree, it would be nice to be sure :) My assumption is that the Windows build is just slower than everything else and that’s why we have only seen it there so far. Looking at https://cirrus-ci.com/github/bitcoin/bitcoin it always seems the be the job that takes the longest time.
why should addconnection be less racy than connect?
Why not use connect_nodes which has the wait_until built into? #25031 (comment)
-connect thread?
I was able to reproduce the issue as well with this, thanks @MarcoFalke !
why should addconnection be less racy than connect?
From my understanding of the code addconnection only returns after the connection has been established (or failed). It doesn’t seem to use threading and since it’s only available for testing I thought something like this should be it’s primary use-case. I confirmed it works with your hint mentioned above.
Why not use connect_nodes which has the wait_until built into? #25031 (comment)
Right, I can replace addconnection here with connect_nodes, I didn’t remember connect_nodes has the wait built into it. It looks a little less efficient (calling more RPCs under the hood) but saves us another line for the ip+port in the test, so I am indifferent between the two. I just didn’t like the one-liner in that link because it’s harder to read and would require a comment to explain that this is a clunky way to protect against a potential race condition.
Nodes are restarted and reconnected as part of the test. Afterwards
`sync_blocks` is called immediately on the nodes. `sync_blocks`
first checks that all the included nodes have at least one
connection. Since adding a connection is usually happening in a
thread, sometimes nodes could run into this check before the
connection was fully established so that it would fail the entire
test.
This fix uses the `connect_nodes` helper to make the connection the
nodes. `connect_nodes` has a wait for the connection built into it.
connect_nodes helper instead of addconnection RPC.
addconnection is somehow better than the stuff in connect_nodes, it could make sense to use addconnection in the connect_nodes impl?
If
addconnectionis somehow better than the stuff inconnect_nodes, it could make sense to useaddconnectionin theconnect_nodesimpl?
Yeah, I was thinking about that as well. I will give it a try and benchmark.
Labels
Tests