The test may fail intermittently when the net thread is lagging while calling DeleteNode. This may result in a split getdata, meaning that peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)]) fails.
Fix it by adding a sync on the net thread.
Fixes #31700
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32092.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | mzumsande |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Can be tested by the diff provided by @tnndbtc in #32063 (comment) (thanks!):
diff --git a/src/net.cpp b/src/net.cpp
index 735985a841..2bb6db0cf7 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1965,6 +1965,9 @@ void CConnman::DisconnectNodes()
// Destroy the object only after other threads have stopped using it.
if (pnode->GetRefCount() <= 0) {
m_nodes_disconnected.remove(pnode);
+
+ std::this_thread::sleep_for(std::chrono::seconds(1)); // SLEEP for 1 second to 100% reproduce the issue
+
DeleteNode(pnode);
}
}
788 | @@ -789,6 +789,7 @@ def test_parents_change(self): 789 | # Disconnect peer1. peer2 should become the new candidate for orphan resolution. 790 | peer1.peer_disconnect() 791 | self.wait_until(lambda: node.num_test_p2p_connections() == 1) 792 | + peer2.sync_with_ping() # Sync with the 'net' thread which completes the disconnection fully 793 | node.bumpmocktime(TXREQUEST_TIME_SKIP)
it would be good to also add a comment in disconnect_p2ps that the wait there does not guarantee that the resources of all nodes (such as outstanding txrequests) are cleared.
thx, done
Code Review ACK fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b
