test: Non-Shy version sender #30453

maflcko · 2024-07-16T11:40:57Z

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 (review)

Fix it by adding a test.

DrahtBot · 2024-07-16T11:41:00Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	brunoerg, tdb3, theStack, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko · 2024-07-16T11:41:36Z

Can be tested by adding a bug. For example:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d674758abd..e6f741a408 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5328,7 +5328,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
 
     // For outbound connections, ensure that the initial VERSION message
     // has been sent first before processing any incoming messages
-    if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
+    // BUG!!!!!!!!!!!!!!!!!!! if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
 
     {
         LOCK(peer->m_getdata_requests_mutex);
@@ -5823,6 +5823,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
 
     // Initiate version handshake for outbound connections
     if (!pto->IsInboundConn() && !peer->m_outbound_version_message_sent) {
+        UninterruptibleSleep(200ms);
         PushNodeVersion(*pto, *peer);
         peer->m_outbound_version_message_sent = true;
     }

maflcko · 2024-07-16T18:12:19Z

Disabled v2 for now, to be left as a follow-up.

test/functional/p2p_add_connections.py

brunoerg

ACK faed5d3

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).

tdb3

ACK faed5d3
Nice addition.
Test causes framework to be "non-shy" (Wireshark view below)

theStack

tACK faed5d3

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:

Patch

diff --git a/src/net.h b/src/net.h
index a4e4b50360..8b9b0003b8 100644
--- a/src/net.h
+++ b/src/net.h
@@ -747,6 +747,13 @@ public:
     std::optional<std::pair<CNetMessage, bool>> PollMessage()
         EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex);
 
+    /** TEST-ONLY: Peek at the next message type from the processing queue of this connection. */
+    std::optional<std::string> PeekMessageType() EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex) {
+        LOCK(m_msg_process_queue_mutex);
+        if (m_msg_process_queue.empty()) return std::nullopt;
+        return m_msg_process_queue.front().m_type;
+    }
+
     /** Account for the total size of a sent message in the per msg type connection stats. */
     void AccountForSentBytes(const std::string& msg_type, size_t sent_bytes)
         EXCLUSIVE_LOCKS_REQUIRED(cs_vSend)
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d674758abd..107bbdda0e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5328,7 +5328,13 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
 
     // For outbound connections, ensure that the initial VERSION message
     // has been sent first before processing any incoming messages
-    if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
+    if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) {
+        LogPrintf("don't process messages yet, no VERSION message sent out yet.\n");
+        if (auto message_type = pfrom->PeekMessageType()) {
+            LogPrintf("ATTENTION: received message type \"%s\" early.\n", *message_type);
+        }
+        return false;
+    }
 
     {
         LOCK(peer->m_getdata_requests_mutex);

I think thematically the test would also fit very well into p2p_handshake.py, but no strong feelings.

maflcko · 2024-07-18T14:10:14Z

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.

glozow · 2024-07-18T15:57:01Z

ACK fa0e227

glozow

ACK faed5d3

faed5d3 test: Non-Shy version sender (MarcoFalke) Pull request description: 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 bitcoin#30394 (review) Fix it by adding a test. ACKs for top commit: brunoerg: ACK faed5d3 tdb3: ACK faed5d3 theStack: tACK faed5d3 glozow: ACK faed5d3 Tree-SHA512: dbf527a39c932e994a1e8248ba78058000811a4bf69275278f1fd1e545716ac4d2d3be5dcf362976bbafa2a49f91d13e3601daf71d29e9c556179b01af62c03c

1e04c56 Merge bitcoin#30556: doc: multisig-tutorial: remove obsolete mention and link to closed PR (merge-script) fd43510 Merge bitcoin#30453: test: Non-Shy version sender (glozow) e3c3a11 Merge bitcoin#30327: build: Drop redundant `sys/sysctl.h` header check (merge-script) 808a77d Merge bitcoin#30156: fuzz: More accurate coverage reports (merge-script) 3ca42ba Merge bitcoin#28874: doc: fixup help output for -upnp and -natpmp (merge-script) ea32090 Merge bitcoin#28461: build: Windows SSP roundup (fanquake) e71c422 Merge bitcoin#28151: build: use `-muse-unaligned-vector-move` for Windows builds (fanquake) 077bbb4 Merge bitcoin#28131: test: Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree (fanquake) de5a2d1 Merge bitcoin#27940: test: Add implicit-signed-integer-truncation:*/include/c++/ suppression (fanquake) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## How Has This Been Tested? Built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1e04c56 knst: utACK 1e04c56 Tree-SHA512: 5e9a3fc4ac2ea06e8da48952bbdb43e7ed0c3d9ab3fdae3d8753bbe10b957c6cbb06e01b9860db4cd5ade91c8cd419dbbc8ee76073d00b4d6ff0f6ae6a4cbfd2

DrahtBot added the Tests label Jul 16, 2024

maflcko mentioned this pull request Jul 16, 2024

net: fix race condition in self-connect detection #30394

Merged

maflcko marked this pull request as draft July 16, 2024 11:54

maflcko marked this pull request as ready for review July 16, 2024 12:26

maflcko marked this pull request as draft July 16, 2024 13:57

maflcko force-pushed the 2407-test-ver branch from fa30247 to 41220d9 Compare July 16, 2024 14:55

DrahtBot added the CI failed label Jul 16, 2024

maflcko force-pushed the 2407-test-ver branch from 41220d9 to fa0e227 Compare July 16, 2024 18:05

maflcko marked this pull request as ready for review July 16, 2024 18:11

DrahtBot removed the CI failed label Jul 16, 2024

brunoerg reviewed Jul 17, 2024

View reviewed changes

test/functional/p2p_add_connections.py Outdated Show resolved Hide resolved

test: Non-Shy version sender

faed5d3

maflcko force-pushed the 2407-test-ver branch from fa0e227 to faed5d3 Compare July 17, 2024 09:49

brunoerg approved these changes Jul 17, 2024

View reviewed changes

tdb3 approved these changes Jul 18, 2024

View reviewed changes

theStack approved these changes Jul 18, 2024

View reviewed changes

DrahtBot requested a review from glozow July 18, 2024 15:57

glozow reviewed Jul 18, 2024

View reviewed changes

glozow merged commit 20ccb30 into bitcoin:master Jul 18, 2024

maflcko deleted the 2407-test-ver branch July 19, 2024 08:44

bitcoin locked and limited conversation to collaborators Jul 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Non-Shy version sender #30453

test: Non-Shy version sender #30453

Uh oh!

maflcko commented Jul 16, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Jul 16, 2024 •

edited

Loading

Uh oh!

maflcko commented Jul 16, 2024

Uh oh!

maflcko commented Jul 16, 2024

Uh oh!

Uh oh!

brunoerg left a comment

Uh oh!

tdb3 left a comment •

edited

Loading

Uh oh!

theStack left a comment •

edited

Loading

Uh oh!

maflcko commented Jul 18, 2024

Uh oh!

glozow commented Jul 18, 2024

Uh oh!

glozow left a comment

Uh oh!

Uh oh!

test: Non-Shy version sender #30453

test: Non-Shy version sender #30453

Uh oh!

Conversation

maflcko commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

maflcko commented Jul 16, 2024

Uh oh!

maflcko commented Jul 16, 2024

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 18, 2024

Uh oh!

glozow commented Jul 18, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jul 16, 2024 •

edited

Loading

DrahtBot commented Jul 16, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading

theStack left a comment •

edited

Loading