net: enable v2transport by default #29347

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202401_gogogo_bip324 changing 2 files +10 −3
  1. 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.

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

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

  3. DrahtBot added the label P2P on Jan 29, 2024
  4. kristapsk approved
  5. kristapsk commented at 8:13 pm on January 29, 2024: contributor
    cr utACK 292e716cde3325bef83e78f7804d2d0bddf03509
  6. pablomartin4btc commented at 10:03 pm on January 29, 2024: member
    ACK 292e716cde3325bef83e78f7804d2d0bddf03509
  7. 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.

  8. 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.
  9. DrahtBot added the label CI failed on Jan 29, 2024
  10. 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

  11. sipa force-pushed on Jan 29, 2024
  12. net: enable v2transport by default 0bef1042ce
  13. sipa force-pushed on Jan 30, 2024
  14. DrahtBot removed the label CI failed on Jan 30, 2024
  15. stratospher commented at 5:01 am on January 30, 2024: contributor
    ACK 0bef104.
  16. DrahtBot requested review from josibake on Jan 30, 2024
  17. DrahtBot requested review from instagibbs on Jan 30, 2024
  18. DrahtBot requested review from kristapsk on Jan 30, 2024
  19. DrahtBot requested review from pablomartin4btc on Jan 30, 2024
  20. DrahtBot removed review request from kristapsk on Jan 30, 2024
  21. DrahtBot removed review request from josibake on Jan 30, 2024
  22. DrahtBot requested review from kristapsk on Jan 30, 2024
  23. 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.
  24. naumenkogs commented at 9:19 am on January 30, 2024: member
    ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  25. DrahtBot removed review request from kristapsk on Jan 30, 2024
  26. DrahtBot requested review from kristapsk on Jan 30, 2024
  27. DrahtBot removed review request from kristapsk on Jan 30, 2024
  28. DrahtBot requested review from kristapsk on Jan 30, 2024
  29. theStack approved
  30. theStack commented at 9:24 am on January 30, 2024: contributor
    ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  31. DrahtBot removed review request from kristapsk on Jan 30, 2024
  32. DrahtBot requested review from kristapsk on Jan 30, 2024
  33. 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.

  34. DrahtBot removed review request from kristapsk on Jan 30, 2024
  35. DrahtBot requested review from kristapsk on Jan 30, 2024
  36. 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.
  37. DrahtBot removed review request from kristapsk on Jan 30, 2024
  38. DrahtBot requested review from kristapsk on Jan 30, 2024
  39. willcl-ark approved
  40. willcl-ark commented at 2:10 pm on January 30, 2024: contributor
    crACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  41. DrahtBot removed review request from kristapsk on Jan 30, 2024
  42. DrahtBot requested review from kristapsk on Jan 30, 2024
  43. BrandonOdiwuor approved
  44. BrandonOdiwuor commented at 2:27 pm on January 30, 2024: contributor
    utACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  45. DrahtBot removed review request from kristapsk on Jan 30, 2024
  46. DrahtBot requested review from kristapsk on Jan 30, 2024
  47. DrahtBot removed review request from kristapsk on Jan 30, 2024
  48. DrahtBot requested review from kristapsk on Jan 30, 2024
  49. 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.

  50. DrahtBot removed review request from kristapsk on Jan 30, 2024
  51. DrahtBot requested review from kristapsk on Jan 30, 2024
  52. pablomartin4btc approved
  53. pablomartin4btc commented at 4:44 pm on January 30, 2024: member
    re ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  54. DrahtBot removed review request from kristapsk on Jan 30, 2024
  55. DrahtBot requested review from mzumsande on Jan 30, 2024
  56. DrahtBot requested review from kristapsk on Jan 30, 2024
  57. epiccurious commented at 4:05 pm on January 31, 2024: none
    utACK
  58. DrahtBot removed review request from kristapsk on Jan 31, 2024
  59. DrahtBot requested review from kristapsk on Jan 31, 2024
  60. kristapsk approved
  61. kristapsk commented at 7:32 pm on January 31, 2024: contributor
    utACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  62. achow101 added the label Needs release note on Jan 31, 2024
  63. achow101 commented at 8:24 pm on January 31, 2024: member
    ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
  64. achow101 merged this on Jan 31, 2024
  65. achow101 closed this on Jan 31, 2024

  66. maflcko commented at 12:20 pm on February 19, 2024: member
    Needs bips.md updated?
  67. glozow referenced this in commit ddf1d72cc2 on Feb 19, 2024
  68. fanquake removed the label Needs release note on Mar 4, 2024
  69. fanquake commented at 11:57 am on March 4, 2024: member
    Added rel-note todo to the draft wiki.
  70. luke-jr referenced this in commit ff52aa4a22 on Mar 5, 2024
  71. 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?
  72. 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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me