From c28b05c5ca1d2b85687fa5e481e32208e7d48adc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 18:17:32 +0000 Subject: [PATCH] merge bitcoin#22831: add addpeeraddress "tried", test addrman checks on restart with asmap We need to continue inheriting the existing set of arguments to prevent block invalidation due to missing arguments that allow for fast DIP3 activation (will manifest as `bad-qc-premature`) --- src/rpc/client.cpp | 1 + src/rpc/net.cpp | 14 +++++++++++--- test/functional/feature_asmap.py | 29 ++++++++++++++++++++++++++-- test/functional/rpc_net.py | 33 ++++++++++++++++++++++++-------- 4 files changed, 64 insertions(+), 13 deletions(-) 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()