rpc: Make v2transport default for addnode RPC when enabled #29239

sipa · 2024-01-11T21:29:19Z

Since #29058, several types of manually configured connections will attempt v2 connections when -v2transport is enabled, except for the addnode RPC, as that one has an explicit argument to enable or disable.

Make the default for that RPC match the -v2transport setting so the behavior matches that of other manual connections from a user perspective.

DrahtBot · 2024-01-11T21:29:21Z

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	kristapsk, theStack, achow101
Concept ACK	fanquake
Approach ACK	jonatack

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

theStack

Concept ACK

theStack · 2024-01-12T00:47:12Z

test/functional/test_framework/test_framework.py

-            # skip the optional third argument (default false) for
-            # compatibility with older clients
-            from_connection.addnode(ip_port, "onetry")


Isn't this still needed for tests with previous releases (earlier than v26.0), which only accept two arguments for addnode?

Unsure how relevant that is, but I've changed it to drop the 3rd argument whenever it matches the node -v2transport setting (I think).

maflcko

Missing rpc: prefix in title?

src/rpc/net.cpp

kristapsk · 2024-01-12T07:25:45Z

Concept ACK

src/rpc/net.cpp

kristapsk

ACK 3ba815b

theStack

Code-review ACK 3ba815b

jonatack

Approach ACK

jonatack · 2024-01-12T17:51:34Z

src/rpc/net.cpp


-    if (use_v2transport && !(node.connman->GetLocalServices() & NODE_P2P_V2)) {
+    if (use_v2transport && !node_v2transport) {
        throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: v2transport requested but not enabled (see -v2transport)");


Test coverage for this conditional logic and error? Haven't looked deeply but didn't see it in #24748 @stratospher.

Missing test coverage seems unrelated to this pull? (This line and condition is not changed)

jonatack · 2024-01-12T18:09:01Z

src/rpc/net.cpp

@@ -313,7 +313,7 @@ static RPCHelpMan addnode()
                {
                    {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The address of the peer to connect to"},
                    {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once"},
-                    {"v2transport", RPCArg::Type::BOOL, RPCArg::Default{false}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},
+                    {"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},


Perhaps helpful to disambiguate the config option from this addnode option (beyond the - convention)

Suggested change

{"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},

{"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport configuration option"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},

fwiw for the v2transport config option help

current

-v2transport Support v2 transport (default: 0)

suggestion

-v2transport Support BIP324 v2 transport (default: 0)

or

-v2transport Support BIP324 v2 transport protocol (default: 0)

fanquake · 2024-01-15T10:28:37Z

Concept ACK for 27.0.

achow101 · 2024-01-16T21:36:00Z

ACK 3ba815b

Github-Pull: bitcoin#29239 Rebased-From: 3ba815b

…abled

, bitcoin#28849, bitcoin#28805, bitcoin#28951, bitcoin#29058, bitcoin#29239, partial bitcoin#28331, bitcoin#28452 (BIP324 backports: part 2) 5dd60c4 merge bitcoin#29239: Make v2transport default for addnode RPC when enabled (Kittywhiskers Van Gogh) b2ac426 merge bitcoin#29058: use v2transport for manual/addrfetch connections, add to -netinfo (Kittywhiskers Van Gogh) 92e862a merge bitcoin#28951: damage ciphertext/aad in full byte range (Kittywhiskers Van Gogh) 4e96e26 merge bitcoin#28805: Make existing functional tests compatible with --v2transport (Kittywhiskers Van Gogh) 9371e2e merge bitcoin#28849: fix node index bug when comparing peerinfo (Kittywhiskers Van Gogh) 400c9dd merge bitcoin#28634: add check for missing garbage terminator detection (Kittywhiskers Van Gogh) 65eb194 merge bitcoin#28588: add checks for v1 prefix matching / wrong network magic detection (Kittywhiskers Van Gogh) 6074974 merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16 (Kittywhiskers Van Gogh) ff92d1a partial bitcoin#28331: BIP324 integration (Kittywhiskers Van Gogh) f9f8805 fix: drop `CConnman::mapNodesWithDataToSend`, use transport data (UdjinM6) d39d8a4 partial bitcoin#28452: Do not use std::vector = {} to release memory (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6280 * Dependent on #6276 * Dependency for #6329 * [bitcoin#28452](bitcoin#28452) was backported as the behavior it introduces is required for backports like [bitcoin#28331](bitcoin#28331). As it pre-dates earlier BIP324 backports, the `ClearShrink` usage from those backports have also been incorporated into this backport. * When backporting [bitcoin#28331](bitcoin#28331), in TestFramework's `add_nodes()`, `extra_args[i]` is not converted to a `list` like it is done upstream and avoiding duplication of `-v2transport=1` is instead done by an additional check. This is done to prevent test failures in `feature_mnehf.py`. * The local variable `args` is removed in [bitcoin#28805](bitcoin#28805) as it does nothing (the argument appending logic is removed as part of the backport) and upstream removes it anyways. Special thanks to UdjinM6 for significant contributions to this and dependent PRs! 🎉 ## 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: light ACK 5dd60c4 PastaPastaPasta: utACK 5dd60c4 Tree-SHA512: 7f3d0e54e1c96fc99b2d6b1e64485110aa73aeec0e4e4245ed4e6dc0529a7608e9c39103af636d5945d289775c40d3d15d7d4a75bea82462194dbf355fca48dc

sipa changed the title ~~Make v2transaction default for addnode RPC when enabled~~ Make v2transport default for addnode RPC when enabled Jan 11, 2024

sipa force-pushed the 202401_default_addnode_bip324 branch 2 times, most recently from 9e96e32 to f4e0a1a Compare January 11, 2024 21:40

theStack reviewed Jan 12, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 12, 2024

maflcko approved these changes Jan 12, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

fanquake changed the title ~~Make v2transport default for addnode RPC when enabled~~ rpc: Make v2transport default for addnode RPC when enabled Jan 12, 2024

sipa force-pushed the 202401_default_addnode_bip324 branch from f4e0a1a to 6a3461d Compare January 12, 2024 14:18

maflcko reviewed Jan 12, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

glozow added this to the 27.0 milestone Jan 12, 2024

Make v2transport default for addnode RPC when enabled

3ba815b

sipa force-pushed the 202401_default_addnode_bip324 branch from 6a3461d to 3ba815b Compare January 12, 2024 14:31

kristapsk approved these changes Jan 12, 2024

View reviewed changes

DrahtBot requested a review from theStack January 12, 2024 17:21

theStack approved these changes Jan 12, 2024

View reviewed changes

jonatack reviewed Jan 12, 2024

View reviewed changes

DrahtBot requested review from jonatack and fanquake January 16, 2024 21:36

achow101 merged commit 8106b26 into bitcoin:master Jan 16, 2024

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

Make v2transport default for addnode RPC when enabled

7caf2a8

Github-Pull: bitcoin#29239 Rebased-From: 3ba815b

kwvg added a commit to kwvg/dash that referenced this pull request Sep 17, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

63a7054

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Sep 17, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

2d8a45b

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Sep 17, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

194ed82

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Oct 2, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

78c1222

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Oct 9, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

3caaa7a

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Oct 14, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

f8b3594

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

c32892c

…abled

kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

5dd60c4

…abled

bitcoin locked and limited conversation to collaborators Jan 15, 2025

	{"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},
	{"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport configuration option"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},

rpc: Make v2transport default for addnode RPC when enabled #29239

rpc: Make v2transport default for addnode RPC when enabled #29239

Uh oh!

Conversation

sipa commented Jan 11, 2024

Uh oh!

DrahtBot commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

sipa Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kristapsk commented Jan 12, 2024

Uh oh!

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jan 15, 2024

Uh oh!

achow101 commented Jan 16, 2024

Uh oh!

Uh oh!

DrahtBot commented Jan 11, 2024 •

edited

Loading

jonatack Jan 12, 2024 •

edited

Loading