Merge bitcoin/bitcoin#22831: test: add addpeeraddress "tried", test addrman checks on restart with asmap

cdaab90662a54e331de0e49a89596bbb94a8ac45 Add test for addrman consistency check on restart with asmap (Jon Atack)
869f136816c6900ce84bc4b5a9c93c0deab85193 Add test for rpc addpeeraddress with "tried" argument (Jon Atack)
ef242f52137f2a79a739447251d7759bd4705be0 Allow passing "tried" to rpc addpeeraddress to call CAddrMan::Good() (Jon Atack)

Pull request description:

  This pull adds a `tried` argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue.

  PR #22697 introduced a reproducible bug in commit 181a1207 that fails addrman consistency checks and causes it to significantly lose peer entries when the `-asmap` configuration option is used.

  The issue occurs upon bitcoind restart due to an initialization order change in `src/init.cpp` in that commit, whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before.

  Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.

  ```
  addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses
  ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted
  ```

  How to reproduce:

  - `git checkout 181a1207`, build, and launch bitcoind with the `-asmap` and `-checkaddrman=1` configuration options enabled
  - restart bitcoind
  - bitcoind aborts on the second call to the addrman consistency checks in `CAddrMan::Check()`

  How to test this pull:

  - `git checkout 181a1207`, cherry pick the first commit of this branch, build, git checkout this branch, run `test/functional/rpc_net.py`, which should pass, and then run `test/functional/feature_asmap.py`, which should fail with the following output:

  ```
  AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed.
  ```

ACKs for top commit:
  jnewbery:
    utACK cdaab90662a54e331de0e49a89596bbb94a8ac45
  mzumsande:
    re-ACK cdaab90662a54e331de0e49a89596bbb94a8ac45 (based on code review of diff to d586817)
  vasild:
    ACK cdaab90662a54e331de0e49a89596bbb94a8ac45

Tree-SHA512: 0251a18fea629b62486fc907d7ab0e96c6df6fadb9e4d62cff018bc681afb6ac31e0e7258809c0a88f91e4a36c4fb0b16ed294ce47ef30585217de89c3342399

4 files changed, 65 insertions, 14 deletions

diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 4357ab2bb3..d6943e066a 100644
--- a/src/rpc/client.cpp
+++ b/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
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 0f554ec5e7..227eec722f 100644
--- a/src/rpc/net.cpp
+++ b/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);
diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py
index 704dd6126b..2dc1e3a7cb 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 bitcoind in these cases:
 
 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 @@ 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 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 @@ 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 aa53e354a3..3fcca97cb7 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -239,7 +239,16 @@ class NetTest(BitcoinTestFramework):
         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 @@ class NetTest(BitcoinTestFramework):
         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()