net: enable v2transport by default

sipa commented at 7:14 pm on January 29, 2024: member

This enables BIP324’s v2 transport by default (see #27634):

Inbound connections will auto-sense whether v1 or v2 is in use.
Automatic outbound connections will use v2 if NODE_P2P_V2 was set in addr gossip, but retry with v1 if met with immediate failure.
Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

It remains possible to run with -v2transport=0 to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the v2transport argument to the addnode RPC as false, to disable attempting a v2 connection for that particular added node.

DrahtBot commented at 7:14 pm on January 29, 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	stratospher, josibake, naumenkogs, theStack, willcl-ark, BrandonOdiwuor, pablomartin4btc, kristapsk, achow101
Concept ACK	1440000bytes, mzumsande, epiccurious
Stale ACK	instagibbs

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

DrahtBot added the label P2P on Jan 29, 2024

josibake commented at 8:05 pm on January 29, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/29347/commits/292e716cde3325bef83e78f7804d2d0bddf03509 :rocket:

kristapsk approved

kristapsk commented at 8:13 pm on January 29, 2024: contributor

cr utACK 292e716cde3325bef83e78f7804d2d0bddf03509

instagibbs commented at 8:24 pm on January 29, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/29347/commits/292e716cde3325bef83e78f7804d2d0bddf03509

pablomartin4btc commented at 10:03 pm on January 29, 2024: member

ACK 292e716cde3325bef83e78f7804d2d0bddf03509

1440000bytes commented at 10:10 pm on January 29, 2024: none

Concept ACK

Not sure if it should be enabled for onion/i2p connections by default.

sipa commented at 10:22 pm on January 29, 2024: member

Hmm, no. The issue is that previous releases may not even support the -v2transport flag, so to determine whether to set it, we need to know what version we’re talking about.

DrahtBot added the label CI failed on Jan 29, 2024

pablomartin4btc commented at 10:29 pm on January 29, 2024: member

Hmm, no. The issue is that previous releases may not even support the -v2transport flag, so to determine whether to set it, we need to know what version we’re talking about.

Sorry I deleted my comment cos I realised it was wrong as other tests would have failed… originally was:

looking at the CI failure very quickly without testing… maybe you need to update this line too? https://github.com/bitcoin/bitcoin/blob/292e716cde3325bef83e78f7804d2d0bddf03509/test/functional/test_framework/test_node.py#L208

sipa force-pushed on Jan 29, 2024

net: enable v2transport by default 0bef1042ce

sipa force-pushed on Jan 30, 2024

DrahtBot removed the label CI failed on Jan 30, 2024

stratospher commented at 5:01 am on January 30, 2024: contributor

ACK 0bef104.

DrahtBot requested review from josibake on Jan 30, 2024

DrahtBot requested review from instagibbs on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

DrahtBot requested review from pablomartin4btc on Jan 30, 2024

josibake commented at 9:00 am on January 30, 2024: member

reACK https://github.com/bitcoin/bitcoin/pull/29347/commits/0bef1042ce6c459acb1de965cbccd98867a417f1

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot removed review request from josibake on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

in test/functional/test_framework/test_node.py:133 in 0bef1042ce

129@@ -130,8 +130,15 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor,
130         # Default behavior from global -v2transport flag is added to args to persist it over restarts.
131         # May be overwritten in individual tests, using extra_args.
132         self.default_to_v2 = v2transport
133-        if self.default_to_v2:

naumenkogs commented at 9:19 am on January 30, 2024:

i’m not sure we want to keep default_to_v2. It’s kinda unused now, and I’m not sure future tests would ever find it useful.

naumenkogs commented at 9:19 am on January 30, 2024: member

ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

theStack approved

theStack commented at 9:24 am on January 30, 2024: contributor

ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

willcl-ark commented at 11:58 am on January 30, 2024: contributor

If we are now defaulting v2 to enabled, do we want to enable v2transport on a few more of the p2p functional tests, here or in a followup? I currently see the following enabled/disabled variants running the test suite with default options:

 0p2p_add_connections.py                                 | ✓ Passed  | 7 s
 1p2p_addr_relay.py                                      | ✓ Passed  | 15 s
 2p2p_addrfetch.py                                       | ✓ Passed  | 1 s
 3p2p_addrv2_relay.py                                    | ✓ Passed  | 1 s
 4p2p_block_sync.py                                      | ✓ Passed  | 2 s
 5p2p_block_sync.py --v2transport                        | ✓ Passed  | 2 s
 6p2p_blockfilters.py                                    | ✓ Passed  | 84 s
 7p2p_blocksonly.py                                      | ✓ Passed  | 16 s
 8p2p_compactblocks.py                                   | ✓ Passed  | 10 s
 9p2p_compactblocks_blocksonly.py                        | ✓ Passed  | 2 s
10p2p_compactblocks_hb.py                                | ✓ Passed  | 18 s
11p2p_compactblocks_hb.py --v2transport                  | ✓ Passed  | 18 s
12p2p_disconnect_ban.py                                  | ✓ Passed  | 2 s
13p2p_disconnect_ban.py --v2transport                    | ✓ Passed  | 2 s
14p2p_dns_seeds.py                                       | ✓ Passed  | 27 s
15p2p_dos_header_tree.py                                 | ✓ Passed  | 2 s
16p2p_eviction.py                                        | ✓ Passed  | 5 s
17p2p_feefilter.py                                       | ✓ Passed  | 16 s
18p2p_filter.py                                          | ✓ Passed  | 3 s
19p2p_fingerprint.py                                     | ✓ Passed  | 2 s
20p2p_getaddr_caching.py                                 | ✓ Passed  | 8 s
21p2p_getdata.py                                         | ✓ Passed  | 1 s
22p2p_headers_sync_with_minchainwork.py                  | ✓ Passed  | 50 s
23p2p_i2p_ports.py                                       | ✓ Passed  | 3 s
24p2p_i2p_sessions.py                                    | ✓ Passed  | 1 s
25p2p_ibd_stalling.py                                    | ✓ Passed  | 37 s
26p2p_ibd_stalling.py --v2transport                      | ✓ Passed  | 37 s
27p2p_ibd_txrelay.py                                     | ✓ Passed  | 2 s
28p2p_initial_headers_sync.py                            | ✓ Passed  | 1 s
29p2p_invalid_block.py                                   | ✓ Passed  | 2 s
30p2p_invalid_block.py --v2transport                     | ✓ Passed  | 2 s
31p2p_invalid_locator.py                                 | ✓ Passed  | 1 s
32p2p_invalid_messages.py                                | ✓ Passed  | 29 s
33p2p_invalid_tx.py                                      | ✓ Passed  | 8 s
34p2p_invalid_tx.py --v2transport                        | ✓ Passed  | 7 s
35p2p_leak.py                                            | ✓ Passed  | 6 s
36p2p_leak_tx.py                                         | ✓ Passed  | 23 s
37p2p_leak_tx.py --v2transport                           | ✓ Passed  | 8 s
38p2p_message_capture.py                                 | ✓ Passed  | 1 s
39p2p_net_deadlock.py                                    | ✓ Passed  | 2 s
40p2p_net_deadlock.py --v2transport                      | ✓ Passed  | 2 s
41p2p_nobloomfilter_messages.py                          | ✓ Passed  | 1 s
42p2p_node_network_limited.py                            | ✓ Passed  | 10 s
43p2p_node_network_limited.py --v2transport              | ✓ Passed  | 11 s
44p2p_orphan_handling.py                                 | ✓ Passed  | 7 s
45p2p_permissions.py                                     | ✓ Passed  | 8 s
46p2p_ping.py                                            | ✓ Passed  | 1 s
47p2p_segwit.py                                          | ✓ Passed  | 104 s
48p2p_sendheaders.py                                     | ✓ Passed  | 23 s
49p2p_sendtxrcncl.py                                     | ✓ Passed  | 5 s
50p2p_timeouts.py                                        | ✓ Passed  | 1 s
51p2p_timeouts.py --v2transport                          | ✓ Passed  | 1 s
52p2p_tx_download.py                                     | ✓ Passed  | 36 s
53p2p_tx_privacy.py                                      | ✓ Passed  | 8 s
54p2p_unrequested_blocks.py                              | ✓ Passed  | 5 s
55p2p_v2_earlykeyresponse.py                             | ✓ Passed  | 4 s
56p2p_v2_encrypted.py                                    | ✓ Passed  | 8 s
57p2p_v2_transport.py                                    | ✓ Passed  | 8 s

By adding some logging I measure that it is currently only enabled on 21 out of 102 p2p_*.py test nodes used running the suite. It can be enabled on all 102 test nodes with the global --v2transport option passed to test_runner, but I don’t think that’s used either by our CI or (routinely) by developers.

I appreciate that v2 should not directly affect many of these tests, but one of the benefits of the functional tests is to catch unforeseen interactions.

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

sipa commented at 1:36 pm on January 30, 2024: member

@willcl-ark I believe @mzumsande is planning to open a PR to enable it everywhere in the tests.

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

willcl-ark approved

willcl-ark commented at 2:10 pm on January 30, 2024: contributor

crACK 0bef1042ce6c459acb1de965cbccd98867a417f1

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

BrandonOdiwuor approved

BrandonOdiwuor commented at 2:27 pm on January 30, 2024: contributor

utACK 0bef1042ce6c459acb1de965cbccd98867a417f1

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

mzumsande commented at 3:09 pm on January 30, 2024: contributor

Concept ACK

@willcl-ark I believe @mzumsande is planning to open a PR to enable it everywhere in the tests.

Yes, I’ll open a PR later this week (current branch https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests which still needs some cleanups)! [Edit: Now opened in #29358]

If we are now defaulting v2 to enabled, do we want to enable v2transport on a few more of the p2p functional tests

It’s more than adding more tests to the list, the -v2transport option currently doesn’t make the python P2PConnection use v2 (even after #24748 added support), so the status quo is that specific tests need to enable that explicitly.

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

pablomartin4btc approved

pablomartin4btc commented at 4:44 pm on January 30, 2024: member

re ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

DrahtBot removed review request from kristapsk on Jan 30, 2024

DrahtBot requested review from mzumsande on Jan 30, 2024

DrahtBot requested review from kristapsk on Jan 30, 2024

epiccurious commented at 4:05 pm on January 31, 2024: none

utACK

DrahtBot removed review request from kristapsk on Jan 31, 2024

DrahtBot requested review from kristapsk on Jan 31, 2024

kristapsk approved

kristapsk commented at 7:32 pm on January 31, 2024: contributor

utACK 0bef1042ce6c459acb1de965cbccd98867a417f1

achow101 added the label Needs release note on Jan 31, 2024

achow101 commented at 8:24 pm on January 31, 2024: member

ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

achow101 merged this on Jan 31, 2024

achow101 closed this on Jan 31, 2024

maflcko commented at 12:20 pm on February 19, 2024: member

Needs bips.md updated?

glozow referenced this in commit ddf1d72cc2 on Feb 19, 2024

fanquake removed the label Needs release note on Mar 4, 2024

fanquake commented at 11:57 am on March 4, 2024: member

Added rel-note todo to the draft wiki.

luke-jr referenced this in commit ff52aa4a22 on Mar 5, 2024

twofaktor commented at 10:58 pm on April 1, 2024: none

Will it still be neccesary to specify v2transport=1 parameter on bitcoin.conf starting from version 27 to support v2 transport protocol?

mzumsande commented at 0:17 am on April 2, 2024: contributor

Will it still be neccesary to specify v2transport=1 parameter on bitcoin.conf starting from version 27 to support v2 transport protocol?

No, that won’t be necessary (thought it won’t hurt to leave it in). The default was changed with this PR, so if you wanted to not support v2 for some reason you’d now have to include v2transport=0 in your bitcoin.conf.

net: enable v2transport by default #29347

Code Coverage

Reviews