Seed order to manage logs overflow #10485
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
core/services/ocr2/plugins/ocr2keeper/evm21/logprovider/provider.go
Outdated
Show resolved
Hide resolved
| @@ -596,3 +604,18 @@ func (r *logRecoverer) removePending(workID string) { | |||
| } | |||
| r.pending = updated | |||
| } | |||
|
|
|||
| // sortPending sorts the pending list by a random order based on the latest block. | |||
There was a problem hiding this comment.
this will be different order on nodes as latestBlock will be out of sync. We don't really need consensus in recoverables so it should be ok here, but in provider we can use latestBlock % 100 or something similar to ensure it stays same across nodes for some time
There was a problem hiding this comment.
Used latestBlock / 10, do you think we need 100? (maybe for fast chains)
core/services/ocr2/plugins/ocr2keeper/evm21/logprovider/buffer.go
Outdated
Show resolved
Hide resolved
value TBD, currently set to 50
amirylm
force-pushed
the
seed-order-to-manage-overflow
branch
from
303ff94 to
7007c6e
Compare
| sort.Slice(b.logs, func(i, j int) bool { | ||
| return shuffledLogIDs[i] < shuffledLogIDs[j] | ||
| }) |
There was a problem hiding this comment.
just want to verify this will work properly if the indices passed in comparator doesn't refer to the slice being sorted?
There was a problem hiding this comment.
Yes, the comparator accepts indices and returns a bool, making it completely decoupled from the underlying slice.
There was a problem hiding this comment.
actually digging more it's giving me some unexpected behaviour in playground. Will followup offline
| // Divided by 10 to ensure that nodes with similar block numbers won't end up with different order. | ||
| // NOTE: the lock must be held before calling this function. | ||
| func (r *logRecoverer) sortPending(latestBlock uint64) { | ||
| normalized := latestBlock / 10 |
There was a problem hiding this comment.
nit. 10 might not be enough on fast chains like arb. 100 might be better (but then its slow on other chains also ) so not sure
There was a problem hiding this comment.
We know the block time within the recoverer, we could also compute something based on the block time.
e.g. 100 / blockTimeSeconds
There was a problem hiding this comment.
that can have further issues when blockTime is not synchronized 😅
| shuffledLogIDs := make(map[string]string, len(b.logs)) | ||
| for _, log := range b.logs { | ||
| logID := log.getLogID() | ||
| shuffledLogIDs[logID] = random.ShuffleString(logID, randSeed) |
There was a problem hiding this comment.
nit. i think this will always be constant for a given log, so we can also make it a part of logID itself
|
SonarQube Quality Gate
|
Using deterministic seed ordering to drop the same logs across the network in cases of logs overflow, where we hit the limits:
buffer.enqueue()- ensures that buffer size won't exceed limits, while dropping the same logs across the networkprovider.GetLatestPayloads()- ensures that in cases of multiple results, the same set of logs are used across the networkrecoverer.GetRecoveryProposals()- same as latest payloads