net: rate limit the processing of incoming addr messages #3008
Conversation
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 messages. Every connection gets an associated token bucket. Processing an address in an ADDR message from non-whitelisted peers consumes a token from the bucket. If the bucket is empty, the address is ignored (it is not processed, stored or forwarded). 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 message). 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). This is similar to allowing bursts. The rate limit of 0.1 addr/s was copied from Bitcoin Core. Backported from: 0d64b8f, 5648138, f424d60 and d930c7f Original authors: Pieter Wuille <pieter@wuille.net>, and Jon Atack <jon@atack.com>
|
built and tested on x86_64-pc-linux-gnu running jammy lts. this time around the timing is much more consistent at 11-12 seconds avg after running about 10 times in a row. that aside i think i've identified the pertinent logs indicating this enhancement is working as intended, most notably: but have included the last test log i ran just in case: lgtm despite my uncertainty regarding the 'error'. |
That was fixed with #2986, not with this PR
That is actually not part of this change, but has always been part of the The actual result of this PR can be seen in the log you posted a little bit above that: |
yeah i remember 👍
gotcha ok great 👍 thx 🙏 |
| const uint64_t nCurrentTime = GetMockableTimeMicros(); | ||
| if (pfrom->nAddrTokenBucket < MAX_ADDR_PROCESSING_TOKEN_BUCKET) { | ||
| const uint64_t nTimeElapsed = std::max(nCurrentTime - pfrom->nAddrTokenTimestamp, uint64_t(0)); | ||
| const double nIncrement = nTimeElapsed * MAX_ADDR_RATE_PER_SECOND / 1e6; |
There was a problem hiding this comment.
I just noticed this could be 0, thanks to the previous line. That seems fine; I just want to ask if there's any reason it wouldn't be.
There was a problem hiding this comment.
0 will only happen when we run something like an ntp client that automatically adjusts forward drift in our system clock. In that case, we assign 0 credits because 0 or less time expired. I think this is ok and in fact rather defensive - worst case we will always allow only an additional 1000 addresses to be processed.
On the p2p network, sometimes custom nodes are sending us lots of peer addresses, the processing of which costs us CPU time and some data transfer (we try to announce every address we receive to 1-2 peers to keep network discovery going.) This was recently addressed on Bitcoin Core 22.0 by limiting the rate at which a node processes addresses received, helping to keep resource usage in check a little bit better.
This PR backports the relevant Bitcoin Core code (from
0d64b8f7,5648138f,f424d601andd930c7f5) with the following:addrmessage.addrmessage can contain up to 1000 addresses, we randomize the list order before processing, to make it unpredictable which addresses will go through and which won't.getaddrmessage (i.e. when we are actively in search of peers to connect to), we assign these peers that we've requested the data from an additional 1000 tokens on top of the already allocated tokens once, temporarily increasing past the limit of 1000. Since our node controls when we send outgetaddr, this is fully controlled by us, not the peer.getpeerinfo, allowing node operators to identify misbehaving peers.