l2geth: sync spec refactor #822
Conversation
|
There was a problem hiding this comment.
Looking good. I have a few small questions for you but not blocking. I'll approve because I'll have another chance to review the diff as a whole in #552.
| if *latest == *index { | ||
| return true, nil | ||
| } | ||
| // The indices are not equal |
There was a problem hiding this comment.
Do we ever need to handle the case where the local tip ends up being greater than the remote tip for any reason?
| sync := func(start, end uint64) error { | ||
| return s.syncTransactionRange(start, end, backend) | ||
| } | ||
| index, err := s.sync(getLatest, s.GetNextIndex, sync) |
There was a problem hiding this comment.
Stylistic question (or maybe a go thing I'm not aware of) but why use getLatest and sync here wrapped around the relevant functions? Could we instead pass backend through to s.sync? Just curious about the design decision. Particularly since you don't do the same thing within syncQueue.
There was a problem hiding this comment.
Its because the third argument to SyncService.sync accepts a function with with the following signature:
func syncer(start, end uint64) errorYou can see that syncing a transaction range requires the backend argument to be passed through. This is done by creating a closure that has the backend argument in scope. The backend argument changes the query param that is sent by the RollupClient to the DTL. It is either l1 or l2 - this allows the client to decided which transaction it wants returned, ie the transaction synced by the DTL from L1 (verifier mode) or from L2 (replica mode).
…552) * l2geth: add Backend enums and config parsing * l2geth: move OVMContext to types file * l2geth: implement syncservice spec * l2geth: fix error handling for get tx batch * l2geth: update tests to compile and pass * l2geth: add sync range functions * l2geth: add batch index indexing * l2geth: update syncservice hot path logging * l2geth: use indexed batch index * chore: add changeset * l2geth: sync spec refactor (#822) * l2geth: more in depth usage string * l2geth: add standard client getters for index * l2geth: refactor sync service into shared codepaths * l2geth: clean up tests * l2geth: better logging and error handling * test: improve test coverage around timestamps * l2geth: improve docstring * l2geth: rename variable * sync-service: better logline * l2geth: better logline * l2geth: test apply indexed transaction * l2geth: better logline * linting: fix * syncservice: fix logline * l2geth: add error and fix logline * l2geth: sync service tests * fix: get last confirmed enqueue (#846) * l2geth: fix get last confirmed enqueue * chore: add changeset * client: return error correctly * batch-submitter: updated config (#847) * batch-submitter: backwards compatible configuration * chore: add changeset * deps: update * js: move bcfg interface to core-utils * batch-submitter: parse USE_SENTRY and add to env example * chore: add changeset * batch-submitter: parse as float instead of int * batch-submitter: better error logging * l2geth: update rawdb logline Co-authored-by: Georgios Konstantopoulos <[email protected]> * l2geth: more robust testing Co-authored-by: Georgios Konstantopoulos <[email protected]> * l2geth: add sanity check for L1ToL2 timestamps * l2geth: handle error in single place * l2geth: fix test tx queue origin * l2geth: add new arg to start.sh * l2geth: return error in the SyncService.Start() * chore: go fmt Co-authored-by: Georgios Konstantopoulos <[email protected]>
Description
Updates to #552 that refactor codepaths into more reusable code paths
Now the general syncing logic all goes through the same codepaths. New methods are added to the rollup client that allow for a shared interface between each so that the syncing logic can be generalized. All syncing is based on indices of transactions or batches, so a remote tip index is fetched and then compared against the current known tip and then the transactions in that range are fetched.
Comments are added to
SyncService.applyTransactionToTipthat describe the way that it mutatesSyncServiceinternal state or incoming transactions. These side effects 1) update the L2 EVM context (blocknumber and timestamp) and 2) guarantee monotonicity by giving timestamps to transactions.A future PR is left to add concurrent querying of transactions/batches.