[rpc] Allow RPC to fetch all addrman records and add records to addrman #19658

jnewbery · 2020-08-04T11:49:04Z

Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to GetAddr(). Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, getnodeaddresses can now return the complete addrman, which is helpful for testing and monitoring.

Also add a test-only RPC addpeeraddress, which adds an IP address:port to addrman. This is helpful for testing (eg #18991).

jnewbery · 2020-08-04T11:49:15Z

cc @naumenkogs

naumenkogs · 2020-08-04T13:41:18Z

I think this is a move in the right direction: GetAddr() should be more dynamic.
It also seems to be useful for testing.

Concept ACK.

DrahtBot · 2020-08-04T17:31:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Make CAddrMan::cs non-recursive #19238 (refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto)
p2p: Try to preserve outbound block-relay-only connections during restart #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)
Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

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.

laanwj · 2020-08-05T14:50:17Z

Concept ACK, I think more control over addrman via RPC is a good thing.

mzumsande

Concept ACK, the getaddr limits only make sense for P2P, not for RPC.

src/rpc/net.cpp

mzumsande

I did a bit of testing via console, and everything worked as expected (after adding addpeeraddress to vRPCConvertParams).
Added some minor issues/nits.

src/rpc/net.cpp

src/net.h

src/net_processing.cpp

test/functional/rpc_net.py

jnewbery · 2020-08-11T09:04:27Z

Thanks for the review @mzumsande . All comments have been addressed.

src/addrman.cpp

src/addrman.h

src/bench/addrman.cpp

src/rpc/net.cpp

CAddrMan.GetAddr() would previously limit the number and percentage of addresses returned (to ADDRMAN_GETADDR_MAX (1000) and ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers responsibility to specify the maximum addresses and percentage they want returned. For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the client.

Allows addresses to be added to Address Manager for testing.

jnewbery · 2020-08-12T08:24:01Z

Thanks for the review @luke-jr . I've addressed all of your comments.

naumenkogs · 2020-08-12T10:05:24Z

utACK 37a480e
I think both methods would be useful.

You updated rpc_net.py test, but there's also p2p_getaddr_cache.py and p2p_addr_relay.py which may benefit from new methods?

jnewbery · 2020-08-12T10:13:46Z

there's also p2p_getaddr_cache.py and p2p_addr_relay.py which may benefit from new methods?

Of course. Those can be done in a follow-up.

laanwj · 2020-08-12T10:44:47Z

Code review and lightly manually tested ACK 37a480e
I found the default of 1 a bit confusing (because the RPC name is plural), but it's well documented so also no real problem with it.

jnewbery · 2020-08-12T11:19:13Z

Thanks @laanwj

I found the default of 1 a bit confusing (because the RPC name is plural), but it's well documented so also no real problem with it.

Yeah, I agree it's a bit confusing, but I didn't want to change the default in this PR.

e9d28da [Doc] Document getnodeaddresses in release notes (Fuzzbawls) ebe2a46 test: improve getnodeaddresses coverage, test by network (Fuzzbawls) 91ac5c7 rpc: enable filtering getnodeaddresses by network (Fuzzbawls) badfc49 p2p: allow CConnman::GetAddresses() by network, add doxygen (Fuzzbawls) fa78233 p2p: allow CAddrMan::GetAddr() by network, add doxygen (Fuzzbawls) a4d2733 p2p: pull time call out of loop in CAddrMan::GetAddr_() (Fuzzbawls) d5c1250 p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Fuzzbawls) 67e2c2f [RPC] add network field to getnodeaddresses (Fuzzbawls) b4832b2 [Net] Add addpeeraddress RPC method (Fuzzbawls) 1a31b67 [test] Test that getnodeaddresses() can return all known addresses (Fuzzbawls) e83fd0e [Addrman] Specify max addresses and pct when calling GetAddresses() (Fuzzbawls) 792655d [RPC] Add getnodeaddresses RPC command (chris-belcher) Pull request description: Implement a new RPC command (`getnodeaddresses`) that provides RPC access to the node's addrman. It may be useful for PIVX wallets to broadcast their transactions over tor for improved privacy without using the centralized DNS seeds. I've included upstream improvements to this feature here as well, and this RPC command was used to scrape the Tor v3 hardcoded seed nodes added in #2502 --------- Based on the following upstream PRs: * bitcoin#13152 * bitcoin#19658 * bitcoin#21594 * bitcoin#21843 ACKs for top commit: furszy: all good, tested ACK e9d28da. random-zebra: utACK e9d28da and merging... Tree-SHA512: 2e50108504ac8e172c2e31eb64813d6aae6b6819cf197eace2e4e15686906708b2f82262909faad00ce64af0f61945089a36b857064d3ce2398b3acb14e4ad7d

Summary: Commit description: > CAddrMan.GetAddr() would previously limit the number and percentage of > addresses returned (to ADDRMAN_GETADDR_MAX (1000) and > ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers > responsibility to specify the maximum addresses and percentage they want > returned. > > For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and > MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the > client. > PR description: > Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to GetAddr(). Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, getnodeaddresses can now return the complete addrman, which is helpful for testing and monitoring. > > Also add a test-only RPC addpeeraddress, which adds an IP address:port to addrman. This is helpful for testing (eg #18991). This is a backport of [[bitcoin/bitcoin#19658 | core#19658]] [1/3] bitcoin/bitcoin@f26502e Test Plan: `ninja all check-all` ``` ./src/bitcoin-cli getnodeaddresses 0 ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10069

Summary: This is a backport of [[bitcoin/bitcoin#19658 | core#19658]] [2/3] bitcoin/bitcoin@ae8051b Depends on D10069 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D10070

Summary: Allows addresses to be added to Address Manager for testing. This is a backport of [[bitcoin/bitcoin#19658 | core#19658]] [3/3] bitcoin/bitcoin@37a480e Depends on D10070 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10071

fanquake added RPC/REST/ZMQ P2P labels Aug 4, 2020

This was referenced Aug 4, 2020

refactor: Make CAddrMan::cs non-recursive #19238

Merged

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

DrahtBot mentioned this pull request Aug 6, 2020

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

mzumsande reviewed Aug 8, 2020

View reviewed changes

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

jnewbery force-pushed the 2020-07-addrman-get branch 3 times, most recently from c7e165d to 5bff6fd Compare August 8, 2020 22:16

mzumsande reviewed Aug 9, 2020

View reviewed changes

jnewbery force-pushed the 2020-07-addrman-get branch from 5bff6fd to 81cbe5e Compare August 11, 2020 09:04

luke-jr suggested changes Aug 11, 2020

View reviewed changes

jnewbery added 3 commits August 12, 2020 09:22

[test] Test that getnodeaddresses() can return all known addresses

ae8051b

[net] Add addpeeraddress RPC method

37a480e

Allows addresses to be added to Address Manager for testing.

jnewbery force-pushed the 2020-07-addrman-get branch from 81cbe5e to 37a480e Compare August 12, 2020 08:24

laanwj merged commit bd00d3b into bitcoin:master Aug 12, 2020

jnewbery deleted the 2020-07-addrman-get branch August 12, 2020 13:24

Fuzzbawls mentioned this pull request Aug 12, 2021

[RPC][Net] Add getnodeaddresses RPC command PIVX-Project/PIVX#2503

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rpc] Allow RPC to fetch all addrman records and add records to addrman #19658

[rpc] Allow RPC to fetch all addrman records and add records to addrman #19658

jnewbery commented Aug 4, 2020

jnewbery commented Aug 4, 2020

naumenkogs commented Aug 4, 2020

DrahtBot commented Aug 4, 2020 •

edited

laanwj commented Aug 5, 2020

mzumsande left a comment

mzumsande left a comment

jnewbery commented Aug 11, 2020

jnewbery commented Aug 12, 2020

naumenkogs commented Aug 12, 2020

jnewbery commented Aug 12, 2020

laanwj commented Aug 12, 2020 •

edited

jnewbery commented Aug 12, 2020

[rpc] Allow RPC to fetch all addrman records and add records to addrman #19658

[rpc] Allow RPC to fetch all addrman records and add records to addrman #19658

Conversation

jnewbery commented Aug 4, 2020

jnewbery commented Aug 4, 2020

naumenkogs commented Aug 4, 2020

DrahtBot commented Aug 4, 2020 • edited

Conflicts

laanwj commented Aug 5, 2020

mzumsande left a comment

Choose a reason for hiding this comment

mzumsande left a comment

Choose a reason for hiding this comment

jnewbery commented Aug 11, 2020

jnewbery commented Aug 12, 2020

naumenkogs commented Aug 12, 2020

jnewbery commented Aug 12, 2020

laanwj commented Aug 12, 2020 • edited

jnewbery commented Aug 12, 2020

DrahtBot commented Aug 4, 2020 •

edited

laanwj commented Aug 12, 2020 •

edited