Repository: bitcoin/bitcoin (language: cpp)

Issue:
rpc_net.py intemittent failure /                                        assert_equal(len(getrawaddrman[table_name]), len(table_info["entries"]))
https://cirrus-ci.com/task/5884375213015040?logs=ci#L4022

```
 test  2023-11-28T23:28:31.543000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_net.py", line 71, in run_test
                                       self.test_getrawaddrman()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_net.py", line 506, in test_getrawaddrman
                                       check_getrawaddrman_entries(expected)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_net.py", line 424, in check_getrawaddrman_entries
                                       assert_equal(len(getrawaddrman[table_name]), len(table_info["entries"]))
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
                                   AssertionError: not(3 == 2)


Discussion / hints:
I'm taking a look at this.
```
node1 [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=addpeeraddress user=__cookie__ 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 2, tried 1, total 3 started 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.60ms) 
node1 [httpworker.2] [addrman.cpp:615] [AddSingle] [addrman] Added nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion:0 to new[238][20] 
node1 [httpworker.2] [addrman.cpp:687] [Add_] [addrman] Added 1 addresses (of 1) from nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion: 1 tried, 3 new 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 3, tried 1, total 4 started 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.87ms) 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 3, tried 1, total 4 started 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.87ms) 
node1 [httpworker.2] [addrman.cpp:668] [Good_] [addrman] Collision with 1.2.3.4:8333 while attempting to move nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion:0 to tried table. Collisions=1 
node1 [httpworker.2] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 3, tried 1, total 4 started 
test TestFramework (DEBUG): Added nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion to tried table 
test TestFramework (DEBUG): Test that the newly added addresses appear in getrawaddrman 
```

seems like this didn't work:

https://github.com/bitcoin/bitcoin/blob/8cf2137dbeaa681f0178f1e2fd70b639e6d4f717/test/functional/rpc_net.py#L492-L504

Did `addpeeraddress` return `{"success": true}` even if there was a collision while moving to tried? 

(still investigating)
I think that the problem is a rare collision in the tried table: 
` node1 2023-11-28T23:28:31.447418Z (mocktime: 2023-11-28T23:28:28Z) [httpworker.2] [addrman.cpp:668] [Good_] [addrman] Collision with 1.2.3.4:8333 while attempting to move nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion:0 to tried table. Collisions=1 `:

`addpeeraddress` reports success if the first part (adding to new) succeeded, but the second part (moving to tried) fails due to a collision (so that the addr stays in new).
@mzumsande is there a reason why `addpeeraddress` should return true if it fails to put in in the table we asked it to do? Sounds like a bug to me. Or, it should at least be documented clearly.

https://github.com/bitcoin/bitcoin/blob/7bc8c5312bf51edd3d85e1599b5d6ae32c8fd7b5/src/rpc/net.cpp#L981-L987
I don't think there is a reason, but returning `false` wouldn't be great either. It might be best could return an error code with 3 values (not added to new; added to new but failed to move to tried; everything successful).

Fix patch:
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 3fa2b18495314..f935a3b08f490 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");
         }
     }
 


Test patch:
diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 22789644f2733..48d86ab59dd99 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -324,10 +324,10 @@ def test_addpeeraddress(self):
         self.restart_node(1, ["-checkaddrman=1", "-test=addrman"], clear_addrman=True)
         node = self.nodes[1]
 
-        self.log.debug("Test that addpeerinfo is a hidden RPC")
+        self.log.debug("Test that addpeeraddress is a hidden RPC")
         # It is hidden from general help, but its detailed help may be called directly.
-        assert "addpeerinfo" not in node.help()
-        assert "addpeerinfo" in node.help("addpeerinfo")
+        assert "addpeeraddress" not in node.help()
+        assert "unknown command: addpeeraddress" not in node.help("addpeeraddress")
 
         self.log.debug("Test that adding an empty address fails")
         assert_equal(node.addpeeraddress(address="", port=8333), {"success": False})
@@ -340,26 +340,50 @@ def test_addpeeraddress(self):
         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)
-
-        self.log.debug("Test that adding a second address, this time to the new table, succeeds")
+            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 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]
@@ -428,7 +452,7 @@ def test_getrawaddrman(self):
         self.log.debug("Test that getrawaddrman is a hidden RPC")
         # It is hidden from general help, but its detailed help may be called directly.
         assert "getrawaddrman" not in node.help()
-        assert "getrawaddrman" in node.help("getrawaddrman")
+        assert "unknown command: getrawaddrman" not in node.help("getrawaddrman")
 
         def check_addr_information(result, expected):
             """Utility to compare a getrawaddrman result entry with an expected entry"""