rpc: Make v2transport default for addnode RPC when enabled #29239
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee 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. |
sipa
changed the title
sipa
force-pushed
the
202401_default_addnode_bip324
branch
2 times, most recently
from
9e96e32
to
f4e0a1a
Compare
| # skip the optional third argument (default false) for | ||
| # compatibility with older clients | ||
| from_connection.addnode(ip_port, "onetry") |
There was a problem hiding this comment.
Isn't this still needed for tests with previous releases (earlier than v26.0), which only accept two arguments for addnode?
There was a problem hiding this comment.
Unsure how relevant that is, but I've changed it to drop the 3rd argument whenever it matches the node -v2transport setting (I think).
|
Concept ACK |
fanquake
changed the title
sipa
force-pushed
the
202401_default_addnode_bip324
branch
from
f4e0a1a
to
6a3461d
Compare
sipa
force-pushed
the
202401_default_addnode_bip324
branch
from
6a3461d
to
3ba815b
Compare
|
|
||
| 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)"); |
There was a problem hiding this comment.
Test coverage for this conditional logic and error? Haven't looked deeply but didn't see it in #24748 @stratospher.
There was a problem hiding this comment.
Missing test coverage seems unrelated to this pull? (This line and condition is not changed)
| @@ -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)"}, | |||
There was a problem hiding this comment.
Perhaps helpful to disambiguate the config option from this addnode option (beyond the - convention)
| {"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)"}, |
There was a problem hiding this comment.
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)
|
Concept ACK for 27.0. |
|
ACK 3ba815b |
Github-Pull: bitcoin#29239 Rebased-From: 3ba815b
Since #29058, several types of manually configured connections will attempt v2 connections when
-v2transportis enabled, except for theaddnodeRPC, as that one has an explicit argument to enable or disable.Make the default for that RPC match the
-v2transportsetting so the behavior matches that of other manual connections from a user perspective.