From 56b27b84877376ffc32b3bad09f1047b23de4ba1 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 21 Oct 2022 15:03:40 -0300 Subject: [PATCH 1/3] rpc, refactor: clean-up `addnode` 1. Use const where possible; 2. Rename variables to make them clearer; 3. There is no need to check whether `command` is null since it's a non-optional field. --- src/rpc/net.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index a2a46ef32f8..da511d1ed94 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -297,10 +297,8 @@ static RPCHelpMan addnode() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::string strCommand; - if (!request.params[1].isNull()) - strCommand = request.params[1].get_str(); - if (strCommand != "onetry" && strCommand != "add" && strCommand != "remove") { + const std::string command{request.params[1].get_str()}; + if (command != "onetry" && command != "add" && command != "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."); } } From effd1efefb53c58f0e43fec4f019a19f97795553 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 21 Oct 2022 15:41:35 -0300 Subject: [PATCH 2/3] test: `addnode` with an invalid command should throw an error --- test/functional/rpc_net.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 5fdd5daddf7..5260656c63f 100755 --- a/test/functional/rpc_net.py +++ b/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() @@ -203,8 +203,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): - self.log.info("Test getaddednodeinfo") + def test_addnode_getaddednodeinfo(self): + 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)) @@ -218,6 +218,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 From f52cb02f700b58bca921a7aa24bfeee04760262b Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 21 Oct 2022 15:56:18 -0300 Subject: [PATCH 3/3] doc: make it clear that `node` in `addnode` refers to the node's address --- src/rpc/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index da511d1ed94..4a24e772e3b 100644 --- a/src/rpc/net.cpp +++ b/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, "", ""},