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

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:

suggested here: https://github.com/bitcoin/bitcoin/pull/22387/files#r672414835

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

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

Conflicts