op-batcher: extract state pruning, block fetching and progress checking into a single pure function #13060
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
We can use the channelManager's state to infer lastStoredBlock. And lastL1Tip is actually unused.
also report fully inclusive range of blocks to load
we can use nil value to signal no state clearing should be performed
return an error instead. This error is ultimated swallowed, matching existing behaviour.
2 tasks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #13060 +/- ##
============================================
- Coverage 44.51% 0 -44.52%
============================================
Files 801 0 -801
Lines 72026 0 -72026
============================================
- Hits 32059 0 -32059
+ Misses 37359 0 -37359
+ Partials 2608 0 -2608
Flags with carried forward coverage won't be shown. Click here to find out more. |
7 tasks
sebastianst
reviewed
Dec 5, 2024
Co-authored-by: Sebastian Stammler <seb@oplabs.co>
also set HeadL1 to more realistic values (generally ahead of currentL1 due to nonzero confirmation depth)
sebastianst
reviewed
Dec 5, 2024
|
Going by the codecov report: we should add a test case where there are no blocks in the state. That should give us 100% coverage of the new pure function. |
sebastianst
reviewed
Dec 5, 2024
Co-authored-by: Sebastian Stammler <seb@oplabs.co>
geoknee
commented
Dec 5, 2024
sebastianst
approved these changes
Dec 6, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
sigma
pushed a commit
that referenced
this pull request
Dec 19, 2024
…ng into a single pure function (#13060) * remove lastStoredBlock and lastL1Tip from BatchSubmitter state We can use the channelManager's state to infer lastStoredBlock. And lastL1Tip is actually unused. * change log line wording * fix typo * remove unecessary method * WIP first pass at computeSyncActions * computeSyncAction takes a ChannelStatuser interface also report fully inclusive range of blocks to load * add happy path test case * clearState is a pointer we can use nil value to signal no state clearing should be performed * add more test cases * add another test case * computeSyncActions only takes prevCurrentL1, not prevSyncStatus * add batcher restart case * safe chain reorg case * failed to make progress case * simplify log messages, print entire struct * add godoc * wire up computeSyncActions * cache prevCurrentL1 on BatchSubmitter * document stages * fix loadBlocksIntoState range interpretation * pass syncStatus, not pointer to syncStatus and add test case for no progress * check unsafe status before trying to get more blocks * do not panic on invalid block ranges return an error instead. This error is ultimated swallowed, matching existing behaviour. * test: add assetions and mock data about blockID passed to clearState * add readme section on max channel duration * add back unit tests for pruning methods * fix pruneBlocks behaviour when blockCursor pointed at block which is now pruned * rename waitForNodeSync to sequencerOutOfSync * Introduce SeqOutOfSyncError * move SyncActions code to a separate file * ChannelStatuser -> channelStatuser * SeqOutOfSyncError -> ErrSeqOutOfSync * move ctx to first position in fn signature * do not update cached prevCurrentL1 value if there is an ErrSeqOutOfSync * Always warn log when computeSyncActions returns an error * move sync actions test to separate file * computeSyncActions returns a bool, not an error There is only ever one kind of error returned * SyncActions -> syncActions * define local variables to aid readability * organise computeSyncActions and introduce startAfresh syncAction Add comments explaining logical flow: the checks get increasingly deep and we return early where possible. * undo changes to submodule * move test utils to sync_actions_test.go file * ensure pruneChannels clears currentChannel when appropriate * fix submodule" * don't try to get number of block if none exists * improve log * Update op-batcher/batcher/driver.go Co-authored-by: Sebastian Stammler <seb@oplabs.co> * use struct for block range, not array * use startAfresh in one more place * add test case for multiple channels also set HeadL1 to more realistic values (generally ahead of currentL1 due to nonzero confirmation depth) * print value of *struct in Stringer * add test case when there are no blocks in state * Update op-batcher/batcher/sync_actions.go Co-authored-by: Sebastian Stammler <seb@oplabs.co> * tighten up log messages and test descriptions --------- Co-authored-by: Sebastian Stammler <seb@oplabs.co>
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.
This approach enables the majority of the complexity in these jobs to be encapsulated, deduplicated, and easily unit tested.
Supercedes #12981.