Fix start height edge case #426
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cam-schultz
requested review from
iansuvak,
feuGeneA,
geoff-vball,
minghinmatthewlam,
bernard-avalabs and
michaelkaplan13
iansuvak
reviewed
Aug 13, 2024
main/main.go
Outdated
| zap.Error(err), | ||
| // Calculate the starting block height for the relayer only if we're processing historical blocks | ||
| // Otherwies, the first block we process is the next block after the current height | ||
| height := currentHeight + 1 |
There was a problem hiding this comment.
nit, free to drop: I prefer having this in an else statement below the if cfg.ProcessMissedBlocks instead of setting it here and overwriting it.
There was a problem hiding this comment.
Yeah, I agree the else statement would make it clearer
There was a problem hiding this comment.
I went with a slightly different approach so that we can emit the process-missed-blocks=false log only once per source chain. Lmk if you agree.
geoff-vball
reviewed
Aug 13, 2024
main/main.go
Outdated
| zap.Error(err), | ||
| // Calculate the starting block height for the relayer only if we're processing historical blocks | ||
| // Otherwies, the first block we process is the next block after the current height | ||
| height := currentHeight + 1 |
There was a problem hiding this comment.
Yeah, I agree the else statement would make it clearer
Co-authored-by: Ian Suvak <ian.suvak@avalabs.org> Signed-off-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com>
geoff-vball
approved these changes
Aug 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
Fixes a bug when handling the edge case in which the the DB stores a height, but the catch up mechanism is disabled. In that scenario, we were initializing the checkpoint manager in such a way that it never committed to the database.
How this works
Suppose the DB stores block 100, the current height is 200, and on restart, we set
process-missed-blocks=false. The relayer will correctly skip blocks 100-200 for processing, but will initialize the checkpoint manager with block 100. To prevent gaps, the checkpoint manager only commits blocks if they are exactly 1 greater than the max committed height. In this scenario, the next block height the checkpoint manager sees is 200, which is never committed.This fixes this by:
process-missed-blocks=falseprocess-missed-blocks=false, there's a race condition between fetching of the current height and the beginning of the Listener that could cause a gap.How this was tested
CI
How is this documented
N/A