test: add addpeeraddress "tried", test addrman checks on restart with asmap

src/rpc/client.cpp

@@ -192,6 +192,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "unloadwallet", 1, "load_on_startup"},
     { "getnodeaddresses", 0, "count"},
     { "addpeeraddress", 1, "port"},
+    { "addpeeraddress", 2, "tried"},
     { "stop", 0, "wait" },
 };
 // clang-format on

src/rpc/net.cpp

@@ -921,6 +921,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, RPCArg::Default{false}, "If true, attempt to add the peer to the tried addresses table"},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
@@ -929,8 +930,8 @@ static RPCHelpMan addpeeraddress()
             },
         },
         RPCExamples{
-            HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 8333")
-    + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 8333")
+            HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 8333 true")
+    + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 8333, true")
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
@@ -941,6 +942,7 @@ static RPCHelpMan addpeeraddress()
 
     const std::string& addr_string{request.params[0].get_str()};
     const uint16_t port{static_cast<uint16_t>(request.params[1].get_int())};
+    const bool tried{request.params[2].isTrue()};
 
     UniValue obj(UniValue::VOBJ);
     CNetAddr net_addr;
@@ -951,7 +953,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);

test/functional/feature_asmap.py

@@ -14,9 +14,11 @@
 
 4. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
 
-5. `bitcoind -asmap` with no file specified and a missing default asmap file
+5. `bitcoind -asmap` restart with an addrman containing new and tried entries
 
-6. `bitcoind -asmap` with an empty (unparsable) default asmap file
+6. `bitcoind -asmap` with no file specified and a missing default asmap file
+
+7. `bitcoind -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 bitcoind with no -asmap arg passed')
@@ -72,6 +80,22 @@ def test_default_asmap(self):
                 self.start_node(0, [arg])
         os.remove(self.default_asmap)
 
+    def test_asmap_interaction_with_addrman_containing_entries(self):
+        self.log.info("Test bitcoind -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 bitcoind -asmap with missing default map file')
         self.stop_node(0)
@@ -97,6 +121,7 @@ def run_test(self):
         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()
 

test/functional/rpc_net.py

@@ -239,7 +239,16 @@ def test_getnodeaddresses(self):
         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, ["-checkaddrman=1"])
         node = self.nodes[1]
 
         self.log.debug("Test that addpeerinfo is a hidden RPC")
@@ -251,17 +260,25 @@ def test_addpeeraddress(self):
         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 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 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 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()