From 6205466512d4b94d1e507a77ab2151425790d29f Mon Sep 17 00:00:00 2001 From: 0xb10c Date: Mon, 4 Dec 2023 15:56:40 +0100 Subject: [PATCH] rpc: "addpeeraddress tried" return error on failure When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue 28964. This is fixed by returning `{ "success": false, "error": "failed-adding-to-tried" }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. Also: To check the number of addresses in each addrman table, the addrman checks were re-run and the log output of this check was asserted. Ideally, logs shouldn't be used as an interface in automated tests. To avoid asserting the logs, use the getaddrmaninfo and getrawaddrman RPCs (which weren't implemented when the test was added). Removing the "getnodeaddress" calls would also remove the addrman checks from the test, which could reduce the test coverage. To avoid this, these are kept. --- src/rpc/net.cpp | 12 +++++++--- test/functional/rpc_net.py | 48 +++++++++++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 3fa2b184953..f935a3b08f4 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -951,7 +951,7 @@ static RPCHelpMan getnodeaddresses() static RPCHelpMan addpeeraddress() { return RPCHelpMan{"addpeeraddress", - "\nAdd the address of a potential peer to the address manager. This RPC is for testing only.\n", + "Add the address of a potential peer to an address manager table. This RPC is for testing only.", { {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address of the peer"}, {"port", RPCArg::Type::NUM, RPCArg::Optional::NO, "The port of the peer"}, @@ -960,7 +960,8 @@ static RPCHelpMan addpeeraddress() RPCResult{ RPCResult::Type::OBJ, "", "", { - {RPCResult::Type::BOOL, "success", "whether the peer address was successfully added to the address manager"}, + {RPCResult::Type::BOOL, "success", "whether the peer address was successfully added to the address manager table"}, + {RPCResult::Type::STR, "error", /*optional=*/true, "error description, if the address could not be added"}, }, }, RPCExamples{ @@ -989,8 +990,13 @@ static RPCHelpMan addpeeraddress() success = true; if (tried) { // Attempt to move the address to the tried addresses table. - addrman.Good(address); + if (!addrman.Good(address)) { + success = false; + obj.pushKV("error", "failed-adding-to-tried"); + } } + } else { + obj.pushKV("error", "failed-adding-to-new"); } } diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 22789644f27..e50aeebad26 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -340,26 +340,52 @@ class NetTest(BitcoinTestFramework): assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=-1) assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=65536) + self.log.debug("Test that adding a valid address to the new table succeeds") + assert_equal(node.addpeeraddress(address="1.0.0.0", tried=False, port=8333), {"success": True}) + addrman = node.getrawaddrman() + assert_equal(len(addrman["tried"]), 0) + new_table = list(addrman["new"].values()) + assert_equal(len(new_table), 1) + assert_equal(new_table[0]["address"], "1.0.0.0") + assert_equal(new_table[0]["port"], 8333) + + self.log.debug("Test that adding an already-present new address to the new and tried tables fails") + for value in [True, False]: + assert_equal(node.addpeeraddress(address="1.0.0.0", tried=value, port=8333), {"success": False, "error": "failed-adding-to-new"}) + assert_equal(len(node.getnodeaddresses(count=0)), 1) + self.log.debug("Test that adding a valid address to the tried table succeeds") self.addr_time = int(time.time()) node.setmocktime(self.addr_time) assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True}) - with node.assert_debug_log(expected_msgs=["CheckAddrman: new 0, tried 1, total 1 started"]): - addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks - assert_equal(len(addrs), 1) - assert_equal(addrs[0]["address"], "1.2.3.4") - assert_equal(addrs[0]["port"], 8333) + addrman = node.getrawaddrman() + assert_equal(len(addrman["new"]), 1) + tried_table = list(addrman["tried"].values()) + assert_equal(len(tried_table), 1) + assert_equal(tried_table[0]["address"], "1.2.3.4") + assert_equal(tried_table[0]["port"], 8333) + node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks self.log.debug("Test that adding an already-present tried address to the new and tried tables fails") for value in [True, False]: - assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False}) - assert_equal(len(node.getnodeaddresses(count=0)), 1) + assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False, "error": "failed-adding-to-new"}) + assert_equal(len(node.getnodeaddresses(count=0)), 2) - self.log.debug("Test that adding a second address, this time to the new table, succeeds") + self.log.debug("Test that adding an address, which collides with the address in tried table, fails") + colliding_address = "1.2.5.45" # grinded address that produces a tried-table collision + assert_equal(node.addpeeraddress(address=colliding_address, tried=True, port=8333), {"success": False, "error": "failed-adding-to-tried"}) + # When adding an address to the tried table, it's first added to the new table. + # As we fail to move it to the tried table, it remains in the new table. + addrman_info = node.getaddrmaninfo() + assert_equal(addrman_info["all_networks"]["tried"], 1) + assert_equal(addrman_info["all_networks"]["new"], 2) + + self.log.debug("Test that adding an another address to the new table succeeds") assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333), {"success": True}) - with node.assert_debug_log(expected_msgs=["CheckAddrman: new 1, tried 1, total 2 started"]): - addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks - assert_equal(len(addrs), 2) + addrman_info = node.getaddrmaninfo() + assert_equal(addrman_info["all_networks"]["tried"], 1) + assert_equal(addrman_info["all_networks"]["new"], 3) + node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks def test_sendmsgtopeer(self): node = self.nodes[0]