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.

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

feat(op): isthmus genesis header #14066

Closed

Conversation

emhane
Copy link
Member

@emhane emhane commented Jan 29, 2025

Closes #14040

This PR is part of l2tol1 withdrawals root which is confirmed for inclusion in isthmus, ref: ethereum-optimism#19 (successfully follows op-geth sequencer, ref https://github.com/ethereum-optimism/op-reth/actions/runs/13059635977/job/36440108405)

@emhane emhane added the A-op-reth Related to Optimism and op-reth label Jan 29, 2025
@emhane emhane marked this pull request as draft January 29, 2025 12:16
@emhane
Copy link
Member Author

emhane commented Jan 29, 2025

opened right off fork branch as place holder, will fix merge conflicts later this week and mark ready for review

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@emhane emhane force-pushed the emhane/genesis-header branch from 46bc02b to ca5ddbb Compare January 30, 2025 18:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@emhane emhane marked this pull request as ready for review January 30, 2025 19:45
@emhane emhane requested a review from clabby January 30, 2025 22:07
@emhane
Copy link
Member Author

emhane commented Feb 3, 2025

needs review asap @mattsse

@emhane
Copy link
Member Author

emhane commented Feb 3, 2025

@meyer9 could use a review to speed up review process!

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is very hard to review, not obvious what this actually changes, can we limit this pr to the necessary changes

@emhane emhane requested a review from mattsse February 4, 2025 17:58
Comment on lines 242 to 243
// header is set in construction of `OpChainSpec` from `ChainSpec`
self.inner.genesis_header.get().expect("header is set")
Copy link
Member Author

Choose a reason for hiding this comment

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

the From<ChainSpec> impl ensures that this is safe here. the visibility of the inner type is restricted to private to ensure that the chainspec is constructed using the conversion from ChainSpec

Copy link
Member Author

Choose a reason for hiding this comment

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

there was no more obvious place to set the l2 withdrawals field, the semantics of the original code weren't straight forward

Copy link
Collaborator

Choose a reason for hiding this comment

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

semantics of the original code weren't

which ones?

all I'm saying is that's unclear to me where the actual change is

Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldn't require any changes to that static defs, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

it does because the genesis hash needs to be computed after the genesis header has been computed with withdrawals root from the alloc of the genesis file, so in some way the static defs would have had to change, I did it this way

@emhane emhane requested a review from mattsse February 6, 2025 13:57
// fill once lock with a value, if not yet init
_ = inner.genesis_hash.get_or_init(|| header.hash_slow());

if inner.hardforks.is_fork_active_at_timestamp(OpHardfork::Isthmus, inner.genesis.timestamp)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make ChainSpec store a SealedHeader directly here?

/// The hash of the genesis block.
///
/// This is either stored at construction time if it is known using [`once_cell_set`], or
/// computed once on the first access.
pub genesis_hash: OnceLock<B256>,
/// The header corresponding to the genesis block.
///
/// This is either stored at construction time if it is known using [`once_cell_set`], or
/// computed once on the first access.
pub genesis_header: OnceLock<Header>,

Then all of the static definitions would have direct access to the field, and we won't need to add any header construction logic like this

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, but then it occurred to me how many bugs it may cause if the inner chainspec has a second wrong header. besides, that would require encapsulating the inner chainspec anyway in order for that approach to be in any way safe, so the construction of OpChainSpec would change anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

I finally understood what this actually does.

I'd like to go with the suggested sealedheader approach for this, because this will also have some additional benefits, for example we could then reuse the entire chainspec type for a different header.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you pls elaborate why you would want to put a different header in the genesis header field of the inner chain spec?

Copy link
Collaborator

@mattsse mattsse Feb 6, 2025

Choose a reason for hiding this comment

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

mainly code reuse #13995

but this would just be a nice side-effect of using sealedheader here

Copy link
Member Author

Choose a reason for hiding this comment

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

wdym @klkvr wrt the OnceLock, do we want to remove them? issue link pls?

Copy link
Member Author

Choose a reason for hiding this comment

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

even if you put the header alongside the inner ChainSpec, instead of setting the genesis header field of inner ChainSpec correctly, the extra header needs a OneLock in order to be initiated in the genesis_header method which takes imm ref to self, like is done in ChainSpec::genesis_header. do we want that?

Copy link
Member

Choose a reason for hiding this comment

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

@emhane wdym by "extra header"? the idea is that the only header ChainSpec stores is the concrete SealedHeader in the inner chainspec which is initialized directly. That way there's no lazy header initialization happening at all

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, now I follow, opened this issue #14355

Copy link
Member Author

Choose a reason for hiding this comment

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

could also be fixed as follow up of this pr? no need to block isthmus feature for handling that debt imo

@emhane emhane requested a review from klkvr February 6, 2025 14:41
…op-reth into emhane/genesis-header
@emhane emhane enabled auto-merge February 10, 2025 10:09
@emhane
Copy link
Member Author

emhane commented Feb 18, 2025

re-opened in #14560 since op-reth fork is archived

@emhane emhane closed this Feb 18, 2025
auto-merge was automatically disabled February 18, 2025 11:29

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Isthmus genesis header
3 participants