rpc: Add test-only RPC getaddrmaninfo for new/tried table address count

[stratospher]

implements #26907. split off from #26988 to keep RPC, CLI discussions separate.

This PR introduces a new RPC getaddrmaninfowhich 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.

$ 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

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 cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency #26988 (comment).
2. cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency #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 cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency #26988 (comment).

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	0xB10C, brunoerg, theStack, willcl-ark, achow101
Concept ACK	jonatack
Stale ACK	ryanofsky, mzumsande, amitiuttarwar

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:

• #26988 (cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency by stratospher)

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.

[jonatack]

This PR is based on master from two months age. You may need to rebase it to current master for the (tidy) CI to be green.

[jonatack]

Tested Concept ACK. It might be good to have the breakdown of pre/post IsTerrible filtering (RPC getnodeaddresses and CLI -addrinfo return post) -- see the large differences between them.

[brunoerg]

Concept ACK

[brunoerg]

tACK c505611

just tested it with my "clearnet only" node, results:

➜  bitcoin-core-dev git:(27511-stratospher) ✗ ./src/bitcoin-cli getaddrmaninfo
{
  "ipv4": {
    "new": 28765,
    "tried": 277,
    "total": 29042
  },
  "ipv6": {
    "new": 7245,
    "tried": 67,
    "total": 7312
  },
  "onion": {
    "new": 0,
    "tried": 0,
    "total": 0
  },
  "i2p": {
    "new": 0,
    "tried": 0,
    "total": 0
  },
  "cjdns": {
    "new": 0,
    "tried": 0,
    "total": 0
  }
}

[mzumsande]

Code Review ACK c505611, I would use this rpc a lot in my work.

I also think that a totals field would be useful.

I don't think that a split-up with respect to "terrible" would be necessary, because in my opinion "terrible" is an internal property used only in address relay / addrman collisions that is not so critical to expose because it isn't used for the main purpose of addrman: selecting peers for outbound connections

[jonatack]

"terrible" is an internal property

It's used in RPC getnodeaddresses and CLI -addrinfo to return only non-IsTerrible peers to users. (I meant pre and post that filtering in the suggestion above).

[mzumsande]

It's used in RPC getnodeaddresses and CLI -addrinfo to return only non-IsTerrible peers to users. (I meant pre and post that filtering in the suggestion above).

in my opinion unfortunately. I think this was mostly coincidental, because #13152 chose to reuse an addrman function used only for GETADDR answers before. Then #21595 used that output without any mentioning of that restriction, and the doc was only adjusted a year later in #24370. I think that ideally we never should have exposed the terrible filter in the first place.

[fanquake]

I think that ideally we never should have exposed the terrible filter in the first place.

@mzumsande do you want to propose some changes to remove its exposure?

[jonatack]

The author and the numerous reviewers of #13152 were aware of the relationship to getaddr (see the PR description and review discussion), as well as mention of IsTerrible in #13152 (review).

I don't see it as unfortunate; as a frequent user of getnodeaddresses and -addrinfo it seems useful that the data doesn't include terrible/non-recent (and banned/discouraged) peers.

But guessing whether it was coincidental is beside the point. What is relevant is that changing those calls to return different data would break user space, including any tracking over time using that data.

Adding data is fine, but let's not needlessly break user space.

[jonatack]

@stratospher do you plan to repush after rebase to current master for the CI and perhaps with the totals?

[mzumsande]

@mzumsande do you want to propose some changes to remove its exposure?

no, I don't want to spend time on discussions on whether some change breaks user space or not. I'd be happy to have an alternative way to query the actual, unfiltered numbers per network, which are much more relevant to my use cases as a developer.

I don't see it as unfortunate; as a frequent user of getnodeaddresses and -addrinfo it seems useful that the data doesn't include terrible/non-recent (and banned/discouraged) peers.

For me, it makes no sense that these commands might withhold our very next outbound peer - which would be picked with no disadvantage over non-terrible peers if it, for example, just has an older timestamp.
Or, when dealing with a peers.dat that is >30 days old, that these commands won't give any results at all anymore, even though the peers.dat is still perfectly useful and able to find peers (many nodes are reachable under the same address for way longer than a month).

[jonatack]

I'd be happy to have an alternative way to query the actual, unfiltered numbers per network

I think that would be useful, too, perhaps by passing an argument to getnodeaddresses and -addrinfo, and/or returning both pre and post filter in -addrinfo.

[stratospher]

so sorry for the delay and thank you for the suggestions! the CI is all green now. i've updated the PR to include:

• addrman new/tried/total count for "all networks"
• test coverage for "all networks" and for total (total refers to both new/tried table count)

I'd want this RPC to be the alternative way to access the actual count of all addresses in the addrman.

[mzumsande]

Tested ACK 7d7ee5e

[fanquake]

no, I don't want to spend time on discussions on whether some change breaks user space or not.

@mzumsande fair enough. I don't quite understand what "user space" means, when it has been used in comments here (and in other PRs), as we are not the Linux Kernel, but I assume it's a somewhat less direct way of saying backwards compatibility/continuity?

In any case, as far as I'm aware, we offer no special "backwards compatiblity", or other, guarantees for our CLI tools, and I don't really think we should. Doing so would put us in an awkward position, of having our RPCs backwards compatibility requirements, extended to those tools (consumers, which just happen to exist in this codebase).

We don't want to be in position where we can't change an RPC, to stop it doing something possibly incorrect/illogical, just because a CLI tool happens to call that RPC. These tools shouldn't get any special treatment, and, as far as I'm concerned, should just adapt to any RPC changes we might make, like every other consumer (internal or external) of that RPC.

[ryanofsky]

The code change here seems very simple, but since this PR does add a new RPC it would be useful to have more concept ACKs to know whether this is appropriate to merge. I noticed a number of contributors added github reactions to some comments here, so perhaps more of the people following the discussion could add concept acks or mention any reservations

[ryanofsky]

Code review ACK 7d7ee5e

[stratospher]

updated the PR to include better docs inspired from #27511 (comment). left "RPC is for testing only" as it is since all hidden RPCs have that line in the help. getaddrmaninfo's help looks like this now:

Provides high-level information about the node's address manager by returning the number of addresses in the `new` and `tried` tables and their sum for all networks.
This RPC is for testing only.

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. The new table contains addresses of potential new peers that are periodically tested for connectivity to increase the node's network reach.
    "tried" : n,    (numeric) number of addresses in tried table. The tried table contains addresses of peers with whom the node has successfully connected to in the past.
    "total" : n     (numeric) total number of addresses in both new/tried tables from a network
  },
  ...
}

Examples:
> bitcoin-cli getaddrmaninfo 
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getaddrmaninfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

[0xB10C]

ACK 28bac81

[brunoerg]

reACK 28bac81

CI failure seems unrelated.

[theStack]

Code-review ACK 28bac81

[willcl-ark]

reACK 28bac81

[achow101]

ACK 28bac81