Skip to content
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.

Sign up for GitHub

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

Jump to bottom

op-node: Add option to only use finalized blocks as l1origin #209

Merged
merged 1 commit into from
Sep 23, 2024
Merged

op-node: Add option to only use finalized blocks as l1origin #209

merged 1 commit into from
Sep 23, 2024

Conversation

palango
Copy link

@palango palango commented Aug 1, 2024 •
edited
Loading

Resolves #200

Adds the --sequencer.use-finalized option to op-node, no only use finalized blocks as l1 origin in the sequencer.

@palango palango mentioned this pull request Aug 1, 2024
Closed
@palango palango requested a review from mcortesi August 1, 2024 11:25
mcortesi
mcortesi previously requested changes Aug 6, 2024
View reviewed changes
op-node/rollup/driver/config.go Show resolved Hide resolved
op-node/rollup/driver/finalized.go Outdated Show resolved Hide resolved
op-node/rollup/driver/finalized.go Outdated Show resolved Hide resolved
mcortesi
mcortesi reviewed Aug 7, 2024
View reviewed changes
Copy link

@mcortesi mcortesi left a comment

Choose a reason for hiding this comment

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

looks much better :)

op-node/rollup/driver/finalized.go Outdated Show resolved Hide resolved
@palango palango requested a review from mcortesi August 14, 2024 09:19
@palango palango mentioned this pull request Aug 20, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 7, 2024

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Sep 7, 2024
@github-actions github-actions bot closed this Sep 12, 2024
@palango palango reopened this Sep 13, 2024
@palango palango removed the Stale label Sep 13, 2024
@palango palango changed the base branch from celo7 to karlb/celo8-1 September 16, 2024 12:39
@palango palango force-pushed the palango/finalized branch 4 times, most recently from 977cd0f to e8aef1f Compare September 17, 2024 12:08
@palango palango changed the base branch from karlb/celo8-1 to celo8 September 17, 2024 12:08
gastonponti
gastonponti approved these changes Sep 17, 2024
View reviewed changes
Copy link

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

We should check the default value. Other than that, LGTM

op-node/flags/flags.go Outdated Show resolved Hide resolved
mcortesi
mcortesi reviewed Sep 18, 2024
View reviewed changes
op-node/rollup/driver/driver.go Outdated
verifConfDepth := confdepth.NewConfDepth(driverCfg.VerifierConfDepth, statusTracker.L1Head, l1)

var verifL1 derive.L1Fetcher
if driverCfg.UseFinalized {
Copy link

Choose a reason for hiding this comment

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

i believe we are mixing 2 differents things and that confuses me.

  1. Make sure when creating a new block, you don't depend on L1 blocks that are not final (l1 origin). This is an alternative to SequencerConfDepth

  2. Make sure that when deriving the 'safe' chain we don't derive information from L1 blocks that are not final. But, OP has already a chain pointer for that, right? There's unsafe (not on l1), safe (on eth, but not final), finalize (on eth and final there)

So i don't think we should be mixing those. If we do, the "safe" chain will be 15 (+25 from batcher) minutes delayed from the unsafe chain.

This comment was marked as outdated.

Sign in to view

Copy link
Author

Choose a reason for hiding this comment

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

I discussed this with Karl a bit more and we agree with Mariano here.
This should only change the sequencer's l1 origin selection. The derivation should use the existing mechanism, l2 blocks get marked as "safe" or "final" when their origin changes its state. My initial idea to avoid reorgs there too will break expectations of users, who couldn't select to see and work on "unsafe" blocks if they trust the sequencer.

karlb
karlb approved these changes Sep 23, 2024
View reviewed changes
Copy link

@karlb karlb left a comment

Choose a reason for hiding this comment

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

The latest version looks like a nice small change.

op-node/rollup/finalized/finalized_test.go Outdated Show resolved Hide resolved
@palango palango dismissed mcortesi’s stale review September 23, 2024 11:45

comments have been addressed.

@palango palango merged commit ecc4700 into celo8 Sep 23, 2024
54 checks passed
@palango palango deleted the palango/finalized branch September 23, 2024 13:14
palango added a commit that referenced this pull request Sep 24, 2024
@palango
op-node: Add option to only use finalized blocks as l1origin in seque…
a876237 
…ncer (#209)
palango added a commit that referenced this pull request Sep 24, 2024
@palango
op-node: Add option to only use finalized blocks as l1origin in seque…
49b24d1 
…ncer (#209)
palango added a commit that referenced this pull request Sep 24, 2024
@palango
op-node: Add option to only use finalized blocks as l1origin in seque…
c9987bd 
…ncer (#209)
karlb pushed a commit that referenced this pull request Oct 12, 2024
@palango @karlb
op-node: Add option to only use finalized blocks as l1origin in seque…
a5cd883 
…ncer (#209)
karlb pushed a commit that referenced this pull request Oct 14, 2024
@palango @karlb
op-node: Add option to only use finalized blocks as l1origin in seque…
6417617 
…ncer (#209)
karlb pushed a commit that referenced this pull request Oct 14, 2024
@palango @karlb
op-node: Add option to only use finalized blocks as l1origin in seque…
b7ba45c 
…ncer (#209)
karlb pushed a commit that referenced this pull request Oct 14, 2024
@palango @karlb
op-node: Add option to only use finalized blocks as l1origin in seque…
215b209 
…ncer (#209)
alecps pushed a commit that referenced this pull request Oct 15, 2024
@palango @alecps
op-node: Add option to only use finalized blocks as l1origin in seque…
ce4743e 
…ncer (#209)
alecps pushed a commit that referenced this pull request Oct 16, 2024
@palango @alecps
op-node: Add option to only use finalized blocks as l1origin in seque…
ec1f607 
…ncer (#209)
karlb pushed a commit that referenced this pull request Oct 16, 2024
@palango @karlb
op-node: Add option to only use finalized blocks as l1origin in seque…
6206728 
…ncer (#209)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karlb karlb karlb approved these changes

@mcortesi mcortesi Awaiting requested review from mcortesi

@gastonponti gastonponti Awaiting requested review from gastonponti

@carterqw2 carterqw2 Awaiting requested review from carterqw2

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement L1Fetcher that works on block finalization state
5 participants
@palango @karlb @mcortesi @gastonponti @carterqw2