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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
601 | @@ -602,12 +602,7 @@ def connect_nodes(self, a, b, *, peer_advertises_v2=None, wait_for_connect: bool 602 | if peer_advertises_v2 is None: 603 | peer_advertises_v2 = from_connection.use_v2transport 604 | 605 | - if peer_advertises_v2: 606 | - from_connection.addnode(node=ip_port, command="onetry", v2transport=True) 607 | - else: 608 | - # skip the optional third argument (default false) for 609 | - # compatibility with older clients 610 | - 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).
Concept ACK
336 | - 337 | - if (use_v2transport && !(node.connman->GetLocalServices() & NODE_P2P_V2)) { 338 | + auto use_v2transport = self.MaybeArg<bool>(2); 339 | + bool node_v2transport = node.connman->GetLocalServices() & NODE_P2P_V2; 340 | + if (!use_v2transport.has_value()) use_v2transport = node_v2transport; 341 | + if (*use_v2transport && !node_v2transport) {
nit: Can be written shorter
bool node_v2transport = node.connman->GetLocalServices() & NODE_P2P_V2;
bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);
if (use_v2transport && !node_v2transport) {
Nice, done.
Missing rpc: prefix in title?
Concept ACK
331 | @@ -332,9 +332,10 @@ static RPCHelpMan addnode() 332 | CConnman& connman = EnsureConnman(node); 333 | 334 | const std::string node_arg{request.params[0].get_str()}; 335 | - bool use_v2transport = self.Arg<bool>(2); 336 | + bool node_v2transport = node.connman->GetLocalServices() & NODE_P2P_V2;
bool node_v2transport = connman.GetLocalServices() & NODE_P2P_V2;
nit, if you re-touch
Done. I needed to re-touch anyway because apparently nool is not a C++ type.
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
Code-review ACK 3ba815b42db74804e341ce15f648c2b297af55ca
336 | + bool node_v2transport = connman.GetLocalServices() & NODE_P2P_V2; 337 | + bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport); 338 | 339 | - if (use_v2transport && !(node.connman->GetLocalServices() & NODE_P2P_V2)) { 340 | + if (use_v2transport && !node_v2transport) { 341 | 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)
312 | @@ -313,7 +313,7 @@ static RPCHelpMan addnode() 313 | { 314 | {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The address of the peer to connect to"}, 315 | {"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"}, 316 | - {"v2transport", RPCArg::Type::BOOL, RPCArg::Default{false}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"}, 317 | + {"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)
{"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)
Approach ACK
Concept ACK for 27.0.
ACK 3ba815b42db74804e341ce15f648c2b297af55ca