test: p2p: check disconnect due to lack of desirable service flags #29279

theStack · 2024-01-19T02:37:51Z

This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:

bitcoin/src/net_processing.cpp

Lines 3384 to 3389 in 5f3a057

    
           if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices)) 
        
           { 
        
               LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices)); 
        
               pfrom.fDisconnect = true; 
        
               return; 
        
           }

This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see CNode::ExpectServicesFromConn(...)). Feeler connections always disconnect, which is also tested here.

In lack of finding a proper file where this test would fit in, I created a new one. Happy to take suggestions there.

DrahtBot · 2024-01-19T02:37:54Z

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	davidgumberg, fjahr, cbergqvist, stratospher, itornaza
Concept ACK	furszy, BrandonOdiwuor
Stale ACK	delta1, epiccurious, marcofleon, mzumsande

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

Conflicts

No conflicts as of last run.

delta1

Code Review ACK 358561e

Adds a new test, increases code coverage.

furszy

See #28170. Obvious concept ACK.
We do have a post-IBD bug on this process.

BrandonOdiwuor

Concept ACK 358561e

Increasing code coverage

stratospher

Concept ACK! i think flaky tests are possible because disconnection could happen before p2p_conn.wait_for_connect() in add_outbound_connection()?

test/functional/p2p_handshake.py

theStack · 2024-01-25T17:52:32Z

Concept ACK! i think flaky tests are possible because disconnection could happen before p2p_conn.wait_for_connect() in add_outbound_connection()?

True, good catch. The flaky case can be reproduced by adding an artificial delay before the wait_for_connect() call, e.g.:

diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 77f6e69e98..4aa54fa64a 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -725,6 +725,7 @@ class TestNode():
             p2p_conn.wait_until(lambda: p2p_conn.message_count["version"] == 1, check_connected=False)
             p2p_conn.wait_until(lambda: not p2p_conn.is_connected, check_connected=False)
         else:
+            time.sleep(1)
             p2p_conn.wait_for_connect()
             self.p2ps.append(p2p_conn)

I guess one solution could be to add a send_version parameter to add_outbound_p2p_connection (like add_p2p_connection already has) to prevent the immediate sending of the version message. We could set that to false in the test and send the version message triggering the disconnect manually. Will set the PR to draft state until I have something working.

theStack · 2024-01-26T02:53:16Z

Solved the flaky test issue by introducing a wait_for_disconnect parameter to add_p2p_outbound_connection, which hits the same code path as for feeler connections if set (i.e. waiting for immediate disconnect).

stratospher

tested ACK ff5f994.

test/functional/p2p_handshake.py

theStack · 2024-01-29T22:59:15Z

Rebased on master (after the merge of #24748 🎉 ), and tackled #29279 (comment). Note that I tried enabling v2 transport for this test, but it seems that it is non-trivial and needs some deeper changes in the test framework regarding the order of version messages sent (see slightly related comment https://mirror.b10c.me/bitcoin-bitcoin/24748/#discussion_r1465420112). Will tackle that in a follow-up.

stratospher · 2024-01-30T03:56:43Z

ACK d82eafb.

Adds a new boolean parameter `wait_for_disconnect` to the `add_outbound_p2p_connection` method. If set, the node under test is checked to disconnect immediately after receiving the version message (same logic as for feeler connections).

…depth) This adds coverage for the logic introduced in PR bitcoin#28170 ("p2p: adaptive connections services flags").

theStack · 2024-03-11T14:42:16Z

Force-pushed with rebase on master and tackled comment #29279 (comment).

I reworked the test_desirable_service_flags method in the last commit by introducing a new desirable_service_flags parameter that has to be passed. Also, there are now two constants DESIRABLE_SERVICE_FLAGS_FULL and DESIRABLE_SERVICE_FLAGS_PRUNED with a comment that the latter is dynamic. Hope that this all is not considered too confusing. Re-review would be much appreciated!

davidgumberg · 2024-03-12T03:14:04Z

reACK 2f23987

Looks good, retested by modifying HasAllDesirableServiceFlags and ExpectServicesFromConn

fjahr

re-utACK 2f23987

(CI failure can be ignored)

fjahr · 2024-03-12T19:06:13Z

test/functional/p2p_handshake.py

+        self.test_desirable_service_flags(node, [NODE_NETWORK_LIMITED | NODE_WITNESS],
+                                          DESIRABLE_SERVICE_FLAGS_FULL, expect_disconnect=True)
+        node.setmocktime(int(time.time()) + 23 * 3600)  # tip inside the 24h window, should succeed
+        self.test_desirable_service_flags(node, [NODE_NETWORK_LIMITED | NODE_WITNESS],


nit: The DESIRABLE_SERVICE_FLAGS_* above are the same as the combinations passed as service_flag_tests so that variable could have been used in both places IMO (with a slight rename maybe). But this isn't critical.

cbergqvist

re ACK 2f23987

Built & re-ran test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp.

stratospher

tested ACK 2f23987. 🚀

itornaza · 2024-03-18T20:03:56Z

tested ACK 2f23987

Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
Run unit tests with make check and all tests pass
Run all functional tests with no extra flags and all tests pass
Run all functional tests with --extended and all tests pass

Notes:

First time I run the tests for a PR so I am verbose to avoid any pitfalls
Tests run on macOS 14.3.1 (23D60)

DrahtBot added the Tests label Jan 19, 2024

theStack force-pushed the 202401-test-check_disconnect_lack_desirable_flags branch from 0f7b25f to 358561e Compare January 19, 2024 02:42

DrahtBot added CI failed and removed CI failed labels Jan 19, 2024

delta1 approved these changes Jan 19, 2024

View reviewed changes

furszy reviewed Jan 19, 2024

View reviewed changes

BrandonOdiwuor reviewed Jan 19, 2024

View reviewed changes

stratospher reviewed Jan 25, 2024

View reviewed changes

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

theStack marked this pull request as draft January 25, 2024 17:52

theStack force-pushed the 202401-test-check_disconnect_lack_desirable_flags branch from 358561e to ff5f994 Compare January 26, 2024 02:37

theStack marked this pull request as ready for review January 26, 2024 02:53

DrahtBot mentioned this pull request Jan 26, 2024

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

stratospher reviewed Jan 29, 2024

View reviewed changes

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

DrahtBot requested review from BrandonOdiwuor, furszy and delta1 and removed request for delta1 and BrandonOdiwuor January 29, 2024 07:57

DrahtBot added the Needs rebase label Jan 29, 2024

theStack force-pushed the 202401-test-check_disconnect_lack_desirable_flags branch from ff5f994 to d82eafb Compare January 29, 2024 22:46

DrahtBot removed the Needs rebase label Jan 30, 2024

DrahtBot removed request for delta1 and BrandonOdiwuor January 30, 2024 03:56

DrahtBot requested review from BrandonOdiwuor, marcofleon, epiccurious and delta1 and removed request for delta1, BrandonOdiwuor, marcofleon and epiccurious March 7, 2024 06:15

theStack added 3 commits March 11, 2024 15:23

test: p2p: check disconnect due to lack of desirable service flags

c4a67d3

test: p2p: check limited peers desirability (depending on best block …

2f23987

…depth) This adds coverage for the logic introduced in PR bitcoin#28170 ("p2p: adaptive connections services flags").

theStack force-pushed the 202401-test-check_disconnect_lack_desirable_flags branch from 49fbd57 to 2f23987 Compare March 11, 2024 14:31

DrahtBot requested review from fjahr and cbergqvist March 12, 2024 03:14

fjahr reviewed Mar 12, 2024

View reviewed changes

cbergqvist approved these changes Mar 14, 2024

View reviewed changes

stratospher reviewed Mar 15, 2024

View reviewed changes

glozow merged commit 3d216ba into bitcoin:master Mar 19, 2024
15 of 16 checks passed

theStack deleted the 202401-test-check_disconnect_lack_desirable_flags branch March 19, 2024 17:23

stickies-v mentioned this pull request Mar 21, 2024

Simplify network-adjusted time warning logic #29623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: p2p: check disconnect due to lack of desirable service flags #29279

test: p2p: check disconnect due to lack of desirable service flags #29279

theStack commented Jan 19, 2024

DrahtBot commented Jan 19, 2024 •

edited

delta1 left a comment

furszy left a comment

BrandonOdiwuor left a comment

stratospher left a comment

theStack commented Jan 25, 2024

theStack commented Jan 26, 2024

stratospher left a comment

theStack commented Jan 29, 2024

stratospher commented Jan 30, 2024

theStack commented Mar 11, 2024

davidgumberg commented Mar 12, 2024

fjahr left a comment •

edited

fjahr Mar 12, 2024

cbergqvist left a comment

stratospher left a comment

itornaza commented Mar 18, 2024 •

edited

	if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices))
	{
	LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices));
	pfrom.fDisconnect = true;
	return;
	}

test: p2p: check disconnect due to lack of desirable service flags #29279

test: p2p: check disconnect due to lack of desirable service flags #29279

Conversation

theStack commented Jan 19, 2024

DrahtBot commented Jan 19, 2024 • edited

Code Coverage

Reviews

Conflicts

delta1 left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

stratospher left a comment

Choose a reason for hiding this comment

theStack commented Jan 25, 2024

theStack commented Jan 26, 2024

stratospher left a comment

Choose a reason for hiding this comment

theStack commented Jan 29, 2024

stratospher commented Jan 30, 2024

theStack commented Mar 11, 2024

davidgumberg commented Mar 12, 2024

fjahr left a comment • edited

Choose a reason for hiding this comment

fjahr Mar 12, 2024

Choose a reason for hiding this comment

cbergqvist left a comment

Choose a reason for hiding this comment

stratospher left a comment

Choose a reason for hiding this comment

itornaza commented Mar 18, 2024 • edited

DrahtBot commented Jan 19, 2024 •

edited

fjahr left a comment •

edited

itornaza commented Mar 18, 2024 •

edited