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

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.

DrahtBot commented at 9:25 pm on January 29, 2023: contributor

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.

stratospher force-pushed on Jan 30, 2023

in test/functional/rpc_net.py:343 in caab2131d6 outdated

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?

mzumsande commented at 6:23 pm on March 13, 2023:

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…

0while (len(node.getnodeaddresses(count=0)) < 2) {
1	node.addpeeraddress(...)
2}

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

obvious concept ack considering I proposed #26907

in src/rpc/net.cpp:980 in 418f811853 outdated

973@@ -974,6 +974,60 @@ static RPCHelpMan addpeeraddress()
974     };
975 }
976 
977+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:

done.

in src/bitcoin-cli.cpp:267 in c098f1c5a9 outdated

275     UniValue ProcessReply(const UniValue& reply) override
276     {
277         if (!reply["error"].isNull()) return reply;
278-        const std::vector<UniValue>& nodes{reply["result"].getValues()};
279-        if (!nodes.empty() && nodes.at(0)["network"].isNull()) {
280+        const std::vector<UniValue>& addrman{reply["result"].getValues()};

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

in src/rpc/net.cpp:977 in 35bd55c378 outdated

973@@ -974,6 +974,60 @@ static RPCHelpMan addpeeraddress()
974     };
975 }
976 
977+static RPCHelpMan addrmaninfo()

amitiuttarwar commented at 4:57 am on February 8, 2023:

realized it probably makes more sense to call it getaddrmaninfo to be consistent with other RPC messages

stratospher commented at 10:03 am on February 22, 2023:

true, i’ve updated the PR to call it getaddrmaninfo.

DrahtBot added the label Needs rebase on Feb 15, 2023

stratospher force-pushed on Feb 21, 2023

DrahtBot removed the label Needs rebase on Feb 21, 2023

stratospher renamed this:
~~rpc: Add test-only RPC addrmaninfo for new/tried table address count~~
rpc: Add test-only RPC getaddrmaninfo for new/tried table address count
on Feb 22, 2023

brunoerg commented at 11:55 am on February 22, 2023: contributor

Concept ACK

in src/rpc/net.cpp:1000 in a04bfa375b outdated

 995+                              throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled");
 996+                          }
 997+
 998+                          const std::optional<Network> network{request.params[0].isNull() ? std::nullopt : std::optional<Network>{ParseNetwork(request.params[0].get_str())}};
 999+                          if (network == NET_UNROUTABLE) {
1000+                              throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Network not recognized: %s", request.params[0].get_str()));

brunoerg commented at 11:57 am on February 22, 2023:

You could cover this error in the functional test

e.g.:

 0diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
 1index 68833fb85..90a9d9a17 100755
 2--- a/test/functional/rpc_net.py
 3+++ b/test/functional/rpc_net.py
 4@@ -337,5 +337,8 @@ class NetTest(BitcoinTestFramework):
 5         assert_equal(res[0]["new"], 1)
 6         assert_equal(res[0]["tried"], 1)
 7 
 8+        self.log.debug("Test that getaddrman with an invalid network throws an error")
 9+        assert_raises_rpc_error(-8, "Network not recognized: ipv8", self.nodes[1].getaddrmaninfo, "ipv8")
10+
11 if __name__ == '__main__':
12     NetTest().main()

stratospher commented at 7:35 pm on March 7, 2023:

thanks! done.

brunoerg commented at 1:23 pm on February 22, 2023: contributor

0"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 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:426 in 7c34c35b47 outdated

328@@ -328,6 +329,15 @@ def test_addpeeraddress(self):
329             addrs = node.getnodeaddresses(count=0)  # getnodeaddresses re-runs the addrman checks
330             assert_equal(len(addrs), 2)
331 
332+    def test_getaddrmaninfo(self):

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 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 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" not in 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

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

in test/functional/rpc_net.py:345 in bca0e42775 outdated

340+
341+        # current count of ipv4 addresses in addrman is {'new':1, 'tried':1}
342+        node.addpeeraddress(address="pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", tried=True, port=8333) # onion address
343+        node.addpeeraddress(address="c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", tried=False, port=8333)  # i2p address
344+        node.addpeeraddress(address="1233:3432:2434:2343:3234:2345:6546:4534", tried=False, port=8333)                       # ipv6 address
345+        node.addpeeraddress(address="fc00:1:2:3:4:5:6:7", tried=False, port=8333)                                            # cjdns address

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 bca0e42775 outdated

 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

0- CLI -addrinfo previously only returned a subset of addresses known to the node. It has been reworked to use the new `getaddrmaninfo` RPC and returns information about the entirety of the node's addrman. (#26988)

mzumsande commented at 2:56 pm on March 14, 2023:

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.

interesting, thanks! i’ve updated it.

jonatack commented at 7:52 pm on March 22, 2023:

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.

(I agree the note was too low-level originally.)

fanquake commented at 11:24 am on March 23, 2023:

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

jonatack commented at 11:22 pm on March 23, 2023:

since it improves the results of -addrinfo

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).

jonatack commented at 11:26 pm on March 23, 2023:

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.

thanks @jonatack! this sounds much better.

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 81ad8c60a9 outdated

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

brunoerg commented at 6:55 pm on March 15, 2023:

What’s the reason for -cjdnsreachable here?

stratospher commented at 7:04 pm on March 15, 2023:

oops, forgot to revert that. thanks!

brunoerg commented at 7:09 pm on March 15, 2023:

ohh, got it! thank you!

stratospher referenced this in commit 1bddd77cbc on Mar 15, 2023

stratospher force-pushed on Mar 15, 2023

in src/bitcoin-cli.cpp:264 in 2fa935b389 outdated

270         if (!args.empty()) {
271             throw std::runtime_error("-addrinfo takes no arguments");
272         }
273-        UniValue params{RPCConvertValues("getnodeaddresses", std::vector<std::string>{{"0"}})};
274-        return JSONRPCRequestObj("getnodeaddresses", params, 1);
275+        return JSONRPCRequestObj("getaddrmaninfo", NullUniValue, 1);

jonatack commented at 7:44 pm on March 22, 2023:

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 !

jonatack commented at 11:11 pm on March 23, 2023:

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.

jonatack commented at 11:17 pm on March 23, 2023:

(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.)

jonatack commented at 11:44 am on March 24, 2023:

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).

 0{
 1  "addresses known, after quality/recency filtering (original -addrinfo)": {
 2    "ipv4": 1,
 3    "ipv6": 0,
 4    "onion": 14673,
 5    "i2p": 1129,
 6    "cjdns": 7,
 7    "total": 15810
 8  },
 9  "all addresses known (no filtering)": {
10    "ipv4": 1,
11    "ipv6": 0,
12    "onion": 14673,
13    "i2p": 1129,
14    "cjdns": 7,
15    "total": 15810
16  }
17}

mzumsande commented at 3:14 pm on March 24, 2023:

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.

jonatack commented at 3:44 pm on March 24, 2023:

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.

jonatack commented at 4:43 pm on April 21, 2023:

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.

jonatack commented at 5:16 pm on April 21, 2023:

(See #27511 (comment) for a current example on mainnet of how different the data can be.)

stratospher commented at 4:57 pm on May 15, 2023:

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.

jonatack commented at 3:50 pm on May 15, 2024:

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: contributor

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 1bddd77cbc outdated

965@@ -966,6 +966,60 @@ static RPCHelpMan addpeeraddress()
966     };
967 }
968 
969+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",

jonatack commented at 8:40 pm on March 22, 2023:

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

 0$ ./src/bitcoin-cli -signet -addrinfo                 
 1{
 2  "addresses_known": {
 3    "ipv4": 2730,
 4    "ipv6": 1011,
 5    "onion": 167,
 6    "i2p": 32,
 7    "cjdns": 0,
 8    "total": 3940
 9  }
10}
11$ ./src/bitcoin-cli -signet getaddrmaninfo           
12[
13  {
14    "network": "ipv4",
15    "new": 2673,
16    "tried": 57
17  },
18  {
19    "network": "ipv6",
20    "new": 981,
21    "tried": 30
22  },
23  {
24    "network": "onion",
25    "new": 101,
26    "tried": 66
27  },
28  {
29    "network": "i2p",
30    "new": 20,
31    "tried": 12
32  },
33  {
34    "network": "cjdns",
35    "new": 0,
36    "tried": 0
37  }
38]

stratospher commented at 4:30 pm on April 21, 2023:

you’re right! I’ve updated getaddrmaninfo taking both these suggestions.

ajtowns commented at 2:18 am on March 30, 2023: contributor

My opinion only, but it seems like it would be simpler to have the RPC not take any arguments and always output an object:

0$ bitcoin-cli getaddrinfo
1{
2    "ipv4": {
3        "new": 5,
4        "tried": 3
5    },
6    ...
7}

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.

That sounds reasonable. Done in #28565.

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 0b4ccbd035 outdated

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.

pablomartin4btc commented at 3:47 am on October 16, 2023:

Should we be more specific perhaps?

0- CLI -addrinfo previously (v22-v24) returned addresses known to the node after filtering for quality and recency.

stratospher commented at 3:39 pm on May 15, 2024:

liked it, done.

pablomartin4btc commented at 4:05 am on October 16, 2023: member

tested ACK 0b4ccbd0358d95632e2e46bfdf7715b199971754

 0/src/bitcoin-cli -signet -addrinfo
 1{
 2  "addresses_known": {
 3    "ipv4": 1097,
 4    "ipv6": 352,
 5    "onion": 0,
 6    "i2p": 0,
 7    "cjdns": 0,
 8    "total": 1449
 9  }
10}

 0./src/bitcoin-cli -signet -addrinfo
 1{
 2  "addresses_known": {
 3    "ipv4": 2157,
 4    "ipv6": 834,
 5    "onion": 0,
 6    "i2p": 0,
 7    "cjdns": 0,
 8    "all_networks": 2991,
 9    "total": 5982
10  }
11}

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

What is the state of this @stratospher? Are you still looking for feedback on #26988 (comment)?

DrahtBot marked this as a draft on May 13, 2024

DrahtBot commented at 8:07 am on May 13, 2024: contributor

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

stratospher referenced this in commit 981aa3ae28 on May 15, 2024

stratospher force-pushed on May 15, 2024

stratospher commented at 3:42 pm on May 15, 2024: contributor

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 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.

cli: modify -addrinfo to use getaddrmaninfo RPC endpoint c6dfed37ff

doc: add release notes for #26988 aebfac1344

stratospher force-pushed on May 15, 2024

jonatack commented at 4:00 pm on May 15, 2024: contributor

@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: contributor

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

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

Code Coverage

Reviews

Conflicts