test: use v2 everywhere for P2PConnection if --v2transport is enabled

mzumsande commented at 7:22 pm on January 31, 2024: contributor

#24748 added v2 transport to the python P2PConnection, but so far each test that wants to make use of it needs to enable it on an individual basis. This PR changes it so that if the test suite is run with --v2transport option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever P2PConnection is used. Individual tests can override this global option.

To do that, a few tests need to be adjusted. In addition, I added a commit to always use v1 in a few select subtests that send a large number of large messages (e.g. large reorgs). These tests don’t have a fundamental problem with v2 but become very slow due to the unoptimised python ChaCha20 implementation (~30 minutes on my computer, so probably not suitable to be run in the CI).

As a result, python3 test_runner.py --v2transport should succeed and use v2 everywhere (unless v1 is chosen explicitly).

[Edit]: To make the “test each commit” CI pass, several test fixes were squashed into the last commit, which actually enables v2 p2p for P2PConnection. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review.

DrahtBot commented at 7:22 pm on January 31, 2024: contributor

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	vasild, stratospher, theStack, fjahr
Stale ACK	epiccurious

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29431 (test/BIP324: disconnection scenarios during v2 handshake by stratospher)
#29420 (test: extend the SOCKS5 Python proxy to actually connect to a destination by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#29279 (test: p2p: check disconnect due to lack of desirable service flags by theStack)
#28521 (net: additional disconnect logging by Sjors)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jan 31, 2024

mzumsande force-pushed on Jan 31, 2024

mzumsande marked this as a draft on Jan 31, 2024

mzumsande commented at 8:59 pm on January 31, 2024: contributor

Hmm, “test each commit fails” The problem is that commit 606f4f32014f029a6999d7f94b7231fefafdf55f (which switches P2PConnection to v2 by default) is enabled last. It might be best to squash everything into one big commit (even if that might make review a bit harder) or I’d have to add some temporary workarounds. Opinions?

DrahtBot added the label Needs rebase on Jan 31, 2024

stratospher commented at 5:50 am on February 1, 2024: contributor

Hmm, “test each commit fails” The problem is that commit https://github.com/bitcoin/bitcoin/commit/606f4f32014f029a6999d7f94b7231fefafdf55f (which switches P2PConnection to v2 by default) is enabled last. It might be best to squash everything into one big commit (even if that might make review a bit harder) or I’d have to add some temporary workarounds. Opinions?

would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits.

epiccurious commented at 3:14 pm on February 1, 2024: none

Testing looks good so far:

 0TEST                              | STATUS    | DURATION
 1
 2feature_block.py                  | ✓ Passed  | 67 s
 3feature_maxuploadtarget.py        | ✓ Passed  | 58 s
 4p2p_ibd_stalling.py               | ✓ Passed  | 7 s
 5p2p_ibd_stalling.py --v2transport | ✓ Passed  | 8 s
 6p2p_invalid_messages.py           | ✓ Passed  | 15 s
 7p2p_timeouts.py                   | ✓ Passed  | 1 s
 8p2p_timeouts.py --v2transport     | ✓ Passed  | 1 s
 9p2p_v2_earlykeyresponse.py        | ✓ Passed  | 5 s
10rpc_net.py                        | ✓ Passed  | 14 s
11rpc_net.py --v2transport          | ✓ Passed  | 10 s
12
13ALL                               | ✓ Passed  | 186 s (accumulated) 
14Runtime: 67 s

(Noob question) I’m struggling to test the changes in test_node.py. Any Idea what I’m doing wrong here?

0user1@comp1:~/bitcoin-pull-29358$ python3 test/functional/test_framework/test_node.py
1Traceback (most recent call last):
2  File "/home/user1/bitcoin-pull-29358/test/functional/test_framework/test_node.py", line 25, in <module>
3    from .authproxy import (
4ImportError: attempted relative import with no known parent package
5user1@comp1:~/bitcoin-pull-29358$ python3 test/functional/test_runner.py test/functional/test_framework/test_node.py
6Temporary test directory at /tmp/test_runner_₿_🏃_20240201_101326
7WARNING! Test 'test/functional/test_framework/test_node.py' not found in full test list.
8No valid test scripts specified. Check that your test is in one of the test lists in test_runner.py, or run test_runner.py with no arguments to run all tests

epiccurious commented at 3:35 pm on February 1, 2024: none

I added a commit disabling a few select subtests

Rather than disabling, would it make sense to keep these subtests enabled but just not use v2transport?

mzumsande commented at 7:09 pm on February 1, 2024: contributor

would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits.

That wouldn’t work either, because with that last commit we actually use v2 when using P2PConnection (if --v2transport is chosen), so all of the tests that are fixed in this PR would fail. The problem is that we already list multiple the tests in the test runner with --v2transport but actually use v1 with P2PConnection… I’ll try to think of other solutions. Or maybe I’ll just keep it as single commits for now and when it comes closer to merging I’ll squash this branch and link to a second branch here that is unsquashed but otherwise identical.

(Noob question) I’m struggling to test the changes in test_node.py. Any Idea what I’m doing wrong here?

test_node.py (and the other files in the test/functional/test_framework subfolder) are not actual tests that can be executed directly, only the actual tests inside the test/functional folder can.

Rather than disabling, would it make sense to keep these subtests enabled but just not use v2transport?

Yes, that makes sense I’ll try that out!

mzumsande force-pushed on Feb 1, 2024

mzumsande commented at 8:36 pm on February 1, 2024: contributor

rebased and always use v1 instead of disabling completely in the three slow (sub)tests.

mzumsande marked this as ready for review on Feb 1, 2024

DrahtBot removed the label Needs rebase on Feb 1, 2024

theStack commented at 12:45 pm on February 2, 2024: contributor

Concept ACK

epiccurious commented at 2:08 pm on February 2, 2024: none

Tested ACK d8165db8e889f423351ae35fd375c982e793d136.

not actual tests that can be executed directly

Thanks for the clarification.

Certain tests take longer to run, to be expected with the added sub-tests.

 0TEST                              | STATUS    | DURATION
 1
 2feature_block.py                  | ✓ Passed  | 197 s
 3feature_maxuploadtarget.py        | ✓ Passed  | 59 s
 4p2p_ibd_stalling.py               | ✓ Passed  | 59 s
 5p2p_ibd_stalling.py --v2transport | ✓ Passed  | 58 s
 6p2p_invalid_messages.py           | ✓ Passed  | 58 s
 7p2p_timeouts.py                   | ✓ Passed  | 1 s
 8p2p_timeouts.py --v2transport     | ✓ Passed  | 1 s
 9p2p_v2_earlykeyresponse.py        | ✓ Passed  | 4 s
10rpc_net.py                        | ✓ Passed  | 22 s
11rpc_net.py --v2transport          | ✓ Passed  | 19 s
12
13ALL                               | ✓ Passed  | 478 s (accumulated) 
14Runtime: 197 s

DrahtBot requested review from theStack on Feb 2, 2024

in test/functional/feature_maxuploadtarget.py:70 in d065b33b25 outdated

66@@ -67,7 +67,8 @@ def run_test(self):
67         p2p_conns = []
68 
69         for _ in range(3):
70-            p2p_conns.append(self.nodes[0].add_p2p_connection(TestP2PConn()))
71+            # Don't use v2transport in this test (too slow with the unoptimized python ChCha20 implementation)

stratospher commented at 4:42 pm on February 2, 2024:

d065b33: nit: ChaCha20 typo here and in feature_block.py

mzumsande commented at 6:32 pm on February 7, 2024:

fixed

in test/functional/feature_block.py:1266 in d065b33b25 outdated

1262@@ -1263,6 +1263,10 @@ def run_test(self):
1263         b89a = self.update_block("89a", [tx])
1264         self.send_blocks([b89a], success=False, reject_reason='bad-txns-inputs-missingorspent', reconnect=True)
1265 
1266+        # Don't use v2transport for the remaining tests, the large reorg is very slow with the unoptimized python ChCha20 implementation

stratospher commented at 5:35 pm on February 2, 2024:

d065b33: maybe say only for LARGE_REORG_SIZE tests? because the last 2 tests have v2 peers after reconnection due to block height mismatch/invalid block header version.

mzumsande commented at 6:33 pm on February 7, 2024:

good point, done something similar.

stratospher commented at 5:36 pm on February 2, 2024: contributor

tested ACK d8165db.

mzumsande commented at 7:45 pm on February 5, 2024: contributor

Will address feedback and #29356 (review) after the merge of #29356

mzumsande marked this as a draft on Feb 5, 2024

test: enable p2p_sendtxrcncl.py with v2transport

By adding to the test framework a wait until the v2 handshake
is completed, so that p2p_sendtxrcncl.py (which doesn't need
to be changed itself) doesnt't send out any other messages before that.

5fc9db504b

test: enable p2p_invalid_messages.py with v2transport

by disabling some sub-tests that test v1-specific features,
and adapting others to v2.

87549c8f89

test: Don't use v2transport when it's too slow.

Sending multiple large messages is rather slow with the non-optimized python
implementation of ChaCha20.
Apart from the slowness, these tests would also run successfully with v2.

6e9e39da43

mzumsande force-pushed on Feb 7, 2024

mzumsande commented at 6:35 pm on February 7, 2024: contributor

I have addressed feedback and squashed several test fixes into the last commit, so that the “test each commit” CI is hopefully green now. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review.

mzumsande marked this as ready for review on Feb 7, 2024

in test/functional/p2p_ibd_stalling.py:83 in b9912e2df9 outdated

79@@ -80,7 +80,8 @@ def run_test(self):
80 
81         # Need to wait until 1023 blocks are received - the magic total bytes number is a workaround in lack of an rpc
82         # returning the number of downloaded (but not connected) blocks.
83-        self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761)

vasild commented at 4:12 pm on February 11, 2024:

nit: in commit message of b9912e2df9767540ab7f53cb3ba37fd2e2491806 test: enable v2 for python p2p depending on global --v2transport flag:

s/all connection/all connections/
“before this test” - should this be “before this change”?

mzumsande commented at 3:47 pm on February 12, 2024:

yes. Fixed both.

in test/functional/p2p_timeouts.py:72 in b9912e2df9 outdated

68@@ -69,11 +69,8 @@ def run_test(self):
69         with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']):
70             no_verack_node.send_message(msg_ping())
71 
72-        # With v2, non-version messages before the handshake would be interpreted as part of the key exchange.

vasild commented at 4:37 pm on February 11, 2024:

test/functional/test_runner.py contains:

0    'rpc_net.py',
1    'rpc_net.py --v2transport',

So, on master, when I execute test_runner.py it will run this test two times, once in v1 mode and once in v2 mode. What about if I run, with this PR, test_runner.py --v2transport? Would that execute rpc_net.py two times and both in v2 mode? If yes, then what about adding --v1transport option as well? Or maybe just change the above to just:

0    'rpc_net.py',

and expect that callers will run test_runner.py and test_runener.py --v2transport?

mzumsande commented at 3:43 pm on February 12, 2024:

Would that execute rpc_net.py two times and both in v2 mode?

Yes, although it’s not really related to this PR it’s the same on master (just that P2PConnections use v1 both times there). I had thought about this before and was thinking of addressing it in a follow-up PR, which would also add a CI job for --v2transport. The downside of dropping the --v2transport specific jobs from test_runner.py is that these would be executed a lot less by individuals running the test suite (even if we have a --v2transport CI job for one specific platform). So I kind of tend to prefer something like --v1transport, especially since the test that run twice currently don’t take a lot of time compared to the longest running jobs.

vasild commented at 4:17 pm on February 12, 2024:

Alright, “it’s the same on master” that is a good point!

So, if we have --v1transport (for each individual test and for test_runner.py?) then what would happen if ran without either one of --v1transport or --v2transport?

mzumsande commented at 4:42 pm on February 12, 2024:

We’d have to make some kind of decision what to default to - maybe v1 now and switch to v2 after a majority of the network has upgraded? My idea for now was to only add --v1transport in test_runner.py to the few tests that are run twice, not everywhere.

fjahr commented at 1:35 pm on February 20, 2024:

I would either expect that the internally set option can not be overridden outside because the developers have decided that this test has to run this setting to keep good coverage no matter what the user thinks or that the test is skipped when the internal option is not compatible with what is provided from outside. Depends a bit on how important v1 test coverage is for us in the short term I think.

mzumsande commented at 7:53 pm on February 26, 2024:

I have opened the draft PR #29483 with a proposal / to continue this discussion about CI and test_runner.

vasild commented at 5:27 pm on February 11, 2024: contributor

Approach ACK b9912e2df9767540ab7f53cb3ba37fd2e2491806

The code changes look good. Just the question below.

test: enable v2 for python p2p depending on global --v2transport flag

This changes the default behavior, individual tests can overwrite this option.
As a result, it is possible to run the entire test suite with
--v2transport, and all connections to the python p2p will then use it.

Also adjust several tests that are already running with --v2transport in the
test runner (although they actually made v1 connection before this change).
This is done in the same commit so that there isn't an
intermediate commit in which the CI fails.

bf5662c678

mzumsande force-pushed on Feb 12, 2024

mzumsande commented at 3:49 pm on February 12, 2024: contributor

b9912e2 to bf5662c: just small fixes to the last commit message.

vasild approved

vasild commented at 4:09 pm on February 12, 2024: contributor

ACK bf5662c678455fb47c402f8520215726ddfe7a94

test: use v2 everywhere for P2PConnection if –v2transport is enabled #29358

Code Coverage

Reviews

Conflicts