Implement ETH JSON-RPC Rate Limiting #484

fabioberger · 2019-10-30T07:25:02Z

Currently, the number of Ethereum JSON-RPC requests a node makes scales with the number of orders being discovered. Instead, we want there to be a max cap, and if it is reached, have the node wait for more orders to accumulate before making the next RPC request.

There are two distinct caps, a per 24hr UTC time window cap, and a max per second cap. Both default to the Infura free-tier settings and are configurable by node operators.

Remaining TODOs:

Expose number of ETH JSON-RPC queries sent in stats response
Make sure we are rate-limiting order validation retry logic requests
Final review

fabioberger · 2019-10-30T17:33:14Z

ethereum/signer/sign.go

@@ -1,4 +1,4 @@
-package ethereum
+package signer


I had to move this into it's own sub-package in order to break a circular import error.

albrow

@fabio I still need to take a closer look at some parts of this PR (including the tests), but in the interest of time I am submitting my thoughts so far.

core/core.go

ratelimit/rate_limiter.go

ethereum/blockwatch/client.go

ratelimit/rate_limiter.go

ethereum/ethrpcclient/eth_rpc_client.go

ethereum/ethwatch/eth_watcher_test.go

albrow · 2019-10-31T22:39:41Z

ethereum/ratelimit/rate_limiter.go

+				// many from the bucket so that it starts empty for the next 24hr period
+				r.mu.Lock()
+				accruedGrants := r.maxRequestsPer24Hrs - r.grantedInLast24hrsUTC
+				if err := r.twentyFourHourLimiter.WaitN(ctx, accruedGrants); err != nil {


Shouldn't we use AllowN instead of WaitN here? There's no reason to block; I think we just want to take accruedGrants out of the bucket.

The documentation does not specify whether callingAllowN will deduct however many tokens have accrued before returning false. I dug into the code and if there aren't N tokens that have accrued, the request is rejected and the bucket is not emptied. We must use WaitN.

Hmm.. that's really annoying. @fabioberger are we confident that there is no situation where this could block?

Yes, I am confident this should never block. When we instantiate the rate limiter, we drain the bucket so that the number of accrued grants is theoreticalGrantsAccrued - grantedInLast24hrsUTC (where the theoretical grants accrued is calculated from the time elapsed since the start of the UTC day interval). Therefore the grants accrued left to be drained will be maxRequestsPer24Hrs - grantedInLast24hrsUTC.

ethereum/ratelimit/rate_limiter.go

ethereum/ratelimit/fake_rate_limiter.go

ethereum/ethwatch/eth_watcher_test.go

ethereum/ratelimit/rate_limiter_test.go

zeroex/orderwatch/order_watcher_test.go

…tadata collection

…proceed with Eth JSON-RPC requests

…go-routines spawned by the RateLimiter

…s with the RateLimiter

…it spawns have exited

…ackage

albrow

Just a few outstanding comments.

…at none block forever on the rate limiter

albrow

Approved after CI passes

fabioberger mentioned this pull request Oct 30, 2019

zeroex,orderwatch,blockwatch: Add upper-bound to # of JSON-RPC requests #372

Closed

fabioberger force-pushed the feature/ethRateLimiting branch from 332903d to 7653fdb Compare October 30, 2019 07:28

fabioberger changed the title ~~WIP: Implement ETH JSON-RPC Rate Limiting~~ Implement ETH JSON-RPC Rate Limiting Oct 30, 2019

fabioberger requested a review from albrow October 30, 2019 21:16

fabioberger added this to the Hardening Mesh milestone Oct 30, 2019

fabioberger commented Oct 30, 2019

View reviewed changes

albrow suggested changes Oct 30, 2019

View reviewed changes

albrow suggested changes Oct 31, 2019

View reviewed changes

fabioberger added 22 commits November 1, 2019 13:07

Added rate limiter library as a dep

5e5e2c9

Add clock dep

18807e9

Implement rate limiter and tests

b94ad8b

Store rate limiter values we wish to persist across restarts to DB me…

647740b

…tadata collection

Embed RateLimiter within Mesh application and wait for permission to …

1e0c879

…proceed with Eth JSON-RPC requests

Update dep lock

3318f3e

Fix linter error

a98972b

Add Eth JSON-RPC stats to mesh_getStats response

4350e64

Reference instance variable maxRequestsPer24Hrs

e83bc03

Simplify conditional

0788fe8

Defer closing the context

f8aa61e

cleanup RateLimiter

1705102

Improve comment

0b8bb80

Give delta a bit more buffer to avoid false negatives

ec1bc74

Use cancellable contexts and defer their cancellation to cleanup the …

5192c95

…go-routines spawned by the RateLimiter

Fix comments and values to match

819ea04

Add getUTCMidnightOfDate convenience method

7f694a2

Handle errors returned from Wait() if context is cancelled/exceeded

81a6d7c

Fix tests exept circular dep

53de0f6

Fix circlular dependency by moving EthWatcher into it's own package

34e965a

Remove print statement

485168d

Remove unused import

84465ba

fabioberger added 15 commits November 1, 2019 13:14

Add buffer to maxRequestsPer24Hrs to account for any unforeseen issue…

2d0101f

…s with the RateLimiter

Fix tests given new max 24hr request buffer

f99922b

Add parsing of options for browser-based Mesh

3a9b314

Fix naming issues and stutter in ethrpcclient package

2fc063b

Remove EthWatcher since unused

a36eb93

Use waitGroup to defer returning from Start() until both go-routines …

0a7bdb9

…it spawns have exited

Reduce stutter in name

dd91c06

Improve stutter and adherence to Go naming conventions in ratelimit p…

d55fd63

…ackage

Add comment above maxRequestsPer24HrsBuffer

62924dd

Remove unnecessary type conversion and declare as constants

c0a3e3d

Unnecessary type conversion

f303bac

Use wait group to wait for go-routine to exit before ending the test

d8ab4a7

Replace assert.Condition with assert.True

52c438d

Add new configs to browser package with defaults

d3735f9

Update leveldb dep

afe35fa

fabioberger force-pushed the feature/ethRateLimiting branch from 84d98ea to afe35fa Compare November 1, 2019 17:15

fabioberger added 3 commits November 1, 2019 13:20

Fix last few instances of networkID

cb67736

Remove outdated EthWatcher that was re-introduced during merge

32c2545

Remove extra character

e222347

albrow self-requested a review November 1, 2019 18:01

albrow suggested changes Nov 1, 2019

View reviewed changes

fabioberger added 5 commits November 1, 2019 18:10

Remove EthWatcher

435466d

Pass down context with appropriate timeout to all ETH RPC calls so th…

916a0df

…at none block forever on the rate limiter

Rename for clarity

36ad658

Set deltas to constants or compute them from other parameters

fa0f5fc

Log number of dropped rate limiter requests

4ad0252

albrow approved these changes Nov 2, 2019

View reviewed changes

Increase expected delta in min expected delay

122eb8c

fabioberger merged commit 6b232ff into development Nov 3, 2019

fabioberger deleted the feature/ethRateLimiting branch November 3, 2019 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ETH JSON-RPC Rate Limiting #484

Implement ETH JSON-RPC Rate Limiting #484

fabioberger commented Oct 30, 2019 •

edited

Loading

fabioberger Oct 30, 2019

albrow left a comment

albrow Oct 31, 2019

fabioberger Oct 31, 2019

albrow Nov 1, 2019

fabioberger Nov 1, 2019

albrow left a comment

albrow left a comment

Implement ETH JSON-RPC Rate Limiting #484

Implement ETH JSON-RPC Rate Limiting #484

Conversation

fabioberger commented Oct 30, 2019 • edited Loading

fabioberger Oct 30, 2019

Choose a reason for hiding this comment

albrow left a comment

Choose a reason for hiding this comment

albrow Oct 31, 2019

Choose a reason for hiding this comment

fabioberger Oct 31, 2019

Choose a reason for hiding this comment

albrow Nov 1, 2019

Choose a reason for hiding this comment

fabioberger Nov 1, 2019

Choose a reason for hiding this comment

albrow left a comment

Choose a reason for hiding this comment

albrow left a comment

Choose a reason for hiding this comment

fabioberger commented Oct 30, 2019 •

edited

Loading