test: Fix all races after a socket is closed gracefully #34425

Currently, functional tests may intermittently fail, due to a lack of synchronization after a node connection socket was closed gracefully: If node A is connected to node B, and node B closes the connection, node A must wait for the connection to be closed before continuing the test. Otherwise, subsequent re-connections may not work while a stale connection is still alive.

This can be reproduced locally via something like:

diff --git a/src/net.cpp b/src/net.cpp
index 6b79e913e8..32bd061500 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2200,2 +2200,3 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
                 }
+            UninterruptibleSleep(599ms);
                 pnode->CloseSocketDisconnect();

With this diff, the tests should fail on master, and pass after this fix.

The fix is placed inside the connect_nodes helper.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34425.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

ACK fa652a5d63d9e1855e73fa72d86c0b54f67aa0c7

Maybe some of these commits can be squashed?

Sure, squashed all down into one commit. It touches a few files, but 16 lines modified should be trivial enough for a single commit.

Concept ACK, but I think len(getpeerinfo()) is kind of a brittle way / hacky way to enforce that some node is or isn't connected to another node.

What do you think of adding a TestNode method that checks if a node is connected to another node by looking for the subver in getpeerinfo(), ie:

class TestNode():
    def is_connected_to(self, other):
        assert isinstance(other, TestNode)
        other_subver = other.getnetworkinfo()['subversion']

        return any(peer["subver"] == other_subver for peer in self.getpeerinfo())

And then doing:

- self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)
+ self.wait_until(lambda: not self.nodes[0].is_connected_to(self.nodes[1]))

alternatively, could do it using the with style

class TestNode(): [@contextlib](/bitcoin-bitcoin/contributor/contextlib/).contextmanager
    def wait_for_disconnected_from(self, other, timeout=5):
        """
        Wait until this node is disconnected from other.
        """
        yield
        self.wait_until(lambda: not self.is_connected_to(other), timeout=timeout)

and then doing:

with self.nodes[0].wait_for_disconnected_from(self.nodes[1]):
    self.restart_node(1, extra_args=[f"-mocktime={old_time+4}"])

Ah nice. I pushed the is_connected_to helper, because it makes the code more self-documenting and easier to understand.

Not sure about the contextmanager, because connections/disconnections are often not the primary target of the test and using contexts for all of them could be confusing, so I'll leave this as-is for now.

crACK fad3df9af6d787f66d3150882ed1

Nice, closes a source of intermittent test failures.

Onboard with the idea but feel that these calls are easy to miss while adding or updating functional tests. disconnect_nodes(self, a, b) does some checking like this internally and given that most of these occurrences are "non disconnect_nodes" cases where one node is restarted, can we consider adding this check at the start of connect_nodes(self, a, b, ...) itself so that we take the onus away from the developer?

An example that I have tested:

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 1f95756445..291b5ec197 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -569,6 +569,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         """
         from_connection = self.nodes[a]
         to_connection = self.nodes[b]
+        from_connection.wait_until(lambda: not from_connection.is_connected_to(to_connection))
         ip_port = "127.0.0.1:" + str(p2p_port(b))
 
         if peer_advertises_v2 is None:
diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py
index ca8393044c..62ad88d2d7 100755
--- a/test/functional/wallet_hd.py
+++ b/test/functional/wallet_hd.py
@@ -86,7 +86,6 @@ class WalletHDTest(BitcoinTestFramework):
             assert_equal(hd_info_2["hdkeypath"], "m/84h/1h/0h/0/" + str(i))
             assert_equal(hd_info_2["hdmasterfingerprint"], hd_fingerprint)
         assert_equal(hd_add, hd_add_2)
-        self.wait_until(lambda: not self.nodes[0].is_connected_to(self.nodes[1]))
         self.connect_nodes(0, 1)
         self.sync_all()
 
@@ -103,7 +102,6 @@ class WalletHDTest(BitcoinTestFramework):
             self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename
         )
         self.start_node(1, extra_args=self.extra_args[1])
-        self.wait_until(lambda: not self.nodes[0].is_connected_to(self.nodes[1]))
         self.connect_nodes(0, 1)
         self.sync_all()
         # Wallet automatically scans blocks older than key on startup
(END)

The changes here lgtm, but I'm also curious if this could be programmed a bit more defensively, as suggested by rkrux here.

so that we take the onus away from the developer?

Yeah, that sounds great. If the heavy lifting isn't done by the test framework, then it will just be annoying and tedious for developers to figure it out by themselves.

Pushed your suggestion, but I had to rework it, because the tests allow two-way connections.

ACK fae807ed25561bab7148c5a0d7bd847314c79d88

Thanks for incorporating the suggestion.

Edit: The last sentence in the description can be updated.

Edit: The last sentence in the description can be updated.

Thx, done

ACK fae807ed25561bab7148c5a0d7bd847314c79d88

(re-ish) crACK fae807ed25561bab7148c5a0d7bd847314c79d88

I re-reviewed the changes here entirely since I recall little since my last review, I think this is a good change that will make tests less brittle, and I like the changes suggested by @rkrux that have been implemented.

ACK fae807ed25561bab7148c5a0d7bd847314c79d88

Backported to 31.x in #35046.