test: Fix intermittent issue in p2p_orphan_handling.py #36617

Turns out #32092 was incomplete and the issue can still happen:

https://github.com/bitcoin/bitcoin/actions/runs/39809026827/job/119929911875#step:14:14454

The basic idea is that peer1 disconnects to clear its orphan state. This happens in DisconnectNodes->DeleteNode->FinalizeNode->TxDownloadManager::DisconnectedPeer.

In my prior fix, I assumed that DisconnectNodes in the net thread would be possible to sync on via a ping-pong. However, the msghand thread may hold a reference to the node, preventing deletion and only moving the node to m_nodes_disconnected. Afterward, the net thread may hand the pings to the msghand thread and go to sleep for a bit.

Thus, the test proceed and cause a split getdata, meaning that peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)]) fails.

Fix this by just waiting for TxDownloadManager::DisconnectedPeer to have happened via self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) == 1).

Edit: Possible diff to reproduce with ~10% failure rate:

diff --git a/src/net.cpp b/src/net.cpp
index 1201d3bcb4..e81623484b 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2229,2 +2229,3 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
                     WakeMessageHandler();
+            UninterruptibleSleep(333ms *m_nodes_disconnected.size());
                 }
@@ -3261,2 +3262,3 @@ void CConnman::ThreadMessageHandler()
             }
+            UninterruptibleSleep(33ms);
         }

Cmd: ./bld-cmake/test/functional/test_runner.py $(for i in {1..20}; do echo -n "p2p_orphan_handling.py ";done) -j $( nproc )

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36617
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36617.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36617#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy 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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

crACK faf819bf23e8752644695e332cc00413d94af938

nit: the comment on peer2.sync_with_ping() ("Sync with the 'net' thread which completes the disconnection fully") is no longer correct given this PR update, since the ping doesn't guarantee peer1's deferred deletion has happened. The comment could be changed to something like "Sync with the 'net' thread and confirm peer2's messages have been processed".

https://github.com/bitcoin/bitcoin/blob/faf819bf23e8752644695e332cc00413d94af938/test/functional/p2p_orphan_handling.py#L829

nit: the comment on peer2.sync_with_ping() ("Sync with the 'net' thread which completes the disconnection fully") is no longer correct given this PR update, since the ping doesn't guarantee peer1's deferred deletion has happened. The comment could be changed to something like "Sync with the 'net' thread and confirm peer2's messages have been processed".

Can you please leave code review threads in the code tab. Otherwise, it is hard to follow the threads. Also, this comment doesn't seem right? self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) == 1) establishes that DisconnectedPeer has been (or is being) called. Thus, the next ping will ensure the disconnect is fully handled and the code and comment is correct?

Can you please leave code review threads in the code tab. Otherwise, it is hard to follow the threads

Sure, I’ll leave code review threads in the Code tab going forward. On the nit, I interpreted the comment as referring to sync_with_ping() synchronizing with the net thread after DisconnectedPeer has been invoked, rather than implying that the ping itself guarantees that the deferred deletion has already happened. Since the preceding wait_until(...) establishes that DisconnectedPeer has been called, the subsequent ping does ensure that the disconnect is fully handled, so I think the existing comment is still accurate.

Also when writing comments in discussions, you can not use LLMs, per https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md