p2p, rpc, test: address rate-limiting follow-ups #22604

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:rate_limit_addr_follow-ups changing 3 files +21 −22

jonatack commented at 2:50 PM on August 2, 2021: member

Incorporates review feedback in #22387.

Edit, could be considered separately: should a release note (or two) be added for 22.0? e.g. the new getpeerinfo fields in Updated RPCs and the rate-limiting itself in P2P and network changes.
DrahtBot added the label P2P on Aug 2, 2021
DrahtBot added the label RPC/REST/ZMQ on Aug 2, 2021
theStack commented at 8:50 PM on August 2, 2021: member

Concept ACK

Will code-review tomorrow. At a first glance, most parts of the diff look straight-forward, the change in the rate-limiting logic (swapping the if conditions) needs probably most attention. To speed up review, adding links to each of the review comments in #22387 that are tackled in this PR could be a good idea.

(the AppVeyor CI build fail seems to be not caused by this PR's changes, by the way)

in src/net_processing.cpp:266 in b7c4b058ba outdated

 240 | @@ -241,14 +241,14 @@ struct Peer {
 241 |      std::atomic_bool m_wants_addrv2{false};
 242 |      /** Whether this peer has already sent us a getaddr message. */
 243 |      bool m_getaddr_recvd{false};
 244 | -    /** Number of addr messages that can be processed from this peer. Start at 1 to
 245 | +    /** Number of addresses that can be processed from this peer. Start at 1 to

jonatack commented at 8:57 PM on August 2, 2021:

addresses the feedback here: https://github.com/bitcoin/bitcoin/pull/22387/files#r665882202

in src/net_processing.cpp:2858 in b7c4b058ba outdated

2824 |                      ++num_rate_limit;
2825 |                      continue;
2826 |                  }
2827 | +            } else {
2828 |                  peer->m_addr_token_bucket -= 1.0;
2829 |              }

jonatack commented at 8:58 PM on August 2, 2021:

proposed here: https://github.com/bitcoin/bitcoin/pull/22387/files#r672410564

in src/net_processing.cpp:2885 in b7c4b058ba outdated

2855 | -                 num_proc,
2856 | -                 num_rate_limit,
2857 | -                 pfrom.GetId(),
2858 | -                 fLogIPs ? ", peeraddr=" + pfrom.addr.ToString() : "");
2859 | +        LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d\n",
2860 | +                 vAddr.size(), num_proc, num_rate_limit, pfrom.GetId());

jonatack commented at 8:58 PM on August 2, 2021:

in src/rpc/net.cpp:155 in b7c4b058ba outdated

 149 | @@ -150,6 +150,8 @@ static RPCHelpMan getpeerinfo()
 150 |                              {
 151 |                                  {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"},
 152 |                              }},
 153 | +                            {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"},
 154 | +                            {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"},

jonatack commented at 8:59 PM on August 2, 2021:

mentioned here: https://github.com/bitcoin/bitcoin/pull/22387/files#r672417898

in test/functional/p2p_addr_relay.py:367 in b7c4b058ba outdated

 300 |              self.mocktime += 1000
 301 |              self.nodes[0].setmocktime(self.mocktime)
 302 |              peer.increment_tokens(100)
 303 |  
 304 | -            self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1610)
 305 | +            self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=200, total_addrs=1610)

jonatack commented at 9:00 PM on August 2, 2021:

review feedback here: https://github.com/bitcoin/bitcoin/pull/22387/files#r671144448

jonatack commented at 9:02 PM on August 2, 2021: member

To speed up review, adding links to each of the review comments in #22387 that are tackled in this PR could be a good idea.

Good idea--added!
sipa commented at 9:07 PM on August 2, 2021: member

Concept ACK
tryphe commented at 4:41 AM on August 3, 2021: contributor

Concept ACK

Nice fixups! Looks like there's essentially only one functional change here with if (rate_limited) {....
MarcoFalke commented at 7:13 AM on August 3, 2021: member

review ACK b7c4b058ba2625a39666636253620bc08a41c501

Thanks!

Also reset appveyor run.
MarcoFalke commented at 7:14 AM on August 3, 2021: member

Looks like there's essentially only one functional change here with if (rate_limited) {....

I think it still counts as refactoring because the change shouldn't be observable by a user
DrahtBot commented at 8:30 PM on August 3, 2021: member



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



Conflicts

No conflicts as of last run.
theStack approved
theStack commented at 11:48 PM on August 3, 2021: member

Code-review ACK b7c4b058ba2625a39666636253620bc08a41c501
DrahtBot added the label Needs rebase on Aug 4, 2021
p2p, rpc, test: address rate-limiting follow-ups d930c7f5b0
jonatack force-pushed on Aug 4, 2021
jonatack commented at 5:19 PM on August 4, 2021: member

Rebased per git range-diff 2b06af1 b7c4b05 d930c7f
DrahtBot removed the label Needs rebase on Aug 4, 2021
theStack commented at 9:31 AM on August 5, 2021: member

re-ACK d930c7f5b091687eb4208a5ffe8a2abe311d8054 🌮

Checked via git range-diff b7c4b058...d930c7f5 that the changes since my last ACK were only rebase-related.
MarcoFalke commented at 9:42 AM on August 5, 2021: member

review ACK d930c7f5b091687eb4208a5ffe8a2abe311d8054
Zero-1729 commented at 1:31 PM on August 13, 2021: contributor

crACK d930c7f

LGTM 🧉
fanquake merged this on Aug 14, 2021
fanquake closed this on Aug 14, 2021
jonatack deleted the branch on Aug 14, 2021
sidhujag referenced this in commit df1a98755d on Aug 15, 2021
Fabcien referenced this in commit b9ceb82661 on Feb 1, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#22387 Rate limit the processing of rumoured addresses

p2p, rpc, test: address rate-limiting follow-ups #22604

Conflicts