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

stratospher · 2023-01-29T21:25:28Z

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 · 2023-01-29T21:25:31Z

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.

mzumsande

Concept ACK, will review soon.

test/functional/rpc_net.py

amitiuttarwar · 2023-02-06T18:54:28Z

obvious concept ack considering I proposed #26907

mzumsande

Looks good to me, only minor nits.

ping @jonatack, could you have a look, especially at the changes to bitcoin-cli?

src/rpc/net.cpp

src/bitcoin-cli.cpp

src/rpc/net.cpp

brunoerg

Concept ACK

src/rpc/net.cpp

brunoerg · 2023-02-22T13:23:51Z

"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 · 2023-03-07T19:45:11Z

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 · 2023-03-09T03:50:18Z

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 · 2023-03-09T14:11:04Z

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

going to review in depth soon.

amitiuttarwar · 2023-03-09T19:01:12Z

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.

ajtowns · 2023-09-28T03:07:55Z

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 · 2023-10-03T05:33:36Z

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.

pablomartin4btc

tested ACK 0b4ccbd

output before this PR

/src/bitcoin-cli -signet -addrinfo
{
  "addresses_known": {
    "ipv4": 1097,
    "ipv6": 352,
    "onion": 0,
    "i2p": 0,
    "cjdns": 0,
    "total": 1449
  }
}

output after this PR

./src/bitcoin-cli -signet -addrinfo
{
  "addresses_known": {
    "ipv4": 2157,
    "ipv6": 834,
    "onion": 0,
    "i2p": 0,
    "cjdns": 0,
    "all_networks": 2991,
    "total": 5982
  }
}

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.

doc/release-notes/release-notes-26988.md

e6e444c refactor: add and use EnsureAnyAddrman in rpc (stratospher) bf589a5 doc: add release notes for #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. [#26988 (comment)](#26988 (comment)) - add missing `all_networks` key to RPC help. [#27511 (comment)](#27511 (comment)) - fix clang format spacing - add and use `EnsureAddrman` in RPC code. [#27511 (comment)](#27511 (comment)) ACKs for top commit: 0xB10C: Code Review re-ACK e6e444c theStack: Code-review ACK e6e444c pablomartin4btc: tested ACK e6e444c Tree-SHA512: c14090d5c64ff15e92d252578de2437bb2ce2e1e431d6698580241a29190f0a3528ae5b013c0ddb76a9ae538507191295c37cab7fd93469941cadbde44587072

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 #26988 (comment), #23084, [#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

sr-gi · 2024-04-26T19:35:29Z

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

DrahtBot · 2024-05-13T08:07:18Z

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

stratospher · 2024-05-15T15:42:05Z

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 · 2024-05-15T15:42:58Z

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 (comment) which simply reports an error in case an older version of bitcoind is used with a newer bitcoin-cli.

jonatack · 2024-05-15T16:00:55Z

@stratospher Referencing the review feedback in #26988 (comment), 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 · 2024-05-15T16:03:46Z

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.

stratospher mentioned this pull request Jan 29, 2023

Expose addrman size info via RPC #26907

Closed

stratospher force-pushed the rpc_addrmaninfo branch 2 times, most recently from cac08f2 to caab213 Compare January 30, 2023 20:41

mzumsande reviewed Jan 30, 2023

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 2, 2023

cli: include local ("unroutable") peers in -netinfo table #26584

Merged

stratospher force-pushed the rpc_addrmaninfo branch from caab213 to c645fb6 Compare February 6, 2023 18:31

mzumsande reviewed Feb 6, 2023

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved

stratospher force-pushed the rpc_addrmaninfo branch from c645fb6 to 35bd55c Compare February 7, 2023 08:58

fanquake changed the title ~~[rpc]: Add test-only RPC addrmaninfo for new/tried table address count~~ rpc: Add test-only RPC addrmaninfo for new/tried table address count Feb 7, 2023

DrahtBot added the RPC/REST/ZMQ label Feb 7, 2023

amitiuttarwar reviewed Feb 8, 2023

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Feb 15, 2023

stratospher force-pushed the rpc_addrmaninfo branch from 35bd55c to a04bfa3 Compare February 21, 2023 20:38

DrahtBot removed the Needs rebase label Feb 21, 2023

stratospher changed the title ~~rpc: Add test-only RPC addrmaninfo for new/tried table address count~~ rpc: Add test-only RPC getaddrmaninfo for new/tried table address count Feb 22, 2023

brunoerg reviewed Feb 22, 2023

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

stratospher force-pushed the rpc_addrmaninfo branch from a04bfa3 to 7c34c35 Compare March 7, 2023 19:34

stratospher mentioned this pull request Oct 3, 2023

rpc: getaddrmaninfo followups #28565

Merged

stratospher added a commit to stratospher/bitcoin that referenced this pull request Oct 3, 2023

doc: add release notes for bitcoin#26988

0b4ccbd

stratospher force-pushed the rpc_addrmaninfo branch from c70d1c9 to 0b4ccbd Compare October 3, 2023 05:29

stratospher marked this pull request as ready for review October 3, 2023 05:35

DrahtBot removed the Needs rebase label Oct 3, 2023

pablomartin4btc reviewed Oct 16, 2023

View reviewed changes

doc/release-notes/release-notes-26988.md Outdated Show resolved Hide resolved

DrahtBot requested review from mzumsande, brunoerg and amitiuttarwar October 16, 2023 04:05

stratospher mentioned this pull request Dec 6, 2023

test: create deterministic addrman in the functional tests #29007

Merged

DrahtBot added the CI failed label Jan 13, 2024

achow101 requested a review from sr-gi April 9, 2024 14:39

DrahtBot marked this pull request as draft May 13, 2024 08:06

stratospher added a commit to stratospher/bitcoin that referenced this pull request May 15, 2024

doc: add release notes for bitcoin#26988

981aa3a

stratospher force-pushed the rpc_addrmaninfo branch from 0b4ccbd to 981aa3a Compare May 15, 2024 15:35

stratospher added 2 commits May 15, 2024 21:16

cli: modify -addrinfo to use getaddrmaninfo RPC endpoint

c6dfed3

doc: add release notes for bitcoin#26988

aebfac1

stratospher force-pushed the rpc_addrmaninfo branch from 981aa3a to aebfac1 Compare May 15, 2024 15:46

DrahtBot removed the CI failed label May 15, 2024

DrahtBot mentioned this pull request Jun 10, 2024

cli: restrict multiple exclusive argument usage in bitcoin-cli #30148

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stratospher commented Jan 29, 2023 •

edited

DrahtBot commented Jan 29, 2023 •

edited

mzumsande left a comment

amitiuttarwar commented Feb 6, 2023

mzumsande left a comment

brunoerg left a comment

brunoerg commented Feb 22, 2023 •

edited

stratospher commented Mar 7, 2023

amitiuttarwar commented Mar 9, 2023

brunoerg commented Mar 9, 2023 •

edited

amitiuttarwar commented Mar 9, 2023

ajtowns commented Sep 28, 2023

stratospher commented Oct 3, 2023

pablomartin4btc left a comment

sr-gi commented Apr 26, 2024

DrahtBot commented May 13, 2024

stratospher commented May 15, 2024

stratospher commented May 15, 2024 •

edited

jonatack commented May 15, 2024

jonatack commented May 15, 2024 •

edited

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

Are you sure you want to change the base?

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

Conversation

stratospher commented Jan 29, 2023 • edited

DrahtBot commented Jan 29, 2023 • edited

Code Coverage

Reviews

Conflicts

mzumsande left a comment

Choose a reason for hiding this comment

amitiuttarwar commented Feb 6, 2023

mzumsande left a comment

Choose a reason for hiding this comment

brunoerg left a comment

Choose a reason for hiding this comment

brunoerg commented Feb 22, 2023 • edited

stratospher commented Mar 7, 2023

amitiuttarwar commented Mar 9, 2023

brunoerg commented Mar 9, 2023 • edited

amitiuttarwar commented Mar 9, 2023

ajtowns commented Sep 28, 2023

stratospher commented Oct 3, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

sr-gi commented Apr 26, 2024

DrahtBot commented May 13, 2024

stratospher commented May 15, 2024

stratospher commented May 15, 2024 • edited

jonatack commented May 15, 2024

jonatack commented May 15, 2024 • edited

stratospher commented Jan 29, 2023 •

edited

DrahtBot commented Jan 29, 2023 •

edited

brunoerg commented Feb 22, 2023 •

edited

brunoerg commented Mar 9, 2023 •

edited

stratospher commented May 15, 2024 •

edited

jonatack commented May 15, 2024 •

edited