Merge bitcoin/bitcoin#26366: rpc, test: addnode improv + add test coverage for invalid command

f52cb02f70 doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg)
effd1efefb test: `addnode` with an invalid command should throw an error (brunoerg)
56b27b8487 rpc, refactor: clean-up `addnode` (brunoerg)

Pull request description:

  This PR:

  - Adds test coverage for an invalid `command` in `addnode`.
  - Rename `test_getaddednodeinfo` to `test_addnode_getaddednodeinfo` and its log since this function also tests `addnode` and it doesn't worth to split into 2 ones.
  - Makes it clear in docs that `node` in `addnode` refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id.
  - Some small improv/clean-up: use `const` where possible, rename some vars, and remove the check for nullance for `command` since it's a non-optional field.

ACKs for top commit:
  achow101:
    ACK f52cb02f70
  jonatack:
    ACK f52cb02f70
  theStack:
    re-ACK f52cb02f70

Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e

src/rpc/net.cpp

@@ -287,7 +287,7 @@ static RPCHelpMan addnode()
                 strprintf("Addnode connections are limited to %u at a time", MAX_ADDNODE_CONNECTIONS) +
                 " and are counted separately from the -maxconnections limit.\n",
                 {
-                    {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node (see getpeerinfo for nodes)"},
+                    {"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"},
                 },
                 RPCResult{RPCResult::Type::NONE, "", ""},
@@ -297,10 +297,8 @@ static RPCHelpMan addnode()
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    std::string strCommand;
+    const std::string command{request.params[1].get_str()};
-    if (!request.params[1].isNull())
+    if (command != "onetry" && command != "add" && command != "remove") {
-        strCommand = request.params[1].get_str();
-    if (strCommand != "onetry" && strCommand != "add" && strCommand != "remove") {
         throw std::runtime_error(
             self.ToString());
     }
@@ -308,24 +306,24 @@ static RPCHelpMan addnode()
     NodeContext& node = EnsureAnyNodeContext(request.context);
     CConnman& connman = EnsureConnman(node);
 
-    std::string strNode = request.params[0].get_str();
+    const std::string node_arg{request.params[0].get_str()};
 
-    if (strCommand == "onetry")
+    if (command == "onetry")
     {
         CAddress addr;
-        connman.OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), ConnectionType::MANUAL);
+        connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grantOutbound=*/nullptr, node_arg.c_str(), ConnectionType::MANUAL);
         return UniValue::VNULL;
     }
 
-    if (strCommand == "add")
+    if (command == "add")
     {
-        if (!connman.AddNode(strNode)) {
+        if (!connman.AddNode(node_arg)) {
             throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Node already added");
         }
     }
-    else if(strCommand == "remove")
+    else if (command == "remove")
     {
-        if (!connman.RemoveAddedNode(strNode)) {
+        if (!connman.RemoveAddedNode(node_arg)) {
             throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: Node could not be removed. It has not been added previously.");
         }
     }

test/functional/rpc_net.py

@@ -61,7 +61,7 @@ class NetTest(BitcoinTestFramework):
         self.test_getpeerinfo()
         self.test_getnettotals()
         self.test_getnetworkinfo()
-        self.test_getaddednodeinfo()
+        self.test_addnode_getaddednodeinfo()
         self.test_service_flags()
         self.test_getnodeaddresses()
         self.test_addpeeraddress()
@@ -205,8 +205,8 @@ class NetTest(BitcoinTestFramework):
         # Check dynamically generated networks list in getnetworkinfo help output.
         assert "(ipv4, ipv6, onion, i2p, cjdns)" in self.nodes[0].help("getnetworkinfo")
 
-    def test_getaddednodeinfo(self):
+    def test_addnode_getaddednodeinfo(self):
-        self.log.info("Test getaddednodeinfo")
+        self.log.info("Test addnode and getaddednodeinfo")
         assert_equal(self.nodes[0].getaddednodeinfo(), [])
         # add a node (node2) to node0
         ip_port = "127.0.0.1:{}".format(p2p_port(2))
@@ -220,6 +220,8 @@ class NetTest(BitcoinTestFramework):
         # check that node can be removed
         self.nodes[0].addnode(node=ip_port, command='remove')
         assert_equal(self.nodes[0].getaddednodeinfo(), [])
+        # check that an invalid command returns an error
+        assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
         # check that trying to remove the node again returns an error
         assert_raises_rpc_error(-24, "Node could not be removed", self.nodes[0].addnode, node=ip_port, command='remove')
         # check that a non-existent node returns an error