After add_outbound_p2p_connection, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier.
However, this is untested, and the missing test coverage leads to bugs being missed. For example #30394#pullrequestreview-2166824948
Fix it by adding a test.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
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 adding a bug. For example:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index d674758abd..e6f741a408 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -5328,7 +5328,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
5
6 // For outbound connections, ensure that the initial VERSION message
7 // has been sent first before processing any incoming messages
8- if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
9+ // BUG!!!!!!!!!!!!!!!!!!! if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
10
11 {
12 LOCK(peer->m_getdata_requests_mutex);
13@@ -5823,6 +5823,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
14
15 // Initiate version handshake for outbound connections
16 if (!pto->IsInboundConn() && !peer->m_outbound_version_message_sent) {
17+ UninterruptibleSleep(200ms);
18 PushNodeVersion(*pto, *peer);
19 peer->m_outbound_version_message_sent = true;
20 }
113@@ -106,5 +114,19 @@ def run_test(self):
114 # Feeler connections do not request tx relay
115 assert_equal(feeler_conn.last_message["version"].relay, 0)
116
117+ self.log.info("Send version message early to node")
118+ # Normally the test framework would be shy and send the version message
119+ # only after it received one. See the on_version method. Check that
120+ # bitcoind behaves properly when a version is sent unexpectedly (but
121+ # toleratably) early.
0 # tolerably) early.
ACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15
I verified that the normal behaviour is sending a version message only in reply to a received version. In this test, I verified that it sends a version message before, and since it already sent it, when it receives a version message from node, it skips replying to version with own version message (on_connection_send_msg = None).
ACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15 Nice addition. Test causes framework to be “non-shy” (Wireshark view below)
tACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15
Thanks for following up! Tested this with the following patch, where the introduced debug messages would pop up (with received message type “version” as expected) in the log every few runs:
0diff --git a/src/net.h b/src/net.h
1index a4e4b50360..8b9b0003b8 100644
2--- a/src/net.h
3+++ b/src/net.h
4@@ -747,6 +747,13 @@ public:
5 std::optional<std::pair<CNetMessage, bool>> PollMessage()
6 EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex);
7
8+ /** TEST-ONLY: Peek at the next message type from the processing queue of this connection. */
9+ std::optional<std::string> PeekMessageType() EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex) {
10+ LOCK(m_msg_process_queue_mutex);
11+ if (m_msg_process_queue.empty()) return std::nullopt;
12+ return m_msg_process_queue.front().m_type;
13+ }
14+
15 /** Account for the total size of a sent message in the per msg type connection stats. */
16 void AccountForSentBytes(const std::string& msg_type, size_t sent_bytes)
17 EXCLUSIVE_LOCKS_REQUIRED(cs_vSend)
18diff --git a/src/net_processing.cpp b/src/net_processing.cpp
19index d674758abd..107bbdda0e 100644
20--- a/src/net_processing.cpp
21+++ b/src/net_processing.cpp
22@@ -5328,7 +5328,13 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
23
24 // For outbound connections, ensure that the initial VERSION message
25 // has been sent first before processing any incoming messages
26- if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
27+ if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) {
28+ LogPrintf("don't process messages yet, no VERSION message sent out yet.\n");
29+ if (auto message_type = pfrom->PeekMessageType()) {
30+ LogPrintf("ATTENTION: received message type \"%s\" early.\n", *message_type);
31+ }
32+ return false;
33+ }
34
35 {
36 LOCK(peer->m_getdata_requests_mutex);
I think thematically the test would also fit very well into p2p_handshake.py, but no strong feelings.
I think thematically the test would also fit very well into
p2p_handshake.py, but no strong feelings.
Sure, I may move-only it, if I have to re-touch for other reasons.
