Skip to content

Commit

Permalink
Merge bitcoin#28998: rpc: "addpeeraddress tried" return error on failure
Browse files Browse the repository at this point in the history 
99954f9 test: fix test to ensure hidden RPC is present in detailed help (stratospher)
0d01f6f test: remove unused mocktime in test_addpeeraddress (0xb10c)
6205466 rpc: "addpeeraddress tried" return error on failure (0xb10c)

Pull request description:

  When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue bitcoin#28964.

  This is fixed by new returning `{ "success":  false, "error": "..." }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. This is done in the functional test for the `getrawaddrman` RPC.

  Fixes bitcoin#28964

ACKs for top commit:
  achow101:
    ACK 99954f9
  stratospher:
    reACK 99954f9. 🚀
  brunoerg:
    utACK 99954f9

Tree-SHA512: 2f1299410c0582ebc2071271ba789a8abed905f9a510821f77afbcf2a555ec31397578ea55cbcd162fb828be27afedd3246c7b13ad8883f2f745bb8e04364a76
  • Loading branch information
@achow101
achow101 committed Mar 22, 2024
2 parents a175efe + 99954f9 commit 2795e89
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 21 deletions.
12 changes: 9 additions & 3 deletions src/rpc/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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{
Expand Down Expand Up @@ -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");
}
}

Expand Down
60 changes: 42 additions & 18 deletions test/functional/rpc_net.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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]
Expand Down Expand Up @@ -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"""
Expand Down

0 comments on commit 2795e89

Please sign in to comment.