[0.21] Rate limit the processing of rumoured addresses

sipa commented at 4:36 pm on July 28, 2021: member

Backport of #22387.

The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that’s orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.

This PR implements a token bucket based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers’ self-announcement.

Due to the lack of the Peer struct in 0.21, the relevant fields have been added to CNodeState instead, necessitating additional locks, and slightly different structure to avoid too much cs_main grabbing. The last test-improving commit has also been dropped, as the code has changed too much. Most of the behavior is still tested however, just not the part that compares with RPC statistics.

DrahtBot added the label Backport on Jul 28, 2021

sipa force-pushed on Jul 28, 2021

fanquake added this to the milestone 0.21.2 on Jul 28, 2021

sipa force-pushed on Jul 29, 2021

in src/net_processing.cpp:405 in 55fcbde5a9 outdated

400+    /** When m_addr_token_bucket was last updated */
401+    std::chrono::microseconds m_addr_token_timestamp{GetTime<std::chrono::microseconds>()};
402+    /** Total number of addresses that were dropped due to rate limiting. */
403+    std::atomic<uint64_t> m_addr_rate_limited{0};
404+    /** Total number of addresses that were processed (excludes rate limited ones). */
405+    std::atomic<uint64_t> m_addr_processed{0};

jnewbery commented at 10:34 am on July 29, 2021:

These don’t strictly need to be atomic since they’re guarded by cs_main now, but there’s no harm in leaving them like this.

sipa commented at 4:07 pm on July 29, 2021:

Agree, I may address this if I retouch.

jnewbery commented at 4:28 pm on July 29, 2021:

Yeah, no need to change the branch. Just pointing it out in case it confused other reviewers.

jnewbery commented at 10:35 am on July 29, 2021: member

utACK 55fcbde5a9fc35b187a8cb31004e5ce94bdbbedd

vasild approved

vasild commented at 3:30 pm on July 29, 2021: member

ACK 55fcbde5a9fc35b187a8cb31004e5ce94bdbbedd

sipa force-pushed on Jul 29, 2021

jnewbery commented at 4:47 pm on July 29, 2021: member

reACK 651216e70b0cd1aaa1318db07e1d89d9cbb65cf3

sipa force-pushed on Jul 29, 2021

sipa commented at 7:05 pm on July 29, 2021: member

@sipsorcery @fanquake Any idea what is causing the appveyor failure here?

sipsorcery commented at 8:07 pm on July 29, 2021: member

I can fix the compiler error by adjusting misc.cpp line 390 to the below. I’m not enough of a C++ expert to say why.

0  if (request.context.Has<NodeContext>()) {
1        auto & chainClients = request.context.Get<NodeContext>().chain_clients;
2        for (const auto& chain_client : chainClients) {
3            chain_client->setMockTime(time);
4        }
5    }

MarcoFalke referenced this in commit 068ac69b56 on Jul 30, 2021

vasild approved

vasild commented at 9:59 am on July 30, 2021: member

ACK 33332768c7842a665679f4bcccc4504c542f09e6

I don’t understand how this:

0-        for (const auto& chain_client : request.context.Get<NodeContext>().chain_clients) {
1+        const auto& chain_clients = request.context.Get<NodeContext>().chain_clients;
2+        for (const auto& chain_client : chain_clients) {

could fix this:

0C:\projects\bitcoin\src\rpc\misc.cpp(396,1): error C2355: 'this': can only be referenced inside non-static member functions or non-static data member initializers

MarcoFalke commented at 10:01 am on July 30, 2021: member

I don’t understand how this: …

Probably a msvc bug in C++11 mode?

sipsorcery commented at 10:16 am on July 30, 2021: member

Probably a msvc bug in C++11 mode?

The msvc build config uses C++14. As an experiment I did try with msvc & C++17 and it generated the same error.

MarcoFalke commented at 11:11 am on July 30, 2021: member

Looks like the bug started happening unrelated to any of our changes between May 31st and June 19th (https://github.com/bitcoin/bitcoin/commit/926f76cb205ce9ef085f4945cdb439746b140414)

sipa commented at 3:37 am on July 31, 2021: member

Hmm, now a fuzzer job times out?

jnewbery commented at 12:13 pm on August 3, 2021: member

reACK 33332768c7

fanquake added this to the "Blockers" column in a project

achow101 commented at 8:31 pm on August 3, 2021: member

ACK 33332768c7842a665679f4bcccc4504c542f09e6

MarcoFalke commented at 2:45 pm on August 5, 2021: member

I don’t understand why this needs the cs_main lock. The following backport compiled just fine:

 0diff --git a/src/net_permissions.h b/src/net_permissions.h
 1index bba0ea1695..3b841ab138 100644
 2--- a/src/net_permissions.h
 3+++ b/src/net_permissions.h
 4@@ -32,3 +32,4 @@ enum NetPermissionFlags {
 5     PF_MEMPOOL = (1U << 5),
 6-    // Can request addrs without hitting a privacy-preserving cache
 7+    // Can request addrs without hitting a privacy-preserving cache, and send us
 8+    // unlimited amounts of addrs.
 9     PF_ADDR = (1U << 7),
10diff --git a/src/net_processing.cpp b/src/net_processing.cpp
11index f29be8d8a3..019d00e7d6 100644
12--- a/src/net_processing.cpp
13+++ b/src/net_processing.cpp
14@@ -148,2 +148,9 @@ static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
15 static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23;
16+/** The maximum rate of address records we're willing to process on average. Can be bypassed using
17+ *  the NetPermissionFlags::Addr permission. */
18+static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1};
19+/** The soft limit of the address processing token bucket (the regular MAX_ADDR_RATE_PER_SECOND
20+ *  based increments won't go above this, but the MAX_ADDR_TO_SEND increment following GETADDR
21+ *  is exempt from this limit. */
22+static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND};
23 
24@@ -456,2 +463,8 @@ struct Peer {
25 
26+    /** Number of addr messages that can be processed from this peer. Start at 1 to
27+     *  permit self-announcement. */
28+    double m_addr_token_bucket{1.0};
29+    /** When m_addr_token_bucket was last updated */
30+    std::chrono::microseconds m_addr_token_timestamp{GetTime<std::chrono::microseconds>()};
31+
32     Peer(NodeId id) : m_id(id) {}
33@@ -2440,2 +2453,5 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
34             pfrom.fGetAddr = true;
35+            // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response
36+            // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit).
37+            peer->m_addr_token_bucket += MAX_ADDR_TO_SEND;
38         }
39@@ -2593,2 +2609,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
40         int64_t nSince = nNow - 10 * 60;
41+
42+        // Update/increment addr rate limiting bucket.
43+        const auto current_time = GetTime<std::chrono::microseconds>();
44+        if (peer->m_addr_token_bucket < MAX_ADDR_PROCESSING_TOKEN_BUCKET) {
45+            // Don't increment bucket if it's already full
46+            const auto time_diff = std::max(current_time - peer->m_addr_token_timestamp, std::chrono::microseconds{0});
47+            const double increment = std::chrono::duration<double>(time_diff).count() * MAX_ADDR_RATE_PER_SECOND;
48+            peer->m_addr_token_bucket = std::min<double>(peer->m_addr_token_bucket + increment, MAX_ADDR_PROCESSING_TOKEN_BUCKET);
49+        }
50+        peer->m_addr_token_timestamp = current_time;
51+
52+        const bool rate_limited = !pfrom.HasPermission(NetPermissionFlags::PF_ADDR);
53         for (CAddress& addr : vAddr)
54@@ -2598,2 +2626,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
55 
56+            // Apply rate limiting.
57+            if (rate_limited) {
58+                if (peer->m_addr_token_bucket < 1.0) break;
59+                peer->m_addr_token_bucket -= 1.0;
60+            }
61             // We only bother storing full nodes, though this may include

MarcoFalke commented at 3:30 pm on August 5, 2021: member

merging this is also fine, if you don’t want to retouch. The only difference should be unneeded cs_main locks

sipa commented at 3:33 pm on August 5, 2021: member

@MarcoFalke Is that a backport? Peer doesn’t exist in 0.21.

MarcoFalke commented at 3:35 pm on August 5, 2021: member

https://github.com/bitcoin/bitcoin/blob/0.21/src/net_processing.cpp#L438

Rate limit the processing of incoming addr messages

While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.

This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).

The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.

Github-Pull: #22387
Rebased-From: 0d64b8f709b4655d8702f810d4876cd8d96ded82

83dfe6c65e

Randomize the order of addr processing

Github-Pull: #22387
Rebased-From: 5648138f5949013331c017c740646e2f4013bc24

8df3e5bd84

Functional tests for addr rate limiting

Github-Pull: #22387
Rebased-From: b4ece8a1cda69cc268d39d21bba59c51fa2fb9ed

aaa4833fc9

Add logging and addr rate limiting statistics

Includes logging improvements by Vasil Dimov and John Newbery.

Github-Pull: #22387
Rebased-From: f424d601e1b6870e20bc60f5ccba36d2e210377b

a653aacbd6

sipa commented at 4:42 pm on August 5, 2021: member

@MarcoFalke It would appear that I need better glasses. Fixed.

sipa force-pushed on Aug 5, 2021

Avoid Appveyor compilation failure 2a57108051

achow101 commented at 7:12 pm on August 5, 2021: member

ACK 2a5710805195ca54a02aff3540ceaefb9cb3b3e2

GeneFerneau commented at 10:07 pm on August 5, 2021: none

Approach + code review ACK 2a57108

jnewbery commented at 9:49 am on August 6, 2021: member

reACK 2a5710805195ca54a02aff3540ceaefb9cb3b3e2

MarcoFalke merged this on Aug 6, 2021

MarcoFalke closed this on Aug 6, 2021

laanwj removed this from the "Blockers" column in a project

ComputerCraftr referenced this in commit 4124c4014a on Aug 18, 2021

tcharding referenced this in commit 433ac6fcef on Jul 22, 2022

tcharding referenced this in commit 60a2417a8f on Aug 3, 2022

DrahtBot locked this on Aug 16, 2022

[0.21] Rate limit the processing of rumoured addresses #22569