8000 net: enable v2transport by default by sipa · Pull Request #29347 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

net: enable v2transport by default #29347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 31, 2024
Merged

net: enable v2transport by default #29347

merged 1 commit into from
Jan 31, 2024

Conversation

sipa
Copy link
Member
@sipa sipa commented Jan 29, 2024 •
edited
Loading

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
Copy link
Contributor
DrahtBot commented Jan 29, 2024 •
edited
Loading

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 DrahtBot added the P2P label Jan 29, 2024
@sipa sipa mentioned this pull request Jan 29, 2024
Closed
43 tasks
@josibake
Copy link
Member

ACK 292e716 🚀

kristapsk
kristapsk approved these changes Jan 29, 2024
View reviewed changes
Copy link
Contributor
@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cr utACK 292e716

@instagibbs
Copy link
Member

ACK 292e716

pablomartin4btc
pablomartin4btc reviewed Jan 29, 2024
View reviewed changes
Copy link
Member
@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 292e716

@1440000bytes
Copy link
1440000bytes 8000 commented Jan 29, 2024

Concept ACK

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

@sipa
Copy link
Member Author
sipa commented Jan 29, 2024

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.

@pablomartin4btc
Copy link
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?

bitcoin/test/functional/test_framework/test_node.py

Line 208 in 292e716

self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args)

@sipa sipa force-pushed the 202401_gogogo_bip324 branch from 292e716 to b7b25ce Compare January 29, 2024 22:30
@sipa sipa force-pushed the 202401_gogogo_bip324 branch from b7b25ce to 0bef104 Compare January 30, 2024 03:48
@stratospher
Copy link
Contributor

ACK 0bef104.

@josibake
Copy link
Member

reACK 0bef104

@DrahtBot DrahtBot requested review from kristapsk and removed request for kristapsk and josibake January 30, 2024 09:00
naumenkogs
naumenkogs reviewed 8000 Jan 30, 2024
View reviewed changes
Copy link
Member
@naumenkogs naumenkogs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0bef104

test/functional/test_framework/test_node.py
@@ -130,8 +130,15 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor,
# Default behavior from global -v2transport flag is added to args to persist it over restarts.
# May be overwritten in individual tests, using extra_args.
self.default_to_v2 = v2transport
if self.default_to_v2:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DrahtBot DrahtBot requested review from kristapsk and removed request for kristapsk January 30, 2024 09:19
theStack
theStack approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor
@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0bef104

@epiccurious
Copy link
Contributor

utACK

@DrahtBot DrahtBot requested review from kristapsk and removed request for kristapsk January 31, 2024 16:05
kristapsk
kristapsk approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor
@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0bef104

@achow101
Copy link
Member

ACK 0bef104

@achow101 achow101 merged commit 3c63c2f into bitcoin:master Jan 31, 2024
@epiccurious epiccurious mentioned this pull request Feb 10, 2024
Open
@reez reez mentioned this pull request Feb 10, 2024
Closed
@maflcko
Copy link
Member
maflcko commented Feb 19, 2024

Needs bips.md updated?

@fanquake fanquake mentioned this pull request Feb 19, 2024
Merged
glozow added a commit that referenced this pull request Feb 19, 2024
@glozow
Merge #29452: doc: document that BIP324 on by default for v27.0
ddf1d72 
0d3e18b doc: document that BIP324 on by default for v27.0 (fanquake)

Pull request description:

  Addresses: #29347 (comment).

ACKs for top commit:
  maflcko:
    lgtm ACK 0d3e18b
  sipa:
    ACK 0d3e18b
  theStack:
    ACK 0d3e18b

Tree-SHA512: a2af6dbba2740e5cf9c51660059d39577f3744e2adf554222bbc2eac454a161c2e68111797a7cbe34943fe2a424f5fadbeeffcd6f7ae42e3333878875ac43424
@fanquake
Copy link
Member
fanquake commented Mar 4, 2024

Added rel-note todo to the draft wiki.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024
@sipa @luke-jr
net: enable v2transport by default
ff52aa4 
Github-Pull: bitcoin#29347
Rebased-From: 0bef104
@tdb3 tdb3 mentioned this pull request Mar 6, 2024
Merged
@rkrux rkrux mentioned this pull request Mar 25, 2024
Closed
@twofaktor
Copy link

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

@mzumsande
Copy link
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.

@twofaktor twofaktor mentioned this pull request Jun 2, 2024
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29347: net: enable v2transport by default
ce9f878 
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#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.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29347: net: enable v2transport by default
c35e47e 
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#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.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29347: net: enable v2transport by default
3eba0e6 
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#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.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29347: net: enable v2transport by default
b216a40 
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#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.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29347: net: enable v2transport by default
e1a02b6 
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#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.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
kwvg added a commit to kwvg/dash that referenced this pull request Dec 14, 2024
@kwvg
merge bitcoin#29347: enable v2transport by default
c53cd93
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Dec 15, 2024
@PastaPastaPasta
Merge #6488: backport: merge bitcoin#27452, bitcoin#29347, bitcoin#29356
e2095bd 
, bitcoin#29353, bitcoin#29452, bitcoin#29483, bitcoin#30545, bitcoin#31383 (BIP324 backports: part 5)

c6f23a7 merge bitcoin#31383: Add missing node.setmocktime(self.mocktime) to p2p_ibd_stalling.py (Kittywhiskers Van Gogh)
9072a10 merge bitcoin#30545: fix intermittent failures in feature_proxy.py (Kittywhiskers Van Gogh)
7e2d435 merge bitcoin#29483: add --v1transport option, add --v2transport to a CI task (Kittywhiskers Van Gogh)
7e59a96 merge bitcoin#29452: document that BIP324 on by default (Kittywhiskers Van Gogh)
0f3b5e0 merge bitcoin#29353: adhere to typical VERSION message protocol flow (Kittywhiskers Van Gogh)
dfdddfd rpc: enable `v2transport` for `masternode connect` when capable (Kittywhiskers Van Gogh)
3c16361 merge bitcoin#29356: make v2transport arg in addconnection mandatory and few cleanups (Kittywhiskers Van Gogh)
c53cd93 merge bitcoin#29347: enable v2transport by default (Kittywhiskers Van Gogh)
7dcf561 merge bitcoin#27452: cover addrv2 anchors by adding TorV3 to CAddress in messages.py (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * When backporting [bitcoin#27452](bitcoin#27452) in `feature_anchors.py`, `P2P_SERVICES` (`NODE_NETWORK | NODE_HEADERS_COMPRESSED`) has been replaced with `NODE_NETWORK` as the former evaluates to a value greater than `256` (specifically `2049`), which causes test failure. The replacement value is acceptable as `NODE_NETWORK` is the desired service flag expected by Dash Core ([source](https://github.com/dashpay/dash/blob/779e4295ad253bf909a7c45ec02743e580abc61b/src/protocol.cpp#L249-L254)).

    <details>

    <summary>Test failure:</summary>

    ```
    dash@89afd55ae77e:/src/dash$ ./test/functional/feature_anchors.py
    2024-12-14T12:31:22.244000Z TestFramework (INFO): PRNG seed is: 8365703189892653614
    2024-12-14T12:31:22.244000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_j0ya02yu
    2024-12-14T12:31:22.776000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist
    2024-12-14T12:31:22.776000Z TestFramework (INFO): Add 2 block-relay-only connections to node
    2024-12-14T12:31:24.781000Z TestFramework (INFO): Add 5 inbound connections to node
    2024-12-14T12:31:29.843000Z TestFramework (INFO): Check node connections
    2024-12-14T12:31:30.848000Z TestFramework (INFO): Check the addresses in anchors.dat
    2024-12-14T12:31:30.848000Z TestFramework (INFO): Perturb anchors.dat to test it doesn't throw an error during initialization
    2024-12-14T12:31:31.356000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist anymore
    2024-12-14T12:31:31.357000Z TestFramework (INFO): Ensure addrv2 support
    2024-12-14T12:31:32.364000Z TestFramework (INFO): Add 256-bit-address block-relay-only connections to node
    2024-12-14T12:31:33.368000Z TestFramework (INFO): Check for addrv2 addresses in anchors.dat
    2024-12-14T12:31:33.369000Z TestFramework (ERROR): Unexpected exception caught during testing
    Traceback (most recent call last):
      File "/src/dash/test/functional/test_framework/test_framework.py", line 161, in main
        self.run_test()
      File "/src/dash/./test/functional/feature_anchors.py", line 135, in run_test
        new_data[services_index] = P2P_SERVICES
    ValueError: byte must be in range(0, 256)
    2024-12-14T12:31:33.870000Z TestFramework (INFO): Stopping nodes
    2024-12-14T12:31:33.871000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_j0ya02yu
    2024-12-14T12:31:33.871000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_j0ya02yu/test_framework.log
    ```

    </details>

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] 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 c6f23a7
  PastaPastaPasta:
    utACK c6f23a7;

Tree-SHA512: ca134432d000d521827a20c75c03913421fe52a31fda1cbf632e0b207c31728406feb090469d592d8e2fd8d64298faa2752ff703de79f737a62c276c6a231097
@bitcoin bitcoin locked and limited conversation to collaborators Apr 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@naumenkogs naumenkogs naumenkogs left review comments

@theStack theStack theStack approved these changes

@kristapsk kristapsk kristapsk approved these changes

@willcl-ark willcl-ark willcl-ark approved these changes

@BrandonOdiwuor BrandonOdiwuor BrandonOdiwuor approved these changes

@pablomartin4btc pablomartin4btc pablomartin4btc approved these changes

@instagibbs instagibbs Awaiting requested review from instagibbs

@mzumsande mzumsande Awaiting requested review from mzumsande

Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0