Use a client side rate limit to reduce chance of getting banned

[prestonvanloon]

Resolves #4588
Resolves #4621

Maybe helps #4631

Part of the issue is that when there are large ranges of skip slots, we might be asking for the peer for many blocks very quickly. Even when there are no blocks present in that range, the server increments that rate limit by the request count. Adding this client side rate limit should reduce the risk of bombing peers with requests.

[mcdee]

Even when there are no blocks present in that range, the server increments that rate limit by the request count

Is this valid for the server to do? It it a similar magnitude of work to check for empty slots as it is to serve those with blocks in it? Or should the rate limiting be done on actual blocks served?

[prestonvanloon]

@mcdee I think it's valid. The client is knowingly requesting a high rate of blocks/slots per second, although they do not know if there are actually blocks in those slots.

[mcdee]

True, but the only reason it is requesting more blocks soon after the last request is because there were few-to-none blocks returned last time around.

Given this situation only really occurs when we have finality issues, I'm wondering if being more lenient on the server side here would help the network get back on its feet if such a thing happened in production. I'm not suggesting the server hurt itself in the process, but if it is cheap and easy to return 0 blocks I'd be inclined to reflect that in the rate limiting.

[prestonvanloon]

Supporting that type of rate limiting will require some database changes to determine how many blocks are present in a given range without unmarshalling them. As it is now, we check the rate limit, if OK then we fetch the request range of blocks.

We could enhance this process to be:

• Secure peer ID based mutex
• Fetch up to min(remainingLimit, req.Count) blocks from db in the requested range
• Increment limit by the amount of blocks returned
• Return blocks

For now it's simpler on the server side to check if the requested count exceeds the limit and exit early without a database read. Given that the limit allows bursts of 320 blocks per second, I think this is a reasonable response from any given peer.

[codecov]

Codecov Report

Merging #4637 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4637   +/-   ##
======================================
  Coverage     6.9%    6.9%           
======================================
  Files         192     192           
  Lines       13374   13374           
======================================
  Hits          923     923           
  Misses      12313   12313           
  Partials      138     138