Skip to content

l2geth: verifier sync in parallel with dtl#2656

Merged
mergify[bot] merged 2 commits intoethereum-optimism:developfrom
tonykogias:sync-l2geth-in-parallel-with-dtl
Jun 15, 2022
Merged

l2geth: verifier sync in parallel with dtl#2656
mergify[bot] merged 2 commits intoethereum-optimism:developfrom
tonykogias:sync-l2geth-in-parallel-with-dtl

Conversation

@tonykogias
Copy link
Contributor

Have verifier node sync in parallel with dtl to save time.

Metadata

@changeset-bot
Copy link

changeset-bot bot commented Jun 2, 2022

🦋 Changeset detected

Latest commit: 197a5c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/l2geth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added 2-reviewers C-protocol-critical Category: Modifies protocol-critical code A-cannon Area: cannon labels Jun 2, 2022
@mergify mergify bot requested a review from smartcontracts June 2, 2022 11:13
Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have successfully synced an L1-syncing node with this change. We should try to sync an L2-syncing node as well.

@smartcontracts
Copy link
Contributor

So this definitely works for L1 sync. Maybe we can just enable this feature for L1 sync for now? L2 sync is generally much faster, so this is less necessary.

@tynes
Copy link
Contributor

tynes commented Jun 2, 2022

So this definitely works for L1 sync. Maybe we can just enable this feature for L1 sync for now? L2 sync is generally much faster, so this is less necessary.

I think this should also work when syncing from L2, we would just need to confirm that it is the case it does work by trying it out

@smartcontracts
Copy link
Contributor

smartcontracts commented Jun 9, 2022

I'd really like to get this merged, but I haven't had the opportunity to test on L2-syncing nodes. L1 syncing nodes is where I really care about this anyway because the DTL is so much slower. In the interest of speed, let's just have the IF statement be:

if !cfg.Verifier || cfg.Backend == BackendL2 {
 ...
}

@tonykogias tonykogias force-pushed the sync-l2geth-in-parallel-with-dtl branch from 6f8e722 to 8cf1016 Compare June 9, 2022 20:10
Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that this properly syncs an L1-syncing node

@smartcontracts
Copy link
Contributor

cc @tynes do you mind dropping one quick review here

@smartcontracts
Copy link
Contributor

@tonykogias just needs a quick rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 15, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Jun 15, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot merged commit ff0723a into ethereum-optimism:develop Jun 15, 2022
@mergify mergify bot removed the on-merge-train label Jun 15, 2022
@mslipper mslipper mentioned this pull request Jun 18, 2022
@smartcontracts
Copy link
Contributor

Eyyyyyy SO glad this is live now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cannon Area: cannon C-protocol-critical Category: Modifies protocol-critical code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Have L2Geth sync in parallel with the DTL

3 participants