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

Isthmus: Tests and misc updates for L2 withdrawals root #399

Open
wants to merge 13 commits into
base: l2-withdrawals-root
Choose a base branch
from

Conversation

vdamle
Copy link
Contributor

@vdamle vdamle commented Oct 9, 2024

Targeted to be merged into: #383

for TODO work items in op-geth documented in: ethereum-optimism/optimism#12044

@vdamle vdamle changed the title Isthumus: Tests and miscellaneous updates for L2 withdrawals root Isthumus: Tests and misc updates for L2 withdrawals root Oct 9, 2024
@vdamle vdamle changed the title Isthumus: Tests and misc updates for L2 withdrawals root Isthmus: Tests and misc updates for L2 withdrawals root Oct 9, 2024
Vinod Damle added 3 commits October 24, 2024 21:32
* To check whether Isthmus is active within NewBlock(), to appropriately
  handle withdrawalRoot, we need to pass in chainConfig.
* Also added a Block RLP encode/decode test.
...of L2ToL1MessagePasser contract
ExecutableDataToBlockNoHash() needs to know whether Isthmus is active
to determine the correct treatment of withdrawalsRoot
Vinod Damle added 2 commits October 25, 2024 14:12
regenerating binding with go1.22 results in different unmarshalling
code that fails existing tests
@vdamle
Copy link
Contributor Author

vdamle commented Oct 25, 2024

@protolambda In order to update the genesis handling when reading from superchain-registry: https://github.com/ethereum-optimism/op-geth/blob/optimism/core/superchain.go#L30-L46 , I will need to make a change to SCR to:

  • become aware of Isthmus fork.
  • read the withdrawalsRoot from the genesis data, if Isthmus is active.

Going to work on that PR, so there's a TBD in op-geth to update that portion of the genesis after the SCR code is in.

@vdamle vdamle marked this pull request as ready for review October 25, 2024 20:59
@vdamle vdamle requested a review from a team as a code owner October 25, 2024 20:59
@vdamle vdamle requested review from ajsutton and protolambda and removed request for a team October 25, 2024 20:59
core/types/block.go Outdated Show resolved Hide resolved
core/genesis.go Show resolved Hide resolved
core/types/block_test.go Outdated Show resolved Hide resolved
Vinod Damle added 2 commits October 28, 2024 10:56
they were mainly to address vscode warnings, will move it
to a separate PR
core/genesis.go Outdated Show resolved Hide resolved
core/genesis.go Outdated Show resolved Hide resolved
core/genesis_test.go Outdated Show resolved Hide resolved
core/types/block.go Outdated Show resolved Hide resolved
core/types/gen_header_json.go Outdated Show resolved Hide resolved
also removed usage of a 3rd party package and custom json tags in header.
// If Isthmus is active at genesis, set the WithdrawalRoot to the storage root of the L2ToL1MessagePasser contract.
if g.IsIsthmus() {
if storageRootMessagePasser == (common.Hash{}) {
// if there was no MessagePasser contract storage, set the WithdrawalsHash to the empty hash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The specs note that After Isthmus activation, if there was never any withdrawal contract storage, a MPT root of an empty list can be set as the withdrawalsRoot.

@vdamle
Copy link
Contributor Author

vdamle commented Oct 31, 2024

This comment is not relevant.

Because hashAlloc() reads the storage root of the contract, no change is needed in `superchain-registry. So the above comment can be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In-review
Development

Successfully merging this pull request may close these issues.

3 participants