cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency#26988
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26988. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2025-12-13 |
cac08f2 to
caab213
Compare
mzumsande
left a comment
There was a problem hiding this comment.
Concept ACK, will review soon.
caab213 to
c645fb6
Compare
|
obvious concept ack considering I proposed #26907 |
c645fb6 to
35bd55c
Compare
35bd55c to
a04bfa3
Compare
"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. |
a04bfa3 to
7c34c35
Compare
i'm confused about this since making it into and there are only few network types. would be interested in what others think. |
|
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 |
|
that was just a question, thinking about complexity I agree on leaving it as is. going to review in depth soon. |
|
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. |
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 |
jonatack
left a comment
There was a problem hiding this comment.
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
}
}
|
|
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 |
vasild
left a comment
There was a problem hiding this comment.
Reviewed as of 016ab85a13
|
thank you for the reviews! I've pushed an update changing the long key names. regarding #26988 (comment):
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)
|
|
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. |
| uint64_t total{0}; // Total address count | ||
| for (size_t i = 1; i < NETWORKS.size() - 1; ++i) { | ||
| addresses.pushKV(NETWORKS[i], counts.at(i)); | ||
| total += counts.at(i); | ||
| } | ||
| addresses.pushKV("total", total); | ||
| result.pushKV("addresses_known", std::move(addresses)); | ||
| result.pushKV("addresses_filtered", std::move(addresses)); |
There was a problem hiding this comment.
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.
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?
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
left a comment
There was a problem hiding this comment.
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.
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.
|
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:
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. |
|
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
left a comment
There was a problem hiding this comment.
ACK 5b05a9959f1633bfee78d9edb180c672b0640ab5
pablomartin4btc
left a comment
There was a problem hiding this comment.
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.
vasild
left a comment
There was a problem hiding this comment.
ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d
pablomartin4btc
left a comment
There was a problem hiding this comment.
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.)
|
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? |
|
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. |
There was no response to #26988 (comment), so it's not really clear what the "EOL, CVE-vulnerable |
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 |
|
ACK b3046cc 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. |
| ++counts.at(network_id); | ||
| if (!reply["error"].isNull()) { | ||
| if (reply["error"]["code"].getInt<int>() == RPC_METHOD_NOT_FOUND) { | ||
| 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."); |
There was a problem hiding this comment.
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.
Rework of
-addrinfoCLI is done usinggetaddrmaninfoRPC 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,
-addrinforeturns total number of addresses the node knows about after filtering them for quality + recency usingisTerrible. HoweverisTerribleaddresses don't matter when selecting outbound peers to connect to. Total number of addresses the node knows about could be higher than what-addrinfocurrently displays. See #24370.