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

[stratospher]

Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in https://github.com/bitcoin/bitcoin/pull/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 https://github.com/bitcoin/bitcoin/pull/24370.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	mzumsande, brunoerg
Stale ACK	amitiuttarwar, pablomartin4btc

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:

• #30148 (cli: restrict multiple exclusive argument usage in bitcoin-cli by naiyoma)

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.

[amitiuttarwar]

obvious concept ack considering I proposed #26907

[brunoerg]

"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]

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]

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]

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

going to review in depth soon.

[amitiuttarwar]

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]

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]

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]

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

[jonatack]

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 https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145326660)

[ajtowns]

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]

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 https://github.com/bitcoin/bitcoin/pull/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 https://github.com/bitcoin/bitcoin/pull/27511 to use these.
• Haven't changed the previous CLI approach because of https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173966799.

[stratospher]

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]

Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511.

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

[amitiuttarwar]

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]

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

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 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 https://github.com/bitcoin/bitcoin/pull/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.

[ajtowns]

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]

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.

[sr-gi]

What is the state of this @stratospher? Are you still looking for feedback on https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1548215577?

[DrahtBot]

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

[stratospher]

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]

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 https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173966799 which simply reports an error in case an older version of bitcoind is used with a newer bitcoin-cli.

[jonatack]

@stratospher Referencing the review feedback in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145326660, 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]

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.