cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency #26988

stratospher commented at 9:25 PM on January 29, 2023: contributor

Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in #27511. This would be useful for users who want to know the total number of addresses the node knows about and can make connections to.

Currently, -addrinfo returns total number of addresses the node knows about after filtering them for quality + recency using isTerrible. However isTerribleaddresses don't matter when selecting outbound peers to connect to. Total number of addresses the node knows about could be higher than what -addrinfo currently displays. See #24370.

DrahtBot commented at 9:25 PM on January 29, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26988.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, vasild, ajtowns, sr-gi
Concept ACK	mzumsande, brunoerg
Approach ACK	jonatack, sipa
Stale ACK	amitiuttarwar

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

[JSONRPCRequestObj("getaddrmaninfo", NullUniValue, 1)] in src/bitcoin-cli.cpp

stratospher force-pushed on Jan 30, 2023

in test/functional/rpc_net.py:343 in caab2131d6 outdated

 338 | +        self.log.info("Test addrmaninfo")
 339 | +        node = self.nodes[1]
 340 | +
 341 | +        # current ipv4 count in node's Addrman: new 1, tried 1
 342 | +        self.log.debug("Test that addrmaninfo provides correct new/tried table address count")
 343 | +        node.addpeeraddress(address="3.1.1.1", tried=True, port=8333)

mzumsande commented at 8:57 PM on January 30, 2023:

I think that this can fail intermittently, because of a small chance that the second address conflicts with the existing one, and then the counts don't match. Since the nKey of AddrMan is randomly chosen in each run, it will only fail once in a while - see also #23084 with the same problem. Maybe just check the counts of the existing addresses for now.

This whole thing is really unfortunate, but unless we add a config option to bitcoind to make AddrMan deterministic it seems that we are quite limited in what we can do with AddrMan in functional tests.

mzumsande commented at 10:34 PM on February 6, 2023:

Maybe it would actually make sense to add the possibility of a deterministic addrman as, it e.g. we could possibly extend the existing -addrmantest test-only RPC. But no need to do it in this PR.

stratospher commented at 9:00 AM on February 7, 2023:

interesting, thanks! so that's why the ci was failing. i've updated the PR to reflect this.

stratospher commented at 4:21 PM on March 13, 2023:

adding addresses from different networks wouldn't cause these kind of collisions because GetGroup() would differ right?

mzumsande commented at 6:23 PM on March 13, 2023:

I don't think so - even with the guarantee that GetGroup() is different, there is still a small chance that two addresses happen to hash to the same bucket in GetNewBucket().

amitiuttarwar commented at 10:34 PM on March 13, 2023:

to add a second address, the test could do something like...

while (len(node.getnodeaddresses(count=0)) < 2) {
	node.addpeeraddress(...)
}

stratospher commented at 6:45 PM on March 15, 2023:

discussed this more with @amitiuttarwar and understood that nKey would remain the same for a node's addrman and hence same bucket position.

i tried generating random ipv6 addresses and inserting into addrman:

while len(node.getnodeaddresses(count=0)) < 3:
            ipv6_address = ":".join(format(randint(0, 0xffff), '04x') for _ in range(8))
            node.addpeeraddress(address=ipv6_address, tried=False, port=8333)

#26988 (review) would still be a concern since there's a small chance that an ipv6 address could hash to the same bucket as an ipv4 address. this would cause the assertions which check the count of different network addresses in the test to fail.

so leaving out the network coverage tests for now.

0xB10C commented at 12:37 PM on September 23, 2023:

~fyi: addpeeraddress returns success indicating if inserting was successful. I'm using this to retry inserting in case there was a collision.~

added = False
while not added:
    result = node.addpeeraddress(address="..", tried=to_tried, port=port)
    added = result["success"]

edit: nope, simply retrying doesn't work. We also need to increase the port on a collision to hash to a different position in the bucket. A new port doesn't change the new_table bucket but changes the tried_table bucket.

mzumsande commented at 9:02 PM on January 30, 2023: contributor

Concept ACK, will review soon.

stratospher force-pushed on Feb 6, 2023

amitiuttarwar commented at 6:54 PM on February 6, 2023: contributor

obvious concept ack considering I proposed #26907

in src/rpc/net.cpp:980 in 418f811853 outdated

 973 | @@ -974,6 +974,60 @@ static RPCHelpMan addpeeraddress()
 974 |      };
 975 |  }
 976 |  
 977 | +static RPCHelpMan addrmaninfo()
 978 | +{
 979 | +    return RPCHelpMan{"addrmaninfo",
 980 | +                      "\nReturns count of addresses in new/tried table of all networks(or a given network). This RPC is for testing only.\n",

mzumsande commented at 9:38 PM on February 6, 2023:

nit: space before "("

stratospher commented at 9:00 AM on February 7, 2023:

done.

in src/bitcoin-cli.cpp:267 in c098f1c5a9 outdated

 275 |      UniValue ProcessReply(const UniValue& reply) override
 276 |      {
 277 |          if (!reply["error"].isNull()) return reply;
 278 | -        const std::vector<UniValue>& nodes{reply["result"].getValues()};
 279 | -        if (!nodes.empty() && nodes.at(0)["network"].isNull()) {
 280 | +        const std::vector<UniValue>& addrman{reply["result"].getValues()};

mzumsande commented at 10:08 PM on February 6, 2023:

I'd prefer a different name (maybe addr_counts or addrman_counts) to avoid confusion that the object isn't addrman, but the result of a query to addrman.

stratospher commented at 9:00 AM on February 7, 2023:

that's true. i've updated the PR.

mzumsande commented at 10:33 PM on February 6, 2023: contributor

Looks good to me, only minor nits.

ping @jonatack, could you have a look, especially at the changes to bitcoin-cli?

stratospher force-pushed on Feb 7, 2023

fanquake renamed this:
~~[rpc]: Add test-only RPC addrmaninfo for new/tried table address count~~
rpc: Add test-only RPC addrmaninfo for new/tried table address count
on Feb 7, 2023

DrahtBot added the label RPC/REST/ZMQ on Feb 7, 2023

in src/rpc/net.cpp:977 in 35bd55c378 outdated

 973 | @@ -974,6 +974,60 @@ static RPCHelpMan addpeeraddress()
 974 |      };
 975 |  }
 976 |  
 977 | +static RPCHelpMan addrmaninfo()

amitiuttarwar commented at 4:57 AM on February 8, 2023:

realized it probably makes more sense to call it getaddrmaninfo to be consistent with other RPC messages

stratospher commented at 10:03 AM on February 22, 2023:

true, i've updated the PR to call it getaddrmaninfo.

DrahtBot added the label Needs rebase on Feb 15, 2023

stratospher force-pushed on Feb 21, 2023

DrahtBot removed the label Needs rebase on Feb 21, 2023

stratospher renamed this:
~~rpc: Add test-only RPC addrmaninfo for new/tried table address count~~
rpc: Add test-only RPC getaddrmaninfo for new/tried table address count
on Feb 22, 2023

brunoerg commented at 11:55 AM on February 22, 2023: contributor

Concept ACK

in src/rpc/net.cpp:1000 in a04bfa375b outdated

 995 | +                              throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled");
 996 | +                          }
 997 | +
 998 | +                          const std::optional<Network> network{request.params[0].isNull() ? std::nullopt : std::optional<Network>{ParseNetwork(request.params[0].get_str())}};
 999 | +                          if (network == NET_UNROUTABLE) {
1000 | +                              throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Network not recognized: %s", request.params[0].get_str()));

brunoerg commented at 11:57 AM on February 22, 2023:

You could cover this error in the functional test

e.g.:

diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 68833fb85..90a9d9a17 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -337,5 +337,8 @@ class NetTest(BitcoinTestFramework):
         assert_equal(res[0]["new"], 1)
         assert_equal(res[0]["tried"], 1)
 
+        self.log.debug("Test that getaddrman with an invalid network throws an error")
+        assert_raises_rpc_error(-8, "Network not recognized: ipv8", self.nodes[1].getaddrmaninfo, "ipv8")
+
 if __name__ == '__main__':
     NetTest().main()

stratospher commented at 7:35 PM on March 7, 2023:

thanks! done.

brunoerg commented at 1:23 PM on February 22, 2023: contributor

"network" : "str",    (string) The network (ipv4, ipv6, onion, i2p, cjdns)

Instead of being a string, wouldn't make sense it to be an array? E.g. I want to get new/tried table address count for ipv4 and ipv6 together.

stratospher force-pushed on Mar 7, 2023

stratospher commented at 7:45 PM on March 7, 2023: contributor

Instead of being a string, wouldn't make sense it to be an array? E.g. I want to get new/tried table address count for ipv5 and ipv6 together.

i'm confused about this since making it into RPCArg::Type::ARR would make the RPC harder to type and use? $ bitcoin-cli getaddrmaninfo "[\"ipv4\", \"ipv6\"]"

and there are only few network types. would be interested in what others think.

amitiuttarwar commented at 3:50 AM on March 9, 2023: contributor

to minimize the complexity that we need to maintain over time, we try to reduce the amount of client-side niftiness that we offer. it seems to me that the string would offer all the information needed for a client to write a simple query if they wanted the sum of multiple. agreed that the syntax is another challenge of the array type

so I am +1 to leaving as is

brunoerg commented at 2:11 PM on March 9, 2023: contributor

that was just a question, thinking about complexity I agree on leaving it as is.

going to review in depth soon.

in test/functional/rpc_net.py:332 in 7c34c35b47 outdated

 328 | @@ -328,6 +329,15 @@ def test_addpeeraddress(self):
 329 |              addrs = node.getnodeaddresses(count=0)  # getnodeaddresses re-runs the addrman checks
 330 |              assert_equal(len(addrs), 2)
 331 |  
 332 | +    def test_getaddrmaninfo(self):

amitiuttarwar commented at 6:40 PM on March 9, 2023:

you could add coverage for getaddrmaninfo with a network arg passed through

stratospher commented at 4:15 PM on March 13, 2023:

done.

amitiuttarwar commented at 7:01 PM on March 9, 2023: contributor

light code review ACK 7c34c35b47. tested that the RPC and cli endpoints make sense & handle errors reasonably. these changes will require release notes, which can be done here or in a separate PR.

brunoerg commented at 2:05 PM on March 10, 2023: contributor

I agree on having test coverage for it with a network arg, something like:

diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 3d39fb47d..6928c1211 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -331,11 +331,23 @@ class NetTest(BitcoinTestFramework):
 
     def test_getaddrmaninfo(self):
         self.log.info("Test getaddrmaninfo")
-        # current ipv4 count in node 1's Addrman: new 1, tried 1
         self.log.debug("Test that getaddrmaninfo provides correct new/tried table address count")
+
+        ipv6_addr = "1233:3432:2434:2343:3234:2345:6546:4534"
+        self.nodes[1].addpeeraddress(address=ipv6_addr, port=8333)
+
+        network_count = {
+            '': { 'new': 2, 'tried': 1 },
+            'ipv6': { 'new': 1, 'tried': 0},
+            'ipv4': { 'new': 1, 'tried': 1}
+        }
+
         res = self.nodes[1].getaddrmaninfo()
-        assert_equal(res[0]["new"], 1)
-        assert_equal(res[0]["tried"], 1)
+        for network, count in network_count.items():
+            res = self.nodes[1].getaddrmaninfo(network=network) if network else self.nodes[1].getaddrmaninfo()
+            assert_equal(sum(map(lambda x: int(x['new']), res)), count["new"])
+            assert_equal(sum(map(lambda x: int(x['tried']), res)), count["tried"])
+
         self.log.debug("Test that getaddrmaninfo with an invalid network throws an error")
         assert_raises_rpc_error(-8, "Network not recognized: ipv8", self.nodes[1].getaddrmaninfo, "ipv8")

could also test other networks but I am not sure if addpeeraddress works well with that.

brunoerg commented at 4:11 PM on March 10, 2023: contributor

perhaps we could also check this is a hidden RPC like addpeerinfo?

self.log.debug("Test that addpeerinfo 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")

stratospher referenced this in commit 91cf49700e on Mar 13, 2023

stratospher force-pushed on Mar 13, 2023

stratospher referenced this in commit bca0e42775 on Mar 13, 2023

stratospher force-pushed on Mar 13, 2023

stratospher commented at 4:14 PM on March 13, 2023: contributor

thank you @amitiuttarwar and @brunoerg! I've updated the PR to include your suggestions:

added release notes
test coverage for different networks
hidden RPC check

in test/functional/rpc_net.py:345 in bca0e42775 outdated

 340 | +
 341 | +        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
 342 | +        node.addpeeraddress(address="pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", tried=True, port=8333) # onion address
 343 | +        node.addpeeraddress(address="c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", tried=False, port=8333)  # i2p address
 344 | +        node.addpeeraddress(address="1233:3432:2434:2343:3234:2345:6546:4534", tried=False, port=8333)                       # ipv6 address
 345 | +        node.addpeeraddress(address="fc00:1:2:3:4:5:6:7", tried=False, port=8333)                                            # cjdns address

amitiuttarwar commented at 11:06 PM on March 13, 2023:

adding these addresses would be subject to the same sporadic failures that we've been discussing elsewhere. although GetGroup would return different values, they aren't isolated so will still occasionally hash to the same positions.

you have to add a while loop or something to keep trying until success to prevent sporadic failures

stratospher commented at 6:52 PM on March 15, 2023:

makes sense, sporadic failures do happen in that commit (reverted that change now). continued this discussion here - #26988 (review)

in doc/release-notes/release-notes-26988.md:11 in bca0e42775 outdated

   6 | +
   7 | +Tools and Utilities
   8 | +--------
   9 | +
  10 | +- CLI -addrinfo would return known addresses including `isTerrrible` addresses
  11 | +  now since it has been reworked to utilise `getaddrmaninfo` RPC. (#26988)

amitiuttarwar commented at 11:10 PM on March 13, 2023:

hm, seems a little low-level for release notes. maybe something like

- CLI -addrinfo previously only returned a subset of addresses known to the node. It has been reworked to use the new `getaddrmaninfo` RPC and returns information about the entirety of the node's addrman. (#26988)

mzumsande commented at 2:56 PM on March 14, 2023:

As far as I know, test-only RPCs, similar to debug-only bitcoind options, aren't usually mentioned in release notes - the target group for release notes are regular users, while test-only options are meant for devs and may not be stable. Related test-only RPCs such as addpeeraddress weren't introduced in release notes either. So maybe only describe the changed behavior of cli -addrinfo and don't mention getaddrmaninfo?

amitiuttarwar commented at 4:27 PM on March 14, 2023:

ah that makes sense. thanks

stratospher commented at 6:48 PM on March 15, 2023:

hm, seems a little low-level for release notes. maybe something like

sounds much better! thank you!

the target group for release notes are regular users, while test-only options are meant for devs and may not be stable.

interesting, thanks! i've updated it.

jonatack commented at 7:52 PM on March 22, 2023:

CLI -addrinfo previously only returned a subset of addresses and referring to a hidden RPC might raise more questions than it answers here for users trying to figure out what's going on.

Maybe borrow from the current -addrinfo help, along the lines of CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.

(I agree the note was too low-level originally.)

fanquake commented at 11:24 AM on March 23, 2023:

Seems kind of weird to change the CLI to use a "test-only" RPC in any case

amitiuttarwar commented at 10:53 PM on March 23, 2023:

Seems kind of weird to change the CLI to use a "test-only" RPC in any case

hm, that's a good point. I originally suggested having the RPC as hidden since I imagine this feature to mostly be used by bitcoin contributors or super-users. but I'd imagine -addrinfo to be more accessible. so the two obvious options are to either (1) make the RPC public (2) leave -addrinfo as is.

I think I'd lean towards (1) because since it improves the results of -addrinfo , but also depends on the outcome of #26988 (review).

curious to hear what other reviewers think / prefer

jonatack commented at 11:22 PM on March 23, 2023:

since it improves the results of -addrinfo

how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.

It might be interesting to have both with and without IsTerrible() filtering to maintain the current info returned and to be able to compare over time, either by returning a breakdown in the new RPC or by leaving -addrinfo unchanged to compare the two. Edit: a third alternative proposed in #26988 (review).

jonatack commented at 11:26 PM on March 23, 2023:

For example, I've been tweeting a bit about the recent increase in Tor and I2P peers known to my node based on -addrinfo output and mentioning that these were also recently active peer counts (active in the last month).

stratospher commented at 4:27 PM on April 21, 2023:

Seems kind of weird to change the CLI to use a "test-only" RPC in any case @fanquake, @amitiuttarwar -generate CLI currently uses a hidden RPC (generatetoaddress) too. i don't think a normal user would be interested in the new/tried table breakdown of addresses. so hopefully it's ok to keep it hidden.

stratospher commented at 4:29 PM on April 21, 2023:

Maybe borrow from the current -addrinfo help, along the lines of CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.

thanks @jonatack! this sounds much better.

It might be interesting to have both with and without IsTerrible() filtering to maintain the current info returned and to be able to compare over time, either by returning a breakdown in the new RPC or by leaving -addrinfo unchanged to compare the two.

true! decided to keep the RPC/CLI in a separate PRs.

stratospher referenced this in commit eb5438af63 on Mar 15, 2023

stratospher force-pushed on Mar 15, 2023

in test/functional/rpc_net.py:294 in 81ad8c60a9 outdated

 290 | @@ -290,7 +291,7 @@ def test_addpeeraddress(self):
 291 |          by first testing adding a tried table entry before testing adding a new table one.
 292 |          """
 293 |          self.log.info("Test addpeeraddress")
 294 | -        self.restart_node(1, ["-checkaddrman=1"])
 295 | +        self.restart_node(1, ["-checkaddrman=1", "-cjdnsreachable"])

brunoerg commented at 6:55 PM on March 15, 2023:

What's the reason for -cjdnsreachable here?

stratospher commented at 7:04 PM on March 15, 2023:

oops, forgot to revert that. thanks!

brunoerg commented at 7:09 PM on March 15, 2023:

ohh, got it! thank you!

stratospher referenced this in commit 1bddd77cbc on Mar 15, 2023

stratospher force-pushed on Mar 15, 2023

in src/bitcoin-cli.cpp:264 in 2fa935b389 outdated

 270 |          if (!args.empty()) {
 271 |              throw std::runtime_error("-addrinfo takes no arguments");
 272 |          }
 273 | -        UniValue params{RPCConvertValues("getnodeaddresses", std::vector<std::string>{{"0"}})};
 274 | -        return JSONRPCRequestObj("getnodeaddresses", params, 1);
 275 | +        return JSONRPCRequestObj("getaddrmaninfo", NullUniValue, 1);

jonatack commented at 7:44 PM on March 22, 2023:

2fa935b389ac05c35 As getaddrmaninfo would be a new RPC, this code would break -addrinfo for clients using this code to call to long-running nodes running earlier versions of bitcoind starting from v22 (#21595). Please maintain compatibility (not in output, just the call still working) with these versions in your approach.

amitiuttarwar commented at 10:56 PM on March 23, 2023:

that's interesting. are you talking about the use case where clients would upgrade their bitcoin-cli but not their bitcoind?

I'm curious to learn more about this, do you have additional context you can share? eg. is this a frequent use case? are there other circumstances / PRs where something similar was handled? are cli updates then versioned, or the expectation is to maintain backward compatibility from the point in time that the new interface is exposed?

I'm not very familiar with the cli history, so appreciate any context you can share in advance !

jonatack commented at 11:11 PM on March 23, 2023:

Yes, for -netinfo we've needed to pay attention to this after feedback from affected users, including colleagues here, who were running nodes on previous, supported versions of Bitcoin Core, either as the nodes were long-running or for benchmarking or other reasons and requested we avoid breaking user space.

One solution could be to do a server version check (there is one for -netinfo) to decide which RPC to call. Another would be to leave -addrinfo as-is and (maybe, just an idea) add the new functionality instead to -getinfo in a human-friendly way as a follow-up, as presumably the new RPC would run faster (I didn't think to check its relative performance yet).

Further thoughts: how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.

jonatack commented at 11:17 PM on March 23, 2023:

(For -netinfo, as it continued to call the same endpoints, maintaining compatibility meant adding IsNull() checks on some getpeerinfo fields that were newer or that changed from always returned to optional.)

jonatack commented at 11:44 AM on March 24, 2023:

Here's another idea. If the server version is less than current master, return the current -addrinfo, and otherwise return something like the following. This adds the new data without the signal loss of removing the existing data, while maintaining the call operational for servers running earlier versions (from v22).

{
  "addresses known, after quality/recency filtering (original -addrinfo)": {
    "ipv4": 1,
    "ipv6": 0,
    "onion": 14673,
    "i2p": 1129,
    "cjdns": 7,
    "total": 15810
  },
  "all addresses known (no filtering)": {
    "ipv4": 1,
    "ipv6": 0,
    "onion": 14673,
    "i2p": 1129,
    "cjdns": 7,
    "total": 15810
  }
}

mzumsande commented at 3:14 PM on March 24, 2023:

how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.

IsTerrible is used for two things outside of RPCs: 1.) Terrible addresses are not included in answers to GETADDR requests from peers 2.) In case of a conflict (two addrs would go to the same bucket and position in the new tables) the old addr is only overwritten if it's terrible.

Notably, IsTerrible isn't used as a filter while making outbound connections, which is the primary function of addrman and the thing users would be most concerned about. That's why I think that the total number of addresses is the better number to report in RPCs.

jonatack commented at 3:44 PM on March 24, 2023:

Since we need to maintain compat with v22-v24 not to break user space, it seems helpful to return both so as not to disrupt signal/context as well for those who have been using -addrinfo (no need to break what wasn't broken), thus the idea in #26988 (review). This would also allow more insight for users as to how much is filtered (and possibly to us regarding its usage in making connections).

stratospher commented at 4:30 PM on April 21, 2023:

Getting stats on filtered/unfiltered addresses is interesting data!

But also feeling confused about what to do here - so haven't changed anything yet. (updated the PR to just discuss CLI changes, opened #27511 for getaddrmaninfo RPC)

what happens in the long run? In the long run, wouldn't ordinary users find it better if the CLI has a simple display of just breakdown of addresses by network? they wouldn't care much about filtering. I'd imagine we'd eventually switch over to displaying just filtered/unfiltered addresses.
should that client-side complexity be handled in the code? Users are free to run whichever client version they like. The code would look really messy if we try preserving backward compatibility when the situation mentioned in 1 happens.

jonatack commented at 4:43 PM on April 21, 2023:

Please don't break user space in terms of the call not working or no longer returning data users have been depending on. Long-term, -addrinfo has been around for a while, and ordinary users may want to continue knowing how many good peers their node has seen recently (I do -- sure, add a total count as well, provided the usual data is still available). Again suggest #26988 (review) or displaying both values on each line in a human-readable way.

jonatack commented at 5:16 PM on April 21, 2023:

(See #27511 (comment) for a current example on mainnet of how different the data can be.)

stratospher commented at 4:57 PM on May 15, 2023:

noted. not convinced how desirable the additional code complexity is. would like to hear how other people think about code complexity too before deciding what to do next.

jonatack commented at 3:50 PM on May 15, 2024:

I don't think there is much complexity, nor is it a valid argument for breaking several years of context, particularly when there are non-difficult alternatives (proposed above).

jonatack commented at 8:05 PM on March 22, 2023: member

ping @jonatack, could you have a look, especially at the changes to bitcoin-cli?

Thanks! Sorry for missing the ping and not looking sooner.

(@stratospher please ping me when you update to address #26988 (review))

in src/rpc/net.cpp:972 in 1bddd77cbc outdated

 965 | @@ -966,6 +966,60 @@ static RPCHelpMan addpeeraddress()
 966 |      };
 967 |  }
 968 |  
 969 | +static RPCHelpMan getaddrmaninfo()
 970 | +{
 971 | +    return RPCHelpMan{"getaddrmaninfo",
 972 | +                      "\nReturns count of addresses in new/tried table of all networks (or a given network). This RPC is for testing only.\n",

jonatack commented at 8:40 PM on March 22, 2023:

Testing the output of this RPC and -addrinfo side-by-side to check their output, I found myself mentally summing the RPC values to compare with the -addrinfo totals and thinking that it would be handy to have the sum returned by the RPC as well, i.e. new/tried/total.

                      "\nReturns the number of addresses in the `new` and `tried` tables and their sum for all networks (default) or a given network. This RPC is for testing only.\n",

$ ./src/bitcoin-cli -signet -addrinfo                 
{
  "addresses_known": {
    "ipv4": 2730,
    "ipv6": 1011,
    "onion": 167,
    "i2p": 32,
    "cjdns": 0,
    "total": 3940
  }
}
$ ./src/bitcoin-cli -signet getaddrmaninfo           
[
  {
    "network": "ipv4",
    "new": 2673,
    "tried": 57
  },
  {
    "network": "ipv6",
    "new": 981,
    "tried": 30
  },
  {
    "network": "onion",
    "new": 101,
    "tried": 66
  },
  {
    "network": "i2p",
    "new": 20,
    "tried": 12
  },
  {
    "network": "cjdns",
    "new": 0,
    "tried": 0
  }
]

stratospher commented at 4:30 PM on April 21, 2023:

you're right! I've updated getaddrmaninfo taking both these suggestions.

ajtowns commented at 2:18 AM on March 30, 2023: contributor

My opinion only, but it seems like it would be simpler to have the RPC not take any arguments and always output an object:

$ bitcoin-cli getaddrinfo
{
    "ipv4": {
        "new": 5,
        "tried": 3
    },
    ...
}

If you want a specific network, you can use jq: bitcoin-cli getaddrinfo | jq .ipv4 ? You can also do sums this way if desired: | jq .ipv4.new + ipv6.new. (Having an object rather than an array makes it easy to get at the values rather than an array and having to select by one of the fields)

stratospher renamed this:
~~rpc: Add test-only RPC getaddrmaninfo for new/tried table address count~~
cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency
on Apr 21, 2023

stratospher referenced this in commit d4f62f9315 on Apr 21, 2023

stratospher force-pushed on Apr 21, 2023

stratospher commented at 4:33 PM on April 21, 2023: contributor

thank you for the useful feedback! Splitting this into 2 separate PRs since it’d be easier to think about RPC and CLI parts separately. I’ve opened #27511 for getaddrmaninfo RPC and updated this PR to reflect just the CLI changes.

I liked returning objects as output and displaying total addresses per network ideas. Updated getaddrmaninfo in #27511 to use these.
Haven't changed the previous CLI approach because of #26988 (review).

DrahtBot added the label CI failed on Apr 21, 2023

stratospher referenced this in commit 2c6bc435e0 on May 15, 2023

stratospher force-pushed on May 15, 2023

DrahtBot removed the label CI failed on May 15, 2023

stratospher commented at 4:57 PM on May 15, 2023: contributor

Rebased. Looking for feedback on this comment - whether it is desirable to have additional code complexity and maintain user space/backward compatability in the bitcoin-cli when client upgrades bitcoind but not bitcoin-cli. (personally don't like the code complexity this introduces for a less common scenario)

luke-jr commented at 3:09 AM on July 27, 2023: member

Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in #27511.

If it's going to be used for this, it shouldn't be hidden/test-only...

amitiuttarwar commented at 6:15 AM on July 30, 2023: contributor

I think this PR should be in draft right now since it builds on top of #27511 & has some outdated changes / the two commits are represented as a "merge" commit here instead of separately. I would either put it into draft or keep it up-to-date in a form that would be ready for master assuming sufficient review.

stratospher commented at 3:32 AM on August 11, 2023: contributor

@luke-jr there was discussion here regarding whether to keep the RPC hidden/public.

it was kept as a hidden RPC because:

a normal user wouldn't be interested in the new/tried table breakdown of addresses.
-generate CLI currently uses a hidden RPC (generatetoaddress) too.

what do you think?

I think this PR should be in draft right now since it builds on top of #27511 & has some outdated changes / the two commits are represented as a "merge" commit here instead of separately. I would either put it into draft or keep it up-to-date in a form that would be ready for master assuming sufficient review. @amitiuttarwar, makes sense. I've converted the PR to a draft and removed the merge commit.

stratospher marked this as a draft on Aug 11, 2023

stratospher referenced this in commit 2feacba944 on Aug 11, 2023

stratospher force-pushed on Aug 11, 2023

DrahtBot added the label Needs rebase on Aug 24, 2023

stratospher referenced this in commit 40a7202ea9 on Aug 27, 2023

stratospher force-pushed on Aug 27, 2023

stratospher referenced this in commit c70d1c9cbf on Aug 27, 2023

stratospher force-pushed on Aug 27, 2023

DrahtBot added the label CI failed on Aug 27, 2023

DrahtBot removed the label Needs rebase on Aug 27, 2023

DrahtBot removed the label CI failed on Aug 27, 2023

achow101 referenced this in commit ff564c75e7 on Sep 20, 2023

DrahtBot added the label Needs rebase on Sep 20, 2023

Frank-GER referenced this in commit da7ea225a7 on Sep 25, 2023

sidhujag referenced this in commit f2c8dfcc9a on Sep 26, 2023

ajtowns commented at 3:07 AM on September 28, 2023: contributor

it was kept as a hidden RPC because:

1. a normal user wouldn't be interested in the new/tried table breakdown of addresses.

2. `-generate CLI` currently [uses](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/bitcoin-cli.cpp#L1178) a [hidden RPC](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/rpc/mining.cpp#L1055) (generatetoaddress) too.

I think it makes sense to unhide this RPC; we have plenty of get*info functions that are only really useful experts, and that's fine. It makes sense to hide rpcs that are only useful for development (generate, setmocktime, echo) or that might cause the node to not work as expected (invalidateblock, sendmsgtopeer), but this is likely useful for some regular users and doesn't do anything potentially harmful, so there shouldn't be any reason to hide it IMO.

stratospher referenced this in commit 0b4ccbd035 on Oct 3, 2023

stratospher force-pushed on Oct 3, 2023

stratospher commented at 5:33 AM on October 3, 2023: contributor

Rebased.

I think it makes sense to unhide this RPC; we have plenty of get*info functions that are only really useful experts, and that's fine. It makes sense to hide rpcs that are only useful for development (generate, setmocktime, echo) or that might cause the node to not work as expected (invalidateblock, sendmsgtopeer), but this is likely useful for some regular users and doesn't do anything potentially harmful, so there shouldn't be any reason to hide it IMO.

That sounds reasonable. Done in #28565.

stratospher marked this as ready for review on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

in doc/release-notes/release-notes-26988.md:4 in 0b4ccbd035 outdated

   0 | @@ -0,0 +1,5 @@
   1 | +Tools and Utilities
   2 | +--------
   3 | +
   4 | +- CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency.

pablomartin4btc commented at 3:47 AM on October 16, 2023:

Should we be more specific perhaps?

- CLI -addrinfo previously (v22-v24) returned addresses known to the node after filtering for quality and recency.

stratospher commented at 3:39 PM on May 15, 2024:

liked it, done.

pablomartin4btc commented at 4:05 AM on October 16, 2023: member

tested ACK 0b4ccbd0358d95632e2e46bfdf7715b199971754

<details> <summary>output before this PR</summary>

/src/bitcoin-cli -signet -addrinfo
{
  "addresses_known": {
    "ipv4": 1097,
    "ipv6": 352,
    "onion": 0,
    "i2p": 0,
    "cjdns": 0,
    "total": 1449
  }
}

</details> <details> <summary>output after this PR</summary>

./src/bitcoin-cli -signet -addrinfo
{
  "addresses_known": {
    "ipv4": 2157,
    "ipv6": 834,
    "onion": 0,
    "i2p": 0,
    "cjdns": 0,
    "all_networks": 2991,
    "total": 5982
  }
}

</details>

I share the same concerns as @jonatack regarding maintaining backward compatibility.

Also, regarding both the totals and per network differences between before and after this changes I wonder if we should add some context in the output or in the help for the user to understand where those differences are coming from. (Mainly due to @jonatack's comment here and @mzumsande's comment here regarding isTerrible()).

Last thing for now, @stratospher I see you agreed with @brunoerg's suggestion but I don't see the changes, perhaps it was removed and I missed some discussion about it.

DrahtBot requested review from mzumsande on Oct 16, 2023

DrahtBot requested review from brunoerg on Oct 16, 2023

DrahtBot requested review from amitiuttarwar on Oct 16, 2023

fanquake referenced this in commit 22fa1f4702 on Oct 16, 2023

DrahtBot added the label CI failed on Jan 13, 2024

achow101 referenced this in commit a945f09fa6 on Mar 11, 2024

achow101 requested review from sr-gi on Apr 9, 2024

sr-gi commented at 7:35 PM on April 26, 2024: member

What is the state of this @stratospher? Are you still looking for feedback on #26988 (comment)?

DrahtBot marked this as a draft on May 13, 2024

DrahtBot commented at 8:07 AM on May 13, 2024: contributor

Turned into draft for now, due to outstanding feedback. If this is no longer a WIP, you can move it out of draft.

stratospher referenced this in commit 981aa3ae28 on May 15, 2024

stratospher force-pushed on May 15, 2024

stratospher commented at 3:42 PM on May 15, 2024: contributor

Thank you for the reviews @pablomartin4btc! I've rebased the PR and included your suggestion.

Also, regarding both the totals and per network differences between before and after this changes I wonder if we should add some context in the output or in the help for the user to understand where those differences are coming from.

i prefer not adding niche details in the user output/help because the difference would only be felt temporarily when users upgrade and it is covered in the release notes.

Last thing for now, @stratospher I see you agreed with @brunoerg's suggestion but I don't see the changes, perhaps it was removed and I missed some discussion about it.

yes! that suggestion isn't applicable anymore since the RPC is public. this was done in #27511.

stratospher commented at 3:42 PM on May 15, 2024: contributor

What is the state of this @stratospher? Are you still looking for feedback on #26988 (comment)? @sr-gi, yes! that's the only unresolved conversation. i personally prefer the current approach as explained in #26988 (review) which simply reports an error in case an older version of bitcoind is used with a newer bitcoin-cli.

stratospher referenced this in commit aebfac1344 on May 15, 2024

stratospher force-pushed on May 15, 2024

jonatack commented at 4:00 PM on May 15, 2024: member

@stratospher Referencing the review feedback in #26988 (review), the argument about complexity doesn't outweigh that, and less so when there are simple alternatives that have been proposed. If this were to be merged as-is while ignoring the review feedback above, then we'd need to propose fixes for it. I don't think it makes sense to break things only to need to fix them.

jonatack commented at 4:03 PM on May 15, 2024: member

simply reports an error in case an older version of bitcoind is used with a newer bitcoin-cli

That's not helpful to someone with a long-running older node (say, for benchmarking or statistical purposes, which may also include compiling data from -addrinfo over time) that they try to call with the latest version of bitcoind. This would be simply breaking it for them needlessly.

DrahtBot removed the label CI failed on May 15, 2024

stratospher referenced this in commit 65956f2adf on Jul 31, 2024

stratospher force-pushed on Jul 31, 2024

stratospher referenced this in commit 7554688f85 on Jul 31, 2024

DrahtBot added the label Needs rebase on Sep 4, 2024

jonatack commented at 7:23 PM on September 12, 2024: member

@stratospher (needs rebase) happy to do a call and look at this together to move it forward if you like (reach out via IRC).

stratospher referenced this in commit 4af769811f on Sep 13, 2024

stratospher force-pushed on Sep 13, 2024

stratospher referenced this in commit 4114a523c2 on Sep 13, 2024

stratospher force-pushed on Sep 13, 2024

DrahtBot added the label CI failed on Sep 13, 2024

DrahtBot commented at 6:45 AM on September 13, 2024: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30092798087</sub>

<details><summary>Hints</summary>

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label Needs rebase on Sep 13, 2024

DrahtBot removed the label CI failed on Sep 13, 2024

stratospher marked this as ready for review on Sep 23, 2024

stratospher commented at 12:36 PM on September 23, 2024: contributor

Rebased. thanks @jonatack! reaching out.

DrahtBot added the label CI failed on Oct 21, 2024

DrahtBot removed the label CI failed on Oct 25, 2024

DrahtBot added the label CI failed on Mar 14, 2025

DrahtBot commented at 11:28 AM on March 15, 2025: contributor

Needs rebase, if still relevant

DrahtBot added the label Needs rebase on Mar 23, 2025

stratospher marked this as a draft on Jun 20, 2025

stratospher referenced this in commit 8f557551de on Jun 20, 2025

stratospher referenced this in commit 878ead4b58 on Jun 20, 2025

stratospher referenced this in commit c66a72480f on Jun 20, 2025

stratospher force-pushed on Jun 20, 2025

stratospher referenced this in commit c2f3225622 on Jun 20, 2025

stratospher force-pushed on Jun 20, 2025

stratospher referenced this in commit 363d69be22 on Jun 20, 2025

DrahtBot removed the label Needs rebase on Jun 20, 2025

DrahtBot removed the label CI failed on Jun 20, 2025

stratospher commented at 12:54 PM on June 20, 2025: contributor

thanks Drahtbot! i've rebased and changed the approach in the PR to display both total + filtered addresses stats as suggested in #26988 (review).

the total address stats is more relevant since that is what is used by node to find peers to connect to.
the filtered addresses stats is kept as it is for backward compatibilty/not breaking user space when a newer bitcoin-cli is used on an older bitcoind binary (v22 - v25).

my personal preference is for displaying only the relevant total addresses stats - see this branch. but i'm fine with either approach based on what reviewers think!

stratospher marked this as ready for review on Jun 20, 2025

pablomartin4btc commented at 9:54 PM on June 24, 2025: member

tACK c2f3225622cbbe516ac23059bff47ed3c28653cf

my personal preference is for displaying only the relevant total addresses stats

I also lean toward showing only the total known addresses, as I think it better reflects what the node has in its addrman and avoids suggesting that only the filtered set "counts." I'd consider whether passing an argument might offer one or both views (filtered/unfiltered) depending on the use case, but I’ve read the previous discussions here and in #27511, and I understand the concerns around compatibility. The filtered view (based on IsTerrible()) may be misleading for users who assume those are the only addresses known — unless they're explicitly inspecting address table quality. That said, happy to follow the current direction — just sharing those thoughts for context.

<details> <summary>Comparing this latest output against my <a href="https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-1679047963">previous testing</a> I noticed that the <code>all_networks</code> element has been removed (?).</summary>

./build_26988/bin/bitcoin-cli -signet -datadir=/tmp/btc -addrinfo
{
  "all addresses known (used for selecting peers)": {
    "ipv4": 1368,
    "ipv6": 893,
    "onion": 0,
    "i2p": 0,
    "cjdns": 0,
    "total": 2261
  },
  "addresses known after quality/recency filtering (for original -addrinfo compatibility)": {
    "ipv4": 1352,
    "ipv6": 883,
    "onion": 0,
    "i2p": 0,
    "cjdns": 0,
    "total": 2235
  }
}

</details>

stratospher referenced this in commit 016ab85a13 on Jun 25, 2025

stratospher force-pushed on Jun 25, 2025

stratospher commented at 6:59 PM on June 25, 2025: contributor

Comparing this latest output against my previous testing I noticed that the all_networks element has been removed (?)

good observation! the old version was incorrect because all_networks and total were the same thing (total number of addresses from all networks in addrman). pushed an update which doesn't require us to sum up total number of addresses again since that information is already computed in getaddrmaninfo.

DashCoreAutoGuix referenced this in commit 6ed8c569db on Jul 28, 2025

DashCoreAutoGuix referenced this in commit dcbe444a37 on Aug 3, 2025

DashCoreAutoGuix referenced this in commit 81f5ed6515 on Aug 5, 2025

DashCoreAutoGuix referenced this in commit 8f1cf1027e on Aug 9, 2025

achow101 removed review request from amitiuttarwar on Oct 22, 2025

achow101 requested review from sipa on Oct 22, 2025

achow101 requested review from vasild on Oct 22, 2025

achow101 requested review from ajtowns on Oct 22, 2025

in doc/release-notes-26988.md:4 in 016ab85a13

   0 | @@ -0,0 +1,6 @@
   1 | +Tools and Utilities
   2 | +--------
   3 | +
   4 | +- CLI -addrinfo previously (v22 - v29) returned addresses known to the node after filtering for quality and recency.

jonatack commented at 9:30 PM on October 31, 2025:

- CLI -addrinfo previously (v22 - v30) returned addresses known to the node after filtering for quality and recency.

in src/bitcoin-cli.cpp:283 in 016ab85a13

 278 | +    static constexpr int ID_FILTERED_ADDRMAN = 1;
 279 | +
 280 | +    /** Prepare a batch of JSON-RPC requests to retrieve addrman data.
 281 | +     *  This combines:
 282 | +     *   - getaddrmaninfo (available from v26.0+) for stats of all addresses in the addrman (used for selecting peers)
 283 | +     *   - getnodeaddresses (v22.0+) for stats of filtered address in the addrman (used by old -addrinfo and kept for not breaking user space)

jonatack commented at 10:05 PM on October 31, 2025:

     *   - getnodeaddresses (v22.0+) for stats of addresses in the addrman filtered by quality (i.e. not IsTerrible), used by -addrinfo since v22.0 and kept for data continuity)

jonatack commented at 10:11 PM on October 31, 2025: member

Approach ACK 016ab85a13408ad980c3dbce4e041e14a4fcf3b8, thanks for updating, tested with a server on master, not yet tested with a server running pre-v26 (before getaddrmaninfo)

$ ./build/bin/bitcoin-cli -addrinfo
{
  "all addresses known (used for selecting peers)": {
    "ipv4": 48133,
    "ipv6": 8017,
    "onion": 17257,
    "i2p": 3757,
    "cjdns": 11,
    "total": 77175
  },
  "addresses known after quality/recency filtering (for original -addrinfo compatibility)": {
    "ipv4": 45921,
    "ipv6": 7487,
    "onion": 16259,
    "i2p": 3035,
    "cjdns": 7,
    "total": 72709
  }
}

fanquake commented at 10:40 AM on November 2, 2025: member

not yet tested with a server running pre-v26 (before getaddrmaninfo)

25.x, 26.x and 27.x are all end-of-life; so that seems out of scope for needing to test / accomodate code-wise?

sipa commented at 11:09 AM on November 2, 2025: member

Concept ACK, but I think it's weird to use a long English phrase as field name. Maybe just "addresses_total" and "addresses_filtered" or so? If we care about backward compatibility, we could add a -deprecatedrpc=addresses_known option to enable the "addresses_known" field (which would be a copy of "addresses_filtered").

in src/bitcoin-cli.cpp:298 in 016ab85a13

 295 | +        result.push_back(JSONRPCRequestObj("getnodeaddresses", params, ID_FILTERED_ADDRMAN));
 296 | +        return result;
 297 |      }
 298 |  
 299 | -    UniValue ProcessReply(const UniValue& reply) override
 300 | +    UniValue ProcessReply(const UniValue &batch_in) override

vasild commented at 5:23 PM on November 6, 2025:

nit:

    UniValue ProcessReply(const UniValue& batch_in) override

in src/bitcoin-cli.cpp:313 in 016ab85a13

 312 | +            for (size_t i = 0; i < network_types.size() - 1; ++i) {
 313 | +                uint64_t addr_count = addrman_counts[i]["total"].getInt<int>();
 314 | +                addresses.pushKV(network_types[i], addr_count);
 315 | +            }
 316 | +            uint64_t total = addrman_counts[network_types.size() - 1]["total"].getInt<int>();
 317 | +            addresses.pushKV("total", total);

vasild commented at 5:34 PM on November 6, 2025:

This assumes that the all_networks key is the last key in the JSON returned by getaddrmaninfo but JSON objects don't have order (unlike arrays). So it is possible that the order changes any time (in theory) and then this code will break. Better write it in such a way that it works regardless of the order in which keys are returned.

Maybe something like:

iterate all from i = 0 to i < network_types.size()
    if key is all_networks
        this is the total
    else
        this is one of the networks

stratospher commented at 1:08 PM on November 20, 2025:

ah thanks! that makes sense.

in src/bitcoin-cli.cpp:309 in 016ab85a13

 308 | +        if (batch[ID_FULL_ADDRMAN]["error"].isNull()) {
 309 | +            UniValue addresses{UniValue::VOBJ};
 310 | +            const std::vector<std::string>& network_types{batch[ID_FULL_ADDRMAN]["result"].getKeys()};
 311 | +            const std::vector<UniValue>& addrman_counts{batch[ID_FULL_ADDRMAN]["result"].getValues()};
 312 | +            for (size_t i = 0; i < network_types.size() - 1; ++i) {
 313 | +                uint64_t addr_count = addrman_counts[i]["total"].getInt<int>();

vasild commented at 5:35 PM on November 6, 2025:

This requests a return type of int (getInt<int>) but it assigns it to uint64_t variable.

stratospher commented at 1:08 PM on November 20, 2025:

I've changed addr_count to int. theoretical max of addrman size is 81,920 (1024 new buckets * 64 entries + 256 tried buckets * 64 entries), so should be enough.

in src/bitcoin-cli.cpp:339 in 016ab85a13

 336 |              addresses.pushKV(NETWORKS[i], counts.at(i));
 337 |              total += counts.at(i);
 338 |          }
 339 |          addresses.pushKV("total", total);
 340 | -        result.pushKV("addresses_known", std::move(addresses));
 341 | +        result.pushKV("addresses known after quality/recency filtering (for original -addrinfo compatibility)", std::move(addresses));

vasild commented at 5:37 PM on November 6, 2025:

I agree that these long keys look weird.

vasild commented at 5:38 PM on November 6, 2025: contributor

Reviewed as of 016ab85a13

stratospher referenced this in commit 3d220032c3 on Nov 20, 2025

stratospher force-pushed on Nov 20, 2025

stratospher referenced this in commit 858f49bcb3 on Nov 20, 2025

stratospher force-pushed on Nov 20, 2025

DrahtBot added the label CI failed on Nov 20, 2025

DrahtBot removed the label CI failed on Nov 20, 2025

stratospher commented at 3:32 PM on November 20, 2025: contributor

thank you for the reviews! I've pushed an update changing the long key names. also retained the original key name in -addrinfo's result (addresses_known) in case some other scripts/tools rely on it.

$ build/bin/bitcoin-cli -addrinfo
{
  "addresses_known": {
    "ipv4": 52037,
    "ipv6": 8855,
    "onion": 3465,
    "i2p": 0,
    "cjdns": 0,
    "total": 64357
  },
  "addresses_filtered": {
    "ipv4": 41811,
    "ipv6": 5808,
    "onion": 2465,
    "i2p": 0,
    "cjdns": 0,
    "total": 50084
  }
}

regarding #26988 (comment):

25.x, 26.x and 27.x are all end-of-life; so that seems out of scope for needing to test / accomodate code-wise?

Good point. Thinking of changing it in the next push.

According to bitnodes.io - among bitcoin core nodes, 95% are already on v26+ and therefore support getaddrmaninfo (introduced 2 years ago).

Maybe the concern regarding older node <-> newer CLI compatibility isn't there anymore considering how much the network/software has changed! (cc @jonatack, curious to hear your thoughts and anyone else's of course)

95% of bitcoin core nodes are on v26+ and have access to getaddrmaninfo (introduced 2 years ago)
the nodes we're concerned about breaking user space (v22 - v26 nodes) with have reached EOL

jonatack commented at 6:29 PM on November 20, 2025: member

CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.

in src/bitcoin-cli.cpp:341 in 858f49bcb3

 338 |              addresses.pushKV(NETWORKS[i], counts.at(i));
 339 |              total += counts.at(i);
 340 |          }
 341 |          addresses.pushKV("total", total);
 342 | -        result.pushKV("addresses_known", std::move(addresses));
 343 | +        result.pushKV("addresses_filtered", std::move(addresses));

jonatack commented at 6:39 PM on November 20, 2025:

I agree with preferably keeping the field names short, but the data corresponding to "addresses_known" should not change.

Perhaps use "addresses_unfiltered" or "addresses_not_filtered_for_quality_and_recency" for the new field.

As this is a CLI output, as opposed to an RPC one, its output presentation can be more optimized for human use (see -getinfo, for instance) while providing consistent historical data.

fanquake commented at 6:41 PM on November 20, 2025: member

CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node,

Could you elaborate on why these people, particularly the developers, or node operators, are running EOL versions of Core (with known CVEs); and why they can't upgrade their nodes, but at the same time, they are downloading and using the latest version of bitcoin-cli?

There doesn't seem to be any reason not to do the same here, rather than choosing to break it.

The reason to not do it, is that it then implies we are still supporting versions of our software, which we have marked as EOL.

mzumsande commented at 7:10 PM on November 20, 2025: contributor

CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.

I don't agree with that. I don't see bitcoin-cli and bitcoind as completely separate programs that should be supported in arbitrary combinations for eternity.

If that was the case, it would have been a bad mistake to ever have added non-trivial functionality like -addrinfo or -netinfo to bitcoin-cli instead of bitcoind in the first place because that would have created an infinite obligation to maintain compatibility of current bitcoind versions with ancient versions of bitcoin-cli long out of support.

While it makes some limited sense to try to keep up compatibility over version ranges supported at the same time (although even that is debatable in my opinion), I don't think that it is reasonable to make any efforts supporting compatibility beyond this.

cli: modify -addrinfo to use getaddrmaninfo RPC endpoint

currently -addrinfo returns addresses known to the node after
filtering for quality and recency. However, the node considers
all known addresses (even the filtered out addresses) when
selecting peers to connect to.

So update -addrinfo to also display the full set of known
addresses for more useful node information.

675be93024

stratospher referenced this in commit 5b05a9959f on Nov 24, 2025

stratospher force-pushed on Nov 24, 2025

stratospher commented at 12:06 PM on November 24, 2025: contributor

I pushed an update. I also don't think we should implement a fallback to support a rare usecase for few individuals. I've run into a few occasions where I used older bitcoind and newer cli but it's when I misconfigure something accidentally and wasn't the behaviour I intended. I guess this is what most normal users would run into.

This is why I think we shouldn't implement a fallback:

since -addrinfo is meant to be a human facing CLI dashboard (as implied in the documentation everywhere) and not an RPC endpoint (which other software/tools use programatically), the human facing CLI dashboard does not require a deprecation path (unlike RPCs). Humans can read and respond to it. It's not meant to used in automated scripts (those should use RPCs directly) and only meant to be read by humans.
if different versions of bitcoind and bitcoin-cli are used - it's the responsibility of the user to do proper package management. bitcoind and bitcoin-cli are versioned and released together.
This feels like an edge case for very few individuals. The transition period is over and 95% of bitcoin core nodes are already on v26+. If someone is capable of downloading the latest bitcoin-cli, they can upgrade their node/use an older bitcoin-cli.
getaddrmaninfo support is there for 95% of bitcoin core nodes that are already on v26+
v22-v27 are all EOL with known security vulnerabilities (from this comment)
it would be contradictory to state in the release notes that we are maintaining compatibility solely for EOL versions only for the human facing CLI dashboard, when we do not maintain EOL support anywhere else.

So my preferred solution is to give human node operators a clear + helpful message explaining the issue and asking them to use an older CLI or newer bitcoind to which they can choose. A clear message is more helpful than silently falling back. @jonatack, thank you for your feedback and I'm sorry there's a difference in opinion + this is dragging out so long. I hope the reasons stated here and the comments (1 and 2) above make sense to you. I appreciate your time spent discussing this, and I hope the reasoning above contributes to finding the best long-term solution!

in doc/release-notes-26988.md:6 in 5b05a9959f

   0 | @@ -0,0 +1,6 @@
   1 | +Tools and Utilities
   2 | +--------
   3 | +
   4 | +- CLI -addrinfo previously (v22 - v30) returned addresses known to the node after filtering for quality and recency.
   5 | +  However, the node considers all known addresses (even the filtered addresses) when selecting peers to connect to.
   6 | +  Update -addrinfo to also return the full set of known addresses for more useful node information. (#26988)

sipa commented at 3:32 PM on December 8, 2025:

It's a bit strange to use an imperative form in release notes, given that it's describing changes that have been made already.

Suggestion:

CLI -addrinfo now returns the full set of known addresses. In previous versions (v22.0 - v30.0) the set of returned addresses was filtered for quality and recency. This was changed since it does not match the logic for selecting peers to connect to, as that does not filter either.

Also worth mentioning that the CLI feature is now imcompatible with old nodes?

sipa commented at 3:34 PM on December 8, 2025: member

Concept/Approach ACK. I think it's fine to break compatibility with unmaintained node versions, given that there is a good error message for this case.

vasild approved

vasild commented at 2:43 PM on December 9, 2025: contributor

ACK 5b05a9959f1633bfee78d9edb180c672b0640ab5

DrahtBot requested review from pablomartin4btc on Dec 9, 2025

DrahtBot requested review from jonatack on Dec 9, 2025

DrahtBot requested review from mzumsande on Dec 9, 2025

DrahtBot requested review from amitiuttarwar on Dec 9, 2025

DrahtBot requested review from sipa on Dec 9, 2025

pablomartin4btc commented at 3:16 AM on December 11, 2025: member

tACK 5b05a9959f1633bfee78d9edb180c672b0640ab5

with bitcoind < v26:

/build/bin/bitcoin-cli -signet -datadir=/tmp/btc -addrinfo
error: -addrinfo requires bitcoind v26.0 or later which supports getaddrmaninfo RPC. Please upgrade your node or use bitcoin-cli from the same version.

stratospher referenced this in commit 325f98bca0 on Dec 12, 2025

stratospher force-pushed on Dec 12, 2025

vasild approved

vasild commented at 6:35 AM on December 12, 2025: contributor

ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d

DrahtBot requested review from pablomartin4btc on Dec 12, 2025

in doc/release-notes-26988.md:8 in 325f98bca0

   0 | @@ -0,0 +1,8 @@
   1 | +Tools and Utilities
   2 | +--------
   3 | +
   4 | +- CLI -addrinfo now returns the full set of known addresses. In previous versions (v22.0 - v30.0) the set of returned
   5 | +  addresses was filtered for quality and recency. This was changed since it does not match the logic for selecting peers
   6 | +  to connect to, which does not filter. Note: CLI -addrinfo now requires bitcoind v26.0 or later, as it uses the
   7 | +  getaddrmaninfo RPC internally. Users with older node versions that are no longer maintained would need to use an older
   8 | +  bitcoin-cli version or upgrade their node. (#26988)

pablomartin4btc commented at 6:55 PM on December 12, 2025:

tiny nit (not a blocker):

  getaddrmaninfo RPC internally. Users querying older, unmaintained node versions would need to use an older bitcoin-cli version. (#26988)

DrahtBot requested review from pablomartin4btc on Dec 12, 2025

pablomartin4btc commented at 6:56 PM on December 12, 2025: member

ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d

(Left a small suggestion since the older node may be operated by someone other than the CLI user, and upgrading it might not be possible or appropriate when this change is the only reason. I think this would also align with @jonatack’s earlier note.)

doc: add release notes for #26988 b3046cca71

stratospher force-pushed on Dec 13, 2025

pablomartin4btc approved

pablomartin4btc commented at 1:51 PM on December 13, 2025: member

re-ACK b3046cca7182f3399a221757318d24e203092301

DrahtBot requested review from vasild on Dec 13, 2025

sedited removed review request from amitiuttarwar on Mar 3, 2026

sedited removed review request from vasild on Mar 3, 2026

sedited requested review from vasild on Mar 3, 2026

sedited commented at 3:44 PM on March 3, 2026: contributor

There are six stale reviews with various levels of approval here, while there has been no activity on this PR for three months now. Is there anything that can be done to resurrect progress again?

DrahtBot requested review from amitiuttarwar on Mar 3, 2026

sr-gi commented at 9:34 AM on March 5, 2026: member

IMO this was dragging because of the difference in opinions on whether to have a simpler approach, or a more complex one to accommodate older versions of bitcoind. It'd be useful to me to know when we have landed.

My two cents on this: being about to release v31, I personally don't think it makes sense to have extra complexity to cater for versions pre-26, which are all EOL. I also think the usecase of having an old version of bitcoind with an up-to-date version of bitcoin-cli should be rare, and up to the user to figure out.

fanquake commented at 10:01 AM on March 5, 2026: member

It'd be useful to me to know when we have landed.

There was no response to #26988 (comment), so it's not really clear what the "EOL, CVE-vulnerable bitcoind + latest available bitcoin-cli" use-case is. Ideally that's not something we are actively trying to support, and instead would be solving whichever issue is preventing those users from upgrading their bitcoind (assuming there is one).

stratospher commented at 10:20 AM on March 5, 2026: contributor

IMO this was dragging because of the difference in opinions on whether to have a simpler approach, or a more complex one to accommodate older versions of bitcoind. It'd be useful to me to know when we have landed.

yeah, PR in the current state has the simpler approach implemented + informative error message if they're using a very old bitcoin-cli on a recent node + mention in the release notes.

sr-gi commented at 10:21 AM on March 5, 2026: member

IMO this was dragging because of the difference in opinions on whether to have a simpler approach, or a more complex one to accommodate older versions of bitcoind. It'd be useful to me to know when we have landed.

yeah, PR in the current state has the simpler approach implemented + informative error message if they're using a very old bitcoin-cli on a recent node + mention in the release notes.

SGTM. Will review

vasild approved

vasild commented at 10:26 AM on March 5, 2026: contributor

ACK b3046cca7182f3399a221757318d24e203092301

ajtowns commented at 12:08 PM on March 5, 2026: contributor

ACK b3046cca7182f3399a221757318d24e203092301

I wonder if a useful analogy is something like: suppose an charity has 9010 volunteers who've signed up to help out, but in practice over the past months it's turned out that only 420 of them actually followup reliably when askt ed. Having that organisation report that there are 9010 volunteers available is correct in a literal sense, but the 420 number is probably useful to at least have available if you're trying to predict how effective the organisation will be at its charitable purposes.

In that case, eliminating the "IsTerrible" nodes arguably performs a similar "we know about X nodes, but really only Y are likely to be useful". But probably the "total" vs "tried" count is already good enough for that? On mainnet, I'm seeing: 60k total, 10k tried, 53k not-terrible; so the 10k tried figure sounds more sensible to me. ~~This PR~~ getaddrmaninfo already reports total and tried figures, so perhaps it would be good if a followup had -addrinfo also do that?

in src/bitcoin-cli.cpp:289 in b3046cca71

 298 | -            const int8_t network_id{NetworkStringToId(network_name)};
 299 | -            if (network_id == UNKNOWN_NETWORK) continue;
 300 | -            ++counts.at(network_id);
 301 | +        if (!reply["error"].isNull()) {
 302 | +            if (reply["error"]["code"].getInt<int>() == RPC_METHOD_NOT_FOUND) {
 303 | +                throw std::runtime_error("-addrinfo requires bitcoind v26.0 or later which supports getaddrmaninfo RPC. Please upgrade your node or use bitcoin-cli from the same version.");

sr-gi commented at 1:36 PM on March 5, 2026:

nit: To my understanding, we tend not to suggest updating the software as this is something the user should choose on their own. I think we could be changed to something like:

"Please make sure your bitcoin-cli and bitcoind versions match".

Feel free to disregard, or only address if you need to retouch.

sr-gi commented at 1:36 PM on March 5, 2026: member

tACK b3046cca7182f3399a221757318d24e203092301

I tested this on top of b3046cca7182f3399a221757318d24e203092301 and against a 25.2 and 26.0 instances. I can confirm the exception is thrown for version < v26.

Shailendra53 referenced this in commit 603d1edd57 on Mar 9, 2026

Shailendra53 referenced this in commit 5444450d98 on Mar 9, 2026

Shailendra53 referenced this in commit 258cb8534e on Mar 10, 2026

Shailendra53 referenced this in commit 4f940c6be1 on Mar 10, 2026

sedited merged this on Mar 11, 2026

sedited closed this on Mar 11, 2026