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 react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
cac08f2 to
caab213
Compare
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 7c34c35. 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. |
|
Rebased. thanks @jonatack! reaching out. |
|
Needs rebase, if still relevant |
4114a52 to
c66a724
Compare
c66a724 to
c2f3225
Compare
|
thanks Drahtbot! i've rebased and changed the approach in the PR to display both total + filtered addresses stats as suggested in #26988 (comment).
my personal preference is for displaying only the relevant total addresses stats - see this branch. |
There was a problem hiding this comment.
tACK c2f3225
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.
Comparing this latest output against my previous testing I noticed that the all_networks element has been removed (?).
./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
}
}
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.
c2f3225 to
016ab85
Compare
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 |
…ied table address count 28bac81 test: add functional test for getaddrmaninfo (stratospher) c8eb8da rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher) Pull request description: implements bitcoin#26907. split off from bitcoin#26988 to keep RPC, CLI discussions separate. This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman. ```jsx $ getaddrmaninfo Result: { (json object) json object with network type as keys "network" : { (json object) The network (ipv4, ipv6, onion, i2p, cjdns) "new" : n, (numeric) number of addresses in new table "tried" : n, (numeric) number of addresses in tried table "total" : n (numeric) total number of addresses in both new/tried tables from a network }, ... } ``` ### additional context from [original PR](bitcoin#26988) 1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see bitcoin#26988 (comment). 2. bitcoin#26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see bitcoin#26988 (comment). ACKs for top commit: 0xB10C: ACK 28bac81 willcl-ark: reACK 28bac81 achow101: ACK 28bac81 brunoerg: reACK 28bac81 theStack: Code-review ACK 28bac81 Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a
…ied table address count 28bac81 test: add functional test for getaddrmaninfo (stratospher) c8eb8da rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher) Pull request description: implements bitcoin#26907. split off from bitcoin#26988 to keep RPC, CLI discussions separate. This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman. ```jsx $ getaddrmaninfo Result: { (json object) json object with network type as keys "network" : { (json object) The network (ipv4, ipv6, onion, i2p, cjdns) "new" : n, (numeric) number of addresses in new table "tried" : n, (numeric) number of addresses in tried table "total" : n (numeric) total number of addresses in both new/tried tables from a network }, ... } ``` ### additional context from [original PR](bitcoin#26988) 1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see bitcoin#26988 (comment). 2. bitcoin#26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see bitcoin#26988 (comment). ACKs for top commit: 0xB10C: ACK 28bac81 willcl-ark: reACK 28bac81 achow101: ACK 28bac81 brunoerg: reACK 28bac81 theStack: Code-review ACK 28bac81 Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a
e6e444c refactor: add and use EnsureAnyAddrman in rpc (stratospher) bf589a5 doc: add release notes for bitcoin#27511 (stratospher) 3931e6a rpc: `getaddrmaninfo` followups (stratospher) Pull request description: - make `getaddrmaninfo` RPC public since it's not for development purposes only and regular users might find it useful. [bitcoin#26988 (comment)](bitcoin#26988 (comment)) - add missing `all_networks` key to RPC help. [bitcoin#27511 (comment)](bitcoin#27511 (comment)) - fix clang format spacing - add and use `EnsureAddrman` in RPC code. [bitcoin#27511 (comment)](bitcoin#27511 (comment)) ACKs for top commit: 0xB10C: Code Review re-ACK e6e444c theStack: Code-review ACK e6e444c pablomartin4btc: tested ACK e6e444c Tree-SHA512: c14090d5c64ff15e92d252578de2437bb2ce2e1e431d6698580241a29190f0a3528ae5b013c0ddb76a9ae538507191295c37cab7fd93469941cadbde44587072
…onal tests 2cc8ca1 [test] Use deterministic addrman in addrman info tests (stratospher) a897866 [test] Restart a node with empty addrman (stratospher) 71c1991 [test] Use deterministic addrman in addpeeraddress test (stratospher) 7b868e6 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher) 69e091f [init] Create deterministic addrman in tests using -test=addrman (stratospher) be25ac3 [init] Remove -addrmantest command line arg (stratospher) 802e6e1 [init] Add new command line arg for use only in functional tests (stratospher) Pull request description: An address is placed in a `[bucket,position]` in the addrman table (new table or tried table) using the `addpeeraddress` RPC. This `[bucket,position]` is calculated using `nKey`(and other metrics) for the addrman which is chosen randomly during every run. Supposing there are 2 addresses to be placed in an addrman table. During every test run, a different `[bucket,position]` would be calculated for each address.These calculated `[bucket,position]` could even be the same for the 2 addresses in some test runs and result in collisions in the addrman. We wouldn't be able to predict when the collisions are going to happen because we can't predict the `nKey` value which is chosen at random. This can cause flaky tests. Because of these non deterministic collisions, we are limited in what we can do to test addrman functionality. Currently in our tests don't add a second address to prevent these collisions from happening - we only keep 1 address in the new table and 1 address in the tried table. See bitcoin#26988 (comment), bitcoin#23084, [bitcoin#22831(comment)](https://github.com/bitcoin/bitcoin/pull/22831/files#r708302639). This PR lets us create a deterministic addrman with fixed `nKey` so that we can know the `[bucket,position]` collisions beforehand, safely add more addresses in an addrman table and write more extensive tests. ACKs for top commit: amitiuttarwar: ACK 2cc8ca1 achow101: ACK 2cc8ca1 0xB10C: ACK 2cc8ca1 mzumsande: Code Review ACK 2cc8ca1 Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189 # Conflicts: # src/addrdb.cpp # src/common/args.cpp # src/init.cpp # src/net.cpp # test/functional/feature_asmap.py # test/functional/rpc_net.py # test/functional/test_framework/test_framework.py
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.open questions:
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.