fix(verifier): add adaptive log fetching and return early on no progress - #1370
fix(verifier): add adaptive log fetching and return early on no progress#1370nvsriram wants to merge 3 commits into
Conversation
| endArg, | ||
| ) | ||
| if err != nil { | ||
| if ctx.Err() != nil || querySize == 1 { |
There was a problem hiding this comment.
I am wondering if we should classify this error instead of triggering the adaptive behaviour on every error? WDYT?
There was a problem hiding this comment.
The only issue with classifying errors is that I don't believe there is a universal error type across all RPCs. Ideally, this should only run for the Exceeded max range limit for eth_getLogs: 1000 errors but that type of string parsing/detection seems finicky.
There was a problem hiding this comment.
Let's spend some time looking into classifying this, as simply doing it on every error might overload the RPC which would ensure there's going to keep being errors.
I feel like I prefer doing this only on range errors, potentially missing some others, over broadly spamming the RPC when any error occurs.
Should we stop before the query size is 1? Right now we'd be doing 10 RPC calls to go from 1k to 1, which feels excessive. Maybe if we scoped the error to exactly Exceeded max range limit it could make sense to go deeper but not for generic errors.
There was a problem hiding this comment.
Done — added error parsing logic to cover common range limit error patterns. We only go down to query size 1 if it is a range limit. Otherwise we early return.
| ) | ||
|
|
||
| querySize = nextQuerySize | ||
| adaptive = true |
There was a problem hiding this comment.
For chains that truly have a limit of logs this behaviour will trigger each time we call this with a range exceeding the limit. Maybe we could persist the querySize for the lifecycle of the sourceReader?
There was a problem hiding this comment.
If we combine this with the error classification above this could lead to shrinking window on transient error might not be ideal but depending on the frequency we hit the max log issue might be worth doing
There was a problem hiding this comment.
I agree, I initially had it persist but changed it since the error classification part was missing.
There was a problem hiding this comment.
With error classification added now, the querySize is persisted using maxBlockRange.
|
Code coverage report:
|
| "range too large", | ||
| "too many blocks", | ||
| "maximum block range", | ||
| "eth_getlogs is limited to", |
There was a problem hiding this comment.
why do we have EVM specific logic here?
There was a problem hiding this comment.
Agree, this should ideally be in the EVM source reader impl
makramkd
left a comment
makramkd
left a comment
There was a problem hiding this comment.
I think the logic makes sense, its just in the wrong place at the moment.
| "range too large", | ||
| "too many blocks", | ||
| "maximum block range", | ||
| "eth_getlogs is limited to", |
There was a problem hiding this comment.
Agree, this should ideally be in the EVM source reader impl
| adaptive := false | ||
| var allEvents []protocol.MessageSentEvent | ||
|
|
||
| for from <= to { |
There was a problem hiding this comment.
Can this kind of batching/adaptive logic be in the EVM source reader instead?
5 participants
Description
Currently, when
loadEventsreturns an error we simply log it and move on, even though there may be no events. For cases where the block range to large, this function would result in a death loop i.e., we fail to fetch logs for 1,000 blocks, we error, return, finalized block never updates, and we’re basically stuck.This PR adds:
fetchRangeAdaptiveto adaptively half the range and retry on failureprocessEventCycleif no progress is madeTesting
TestSRS_FetchRangeAdaptive_ShrinksAndCompletesRangeto ensure adaptive range logic works as intendedlatestvalues in test to ensure chunks are non-bisectable and tests are intact