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.
open questions:
should we continue displaying filtered address stats?
currently the PR displays both total + filtered addresses stats. 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 not breaking backward compatibilty 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.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
LLM Linter (✨ experimental)
Possible typos and grammar issues:
filtered address -> filtered addresses [plural form for consistency in “stats of filtered address(s) in the addrman”]
drahtbot_id_4_m
stratospher force-pushed
on Jan 30, 2023
stratospher force-pushed
on Jan 30, 2023
in
test/functional/rpc_net.py:343
in
caab2131d6outdated
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?
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…
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:
0while len(node.getnodeaddresses(count=0)) < 3:
1 ipv6_address = ":".join(format(randint(0, 0xffff), '04x') for _ in range(8))
2 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.
0added = False
1while not added:
2 result = node.addpeeraddress(address="..", tried=to_tried, port=port)
3 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
973@@ -974,6 +974,60 @@ static RPCHelpMan addpeeraddress()
974 };
975 }
976977+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:
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
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:436
in
7c34c35b47outdated
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 ACK7c34c35b47. 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:
0diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
1index 3d39fb47d..6928c1211 100755
2--- a/test/functional/rpc_net.py
3+++ b/test/functional/rpc_net.py
4@@ -331,11 +331,23 @@ class NetTest(BitcoinTestFramework):
5 6 def test_getaddrmaninfo(self):
7 self.log.info("Test getaddrmaninfo")
8- # current ipv4 count in node 1's Addrman: new 1, tried 1
9 self.log.debug("Test that getaddrmaninfo provides correct new/tried table address count")
10+
11+ ipv6_addr = "1233:3432:2434:2343:3234:2345:6546:4534"
12+ self.nodes[1].addpeeraddress(address=ipv6_addr, port=8333)
13+
14+ network_count = {
15+ '': { 'new': 2, 'tried': 1 },
16+ 'ipv6': { 'new': 1, 'tried': 0},
17+ 'ipv4': { 'new': 1, 'tried': 1}
18+ }
19+
20 res = self.nodes[1].getaddrmaninfo()
21- assert_equal(res[0]["new"], 1)
22- assert_equal(res[0]["tried"], 1)
23+ for network, count in network_count.items():
24+ res = self.nodes[1].getaddrmaninfo(network=network) if network else self.nodes[1].getaddrmaninfo()
25+ assert_equal(sum(map(lambda x: int(x['new']), res)), count["new"])
26+ assert_equal(sum(map(lambda x: int(x['tried']), res)), count["tried"])
27+
28 self.log.debug("Test that getaddrmaninfo with an invalid network throws an error")
29 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?
0self.log.debug("Test that addpeerinfo is a hidden RPC")
1# It is hidden from general help, but its detailed help may be called directly.2assert"addpeerinfo"notin node.help()
3assert"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
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
bca0e42775outdated
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
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.
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.
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
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).
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.
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
81ad8c60a9outdated
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"])
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 !
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.
(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.)
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).
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.
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.
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.
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.
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
1bddd77cbcoutdated
965@@ -966,6 +966,60 @@ static RPCHelpMan addpeeraddress()
966 };
967 }
968969+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",
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.
0"\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",
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.
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
0b4ccbd035outdated
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.
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
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
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.
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
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
tACKc2f3225622cbbe516ac23059bff47ed3c28653cf
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.
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. The address stats
after filtering is kept as it is and displayed for not breaking
userspace and could someday be removed.
3b6cbdde14
doc: add release notes for #26988016ab85a13
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-10-24 03:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
