-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
op-node: generalize and improve alt-sync #5179
Conversation
|
✅ Deploy Preview for opstack-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
009bf1f
to
34fec18
Compare
34fec18
to
d0047bd
Compare
Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review. |
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.
Looks like a good improvement. Just a couple of questions that I could be pretty easily persuaded to not worry about. :)
93682ce
to
cdac097
Compare
Rebased and addressed PR review. Ready for final review |
Co-authored-by: Adrian Sutton <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #5179 +/- ##
===========================================
- Coverage 40.09% 37.68% -2.42%
===========================================
Files 371 215 -156
Lines 23453 18237 -5216
Branches 832 0 -832
===========================================
- Hits 9404 6873 -2531
+ Misses 13345 10690 -2655
+ Partials 704 674 -30
Flags with carried forward coverage won't be shown. Click here to find out more.
|
232b39e
to
fbe1330
Compare
Hmm, found an issue with the sync-changes: the payload queue is removing duplicates based on block number, not block hash: so on reorgs it is more likely to drop the new canonical blocks. |
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.
Need to fix lint + tests, but after that should be good to go.
This PR has been added to the merge queue, and will be merged soon. |
This PR is next in line to be merged, and will be merged as soon as checks pass. |
1 similar comment
This PR is next in line to be merged, and will be merged as soon as checks pass. |
Description
This PR iterates on top of the RPC-sync implementation to prepare for P2P-sync functionality as second form of alt-sync method, by encapsulating the RPC sync and updating its interface to make p2p sync fit in as well. Also fixes an off-by-one bug in the range to sync.
Changes:
L2SyncEndpointConfig
. Previously it was hardcoded to true, yet also called blockhash verification. We can just allow the user to set it like they do with the L1 RPC.GetUnsafeQueueGap
start
is incl.,end
is excl., and fix off by one issue: we don't want to include the first block of the queue, but we do want to include the expected block number if queue is empty.Close
the RPC sync by reaching into attributes of the driver, but instead maintain the sync as separate lifetime thing in the node to close; there will be other sync methods.AltSync
interface: the node itself implements it, and then routes it to the active & preferred sync methods that each also implement it, to allow more to be added.SyncClient
now consumes these range requests and does the fetching, instead of getting individual block numbers. Once new range information arrives, we now also drop previously scheduled information that may be stale.Tests
L2SyncRPCConfig
toPreparedL2SyncEndpoint
to match the other config utils of L1 and L2 endpoints that insert a prepared RPC client directly.Metadata
Fix CLI-3643