[proxyd]: Add block range rate limiting, and max block range to non-consensus mode #114
Conversation
…ing for logs per block
|
Lmk if this is ready for review, typically would like to see CI passing as a prerequisite |
mdehoog
force-pushed
the
michael/block-range
branch
4 times, most recently
from
22d61a5 to
9891c81
Compare
mdehoog
force-pushed
the
michael/block-range
branch
from
9891c81 to
d854e76
Compare
| // we are concerned about network error rates, so we record 1 request independently of how many are in the batch | ||
| b.networkRequestsSlidingWindow.Incr() | ||
| // (we don't count non-consensus polling towards error rates) |
There was a problem hiding this comment.
The alternative to this is to disable the non-consensus poller in more tests by setting the nonconsensus_poller_interval negative... what do you think? To me it feels like we shouldn't be counting polling towards error rates, but I don't feel strongly.
There was a problem hiding this comment.
I think the sliding network requests sliding window should be counted. IRRC, its counted for the consensus_poller, I think it would be odd if it was counted for one strategy but not the other
There was a problem hiding this comment.
Additionally if the poll fails that helps proxyd determine if the backend is unstable at the moment, which is why I'd lean toward tracking the intermittent sliding window error as well
|
@jelias2 thank you 🙏 Apologies, I thought tests were passing when I marked this as ready for review, but I now realize it was only the |
|
|
||
| func ExtractBlockRange(req *RPCReq, tracker BlockNumberTracker) *BlockRange { | ||
| switch req.Method { | ||
| case "eth_getLogs", "eth_newFilter": |
There was a problem hiding this comment.
eth_newFilter is only appended to a single backend, so it quite difficult to get the results from the requesting the filter through proxyd. We typically don't expose it on proxyd on our end. Curious your experience with it
A while ago there was a PR for similar functionality: #105, but I didn't see any interest in at the time.
There was a problem hiding this comment.
i guess eth_newFilter is used for overwriting the reqeust parameters, so it makes sense to keep it in here, but still I don't think that method works perfectly
| @@ -182,6 +188,8 @@ type BackendGroupConfig struct { | |||
| ConsensusHALockPeriod TOMLDuration `toml:"consensus_ha_lock_period"` | |||
| ConsensusHARedis RedisConfig `toml:"consensus_ha_redis"` | |||
|
|
|||
| NonconsensusPollerInterval TOMLDuration `toml:"nonconsensus_poller_interval"` | |||
There was a problem hiding this comment.
Overall I'm curious why the non-consensus poller is needed? If a backend was unable to serve a eth_getLogs in fallback mode, it would just request the next backend in the backend group? Whats the benefit of adding this component?
There was a problem hiding this comment.
The consensus poller allows re-writing of latest, safe, and finalized, I guess the major drawback would be if a eth_getLogs request goes a backend which doesn't have the non-consensus latest if would be missing the latest logs. Could you provide a bit more background on the behavior that this PR aims to fix and the expected results?
| return incr.Val()-1 < int64(r.max), nil | ||
| return incr.Val() <= int64(r.max), nil |
There was a problem hiding this comment.
Just curious why removing the -1
| p.lock.Lock() | ||
| defer p.lock.Unlock() | ||
|
|
||
| if *ptr < value || *ptr == math.MaxUint64 { |
There was a problem hiding this comment.
Whats the purpose of *ptr == math.MaxUnit64 in the conditional? Is this if the ptr is uninitialzed?
|
Overall the blockrange rate limiting looks good if you want to open a second PR for that. For the max block range on non-consensus mode would like a bit more context. Thanks for the contribution! |
Description
This PR implements two things:
Tests
Added new tests.
Additional context
ethereum-optimism/optimism#6686 originally implemented the ability to limit block range requests. The initial implementation supported both consensus and non-consensus mode, however there was feedback on that PR that we already had latest block polling in consensus mode, and any block range limiting should be added to that.
We've since tried to use consensus mode, and even opened some PRs to help (e.g. ethereum-optimism/optimism#7275), but unfortunately our setup is not conducive to consensus mode.