Skip to content
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

net: enable v2transport by default #29347

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

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

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

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)

@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 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

@DrahtBot DrahtBot requested review from kristapsk and removed request for kristapsk January 30, 2024 14:27
mzumsande
mzumsande reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment •
edited

Choose a reason for hiding this comment

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

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 DrahtBot requested review from kristapsk and removed request for kristapsk January 30, 2024 15:09
pablomartin4btc
pablomartin4btc approved these changes Jan 30, 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.

re ACK 0bef104

@DrahtBot DrahtBot requested review from mzumsande and kristapsk and removed request for kristapsk January 30, 2024 16:44
@epiccurious
Copy link

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
16 checks passed
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet