-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CCIP-1691 Using more accurate and detailed query for fetching messages and execution state changes #1135
Conversation
5180f3d
to
3d67cd7
Compare
8aebd2b
to
63b0f30
Compare
34db4a2
to
d4c146f
Compare
24345c3
to
44308f4
Compare
44308f4
to
817544d
Compare
817544d
to
b97523a
Compare
b97523a
to
f3d98eb
Compare
5d2116d
to
ea48de0
Compare
ea48de0
to
0f83c77
Compare
8e447d4
to
c0b8e62
Compare
c0b8e62
to
386ac69
Compare
)) | ||
} | ||
|
||
// TODO Move to chainlink-common, querying layer should cover cases like these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this feature be added to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can add it there ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO There are some cases that could be simplified, e.g. getting logs for continuous range could be replaced with a single condition | ||
// GetExecutionStateChangesForSeqNums([1:10], [11:20], [21:30]) -> GetExecutionStateChangesForSeqNums([1:30]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a similar function that might be helpful here: https://github.com/smartcontractkit/chainlink-ccip/blob/d88fcd5b95bf952c57eb33d41f6aab7afb850331/execute/plugin_functions.go#L83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I guess I need to copy paste that code then :D
24890c4
to
5960f0b
Compare
Quality Gate failedFailed conditions |
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
This PR was closed because it has been stalled for 10 days with no activity. |
Motivation
Every time root is batched and scheduled for execution we snooze that for some time (usually 3-5 minutes). After that time, the root returns to the queue and we verify if it’s executed. Root is considered fully executed when all messages are executed and related ExecutionStateChanges events are finalized. For instance, if the time to finality is 30 minutes and the root snooze time is set to 3 minutes, the root will be checked 10 times before marking it as fully executed.
There is one caveat here, we fetch messages from OnRamp using range, we pick the oldest and youngest roots and fetch all messages where
seq >= olderRoot.MinSeqNr
andseq <= youngestRoot.MaxSeqNr
Suppose you have the following scenario
Instead of fetching messages 1-100 and 401 - 500, the current implementation will fetch 1 → 500. The slower the finality the bigger the problem with current implementation is.
Solution
Switch to the query that supports fetching only the sequence numbers we need. This can be achieved with
FilteredLogs
and querying by ranges(report1.min <= seqNr <= report2.max) OR ... OR (reportN.min <= seqNr <= reportN.max)
Offramp: Go bench results
Scenario 1: Using previous LogPoller query for fetching based on min/max
Scenario 2: Using query built with FilteredLogs but using single range
Scenario 3: Using new query but with more sophisticated case and multiple ranges passed (on average ~10ms to the query time)
Offramp: Filtering larger ranges with proper smaller ranges with query analyzer
The old query needs to scan the entire range from
report[0].Min
tillreport[-1].Max
, whereas the new query selects proper smaller ranges which leads to fetching deterministic and smaller datasets from the database. Tests run on the prod-testnet environment.PR in the chainlink-common enabling new function in readers smartcontractkit/chainlink-common#633