diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index be89fea835..8af1bcf441 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -225,6 +225,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "upgradetohd", 3, "rescan"}, { "getnodeaddresses", 0, "count"}, { "addpeeraddress", 1, "port"}, + { "addpeeraddress", 2, "tried"}, { "stop", 0, "wait" }, { "verifychainlock", 2, "blockHeight" }, { "verifyislock", 3, "maxHeight" }, diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 29b99c9e51..b8bdd38939 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -970,6 +970,7 @@ static RPCHelpMan addpeeraddress() { {"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"}, + {"tried", RPCArg::Type::BOOL, /* default */ "false", "If true, attempt to add the peer to the tried addresses table"}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -978,8 +979,8 @@ static RPCHelpMan addpeeraddress() }, }, RPCExamples{ - HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 9999") - + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 9999") + HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 9999 true") + + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 9999, true") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { @@ -991,6 +992,7 @@ static RPCHelpMan addpeeraddress() const std::string& addr_string{request.params[0].get_str()}; const uint16_t port = request.params[1].get_int(); + const bool tried{request.params[2].isTrue()}; UniValue obj(UniValue::VOBJ); CNetAddr net_addr; @@ -1001,7 +1003,13 @@ static RPCHelpMan addpeeraddress() address.nTime = GetAdjustedTime(); // The source address is set equal to the address. This is equivalent to the peer // announcing itself. - if (node.addrman->Add({address}, address)) success = true; + if (node.addrman->Add({address}, address)) { + success = true; + if (tried) { + // Attempt to move the address to the tried addresses table. + node.addrman->Good(address); + } + } } obj.pushKV("success", success); diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py index 0eb998fab7..029d6555c6 100755 --- a/test/functional/feature_asmap.py +++ b/test/functional/feature_asmap.py @@ -14,9 +14,11 @@ Verify node behaviour and debug log when launching dashd in these cases: 4. `dashd -asmap/-asmap=` with no file specified, using the default asmap -5. `dashd -asmap` with no file specified and a missing default asmap file +5. `dashd -asmap` restart with an addrman containing new and tried entries -6. `dashd -asmap` with an empty (unparsable) default asmap file +6. `dashd -asmap` with no file specified and a missing default asmap file + +7. `dashd -asmap` with an empty (unparsable) default asmap file The tests are order-independent. @@ -37,6 +39,12 @@ def expected_messages(filename): class AsmapTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 + self.extra_args = [["-checkaddrman=1"]] # Do addrman checks on all operations. + + def fill_addrman(self, node_id): + """Add 2 tried addresses to the addrman, followed by 2 new addresses.""" + for addr, tried in [[0, True], [1, True], [2, False], [3, False]]: + self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333) def test_without_asmap_arg(self): self.log.info('Test dashd with no -asmap arg passed') @@ -72,6 +80,22 @@ class AsmapTest(BitcoinTestFramework): self.start_node(0, [arg]) os.remove(self.default_asmap) + def test_asmap_interaction_with_addrman_containing_entries(self): + self.log.info("Test dashd -asmap restart with addrman containing new and tried entries") + self.stop_node(0) + shutil.copyfile(self.asmap_raw, self.default_asmap) + self.start_node(0, ["-asmap", "-checkaddrman=1"]) + self.fill_addrman(node_id=0) + self.restart_node(0, ["-asmap", "-checkaddrman=1"]) + with self.node.assert_debug_log( + expected_msgs=[ + "Addrman checks started: new 2, tried 2, total 4", + "Addrman checks completed successfully", + ] + ): + self.node.getnodeaddresses() # getnodeaddresses re-runs the addrman checks + os.remove(self.default_asmap) + def test_default_asmap_with_missing_file(self): self.log.info('Test dashd -asmap with missing default map file') self.stop_node(0) @@ -97,6 +121,7 @@ class AsmapTest(BitcoinTestFramework): self.test_asmap_with_absolute_path() self.test_asmap_with_relative_path() self.test_default_asmap() + self.test_asmap_interaction_with_addrman_containing_entries() self.test_default_asmap_with_missing_file() self.test_empty_asmap() diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index aebd9e1672..f09e6031fb 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -257,7 +257,16 @@ class NetTest(DashTestFramework): assert_raises_rpc_error(-8, "Network not recognized: Foo", self.nodes[0].getnodeaddresses, 1, "Foo") def test_addpeeraddress(self): + """RPC addpeeraddress sets the source address equal to the destination address. + If an address with the same /16 as an existing new entry is passed, it will be + placed in the same new bucket and have a 1/64 chance of the bucket positions + colliding (depending on the value of nKey in the addrman), in which case the + new address won't be added. The probability of collision can be reduced to + 1/2^16 = 1/65536 by using an address from a different /16. We avoid this here + by first testing adding a tried table entry before testing adding a new table one. + """ self.log.info("Test addpeeraddress") + self.restart_node(1, self.extra_args[1] + ["-checkaddrman=1"]) node = self.nodes[1] self.log.debug("Test that addpeerinfo is a hidden RPC") @@ -269,17 +278,25 @@ class NetTest(DashTestFramework): assert_equal(node.addpeeraddress(address="", port=8333), {"success": False}) assert_equal(node.getnodeaddresses(count=0), []) - self.log.debug("Test that adding a valid address succeeds") - assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": True}) - addrs = node.getnodeaddresses(count=0) - assert_equal(len(addrs), 1) - assert_equal(addrs[0]["address"], "1.2.3.4") - assert_equal(addrs[0]["port"], 8333) + self.log.debug("Test that adding a valid address to the tried table succeeds") + assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True}) + with node.assert_debug_log(expected_msgs=["Addrman checks started: new 0, tried 1, total 1"]): + 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) - self.log.debug("Test that adding the same address again when already present fails") - assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": False}) + 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) + self.log.debug("Test that adding a second address, this time 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=["Addrman checks started: new 1, tried 1, total 2"]): + addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks + assert_equal(len(addrs), 2) + if __name__ == '__main__': NetTest().main()