op-chain-ops: implement withdrawal hashing by tynes · Pull Request #3469 · ethereum-optimism/optimism

tynes · 2022-09-14T16:50:54Z

Description

Implement both the new hashing scheme and the legacy hashing for withdrawals so that the withdrawals can be migrated.
This only includes tests for legacy style hashing, will follow up with an additional PR to add tests for bedrock style withdrawal hashing to prevent this PR from growing too large.

Closes ENG-2718

changeset-bot · 2022-09-14T16:50:57Z

⚠️ No Changeset found

Latest commit: 8151bb8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

mergify · 2022-09-14T16:51:13Z

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

tynes · 2022-09-20T20:27:01Z

Can get test vectors from here: https://optimistic.etherscan.io/address/0x4200000000000000000000000000000000000000#internaltx

op-chain-ops/crossdomain/withdrawal.go

trianglesphere

LGTM - one Q about some test setup. I really like how it compares the computed slot vs what get set in the trace.

op-chain-ops/crossdomain/withdrawal_test.go

mslipper · 2022-09-21T23:33:42Z

Recusing myself from review here; others will have more insightful feedback.

tynes · 2022-09-22T12:28:45Z

LGTM - one Q about some test setup. I really like how it compares the computed slot vs what get set in the trace.

Good call on catching that error, I have updated it to panic on error of abi deserialization

Fuzzes the L2ToL1MessagePasser for state changes to its storage. This specifically tests that the storage slot that the proofs are generated against is computed correctly. This is important to test as any changes that break the proof would also break this test. The fuzz runs were used to generate test cases in #3469 for off chain computation of the storage slots for withdrawals.

tynes · 2022-09-22T13:22:40Z

I've added some additional functionality to this PR:

Split the withdrawal type into LegacyWithdrawal and Withdrawal
encoding + decoding with roundtrip fuzz tests of both withdrawal types
Test coverage of the storage slot generation for the Withdrawal type
- Test vectors generated using foundry, this is important for the storage slot migration

Fuzzes the L2ToL1MessagePasser for state changes to its storage. This specifically tests that the storage slot that the proofs are generated against is computed correctly. This is important to test as any changes that break the proof would also break this test. The fuzz runs were used to generate test cases in #3469 for off chain computation of the storage slots for withdrawals.

op-chain-ops/crossdomain/message.go

trianglesphere

A couple small code things, but the rest is looking really good.

op-chain-ops/crossdomain/legacy_withdrawal.go

op-chain-ops/crossdomain/withdrawal.go

tynes · 2022-09-22T21:51:15Z

i have addressed the issues, thanks for the review

trianglesphere

There's a small bug in the length check code. Fix that and it's good to go.

op-chain-ops/crossdomain/legacy_withdrawal.go

mergify · 2022-09-23T17:27:02Z

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

tynes · 2022-09-23T18:04:00Z

Not sure why this is failing in CI but not locally for me

mslipper · 2022-09-23T20:09:27Z

It fails for me locally as well.

trianglesphere · 2022-09-23T20:31:42Z

@tynes is there an issue with the files checked in / something around the path where it's not able to load in the test cases?

Implement both the new hashing scheme and the legacy hashing for withdrawals so that the withdrawals can be migrated. Also implement encoding and decoding of the withdrawals

Now that the old message passer is being kept in the L2 state, be sure to the use legacy address. The new message passer is at a different address.

tynes · 2022-09-25T20:22:02Z

@tynes is there an issue with the files checked in / something around the path where it's not able to load in the test cases?

Found the source of the bug, it was due to the way that the L2ToL1MessagePasser is now being handled. Instead of migrating the state at address(0x42..00) we are introducing a new predeploy at address(0x42..16) that will receive the migrated hashes and the proof on L1 will work against that account.

Fuzzes the L2ToL1MessagePasser for state changes to its storage. This specifically tests that the storage slot that the proofs are generated against is computed correctly. This is important to test as any changes that break the proof would also break this test. The fuzz runs were used to generate test cases in #3469 for off chain computation of the storage slots for withdrawals.

mergify · 2022-09-25T20:34:18Z

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

mergify · 2022-09-25T20:34:25Z

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

Fuzzes the L2ToL1MessagePasser for state changes to its storage. This specifically tests that the storage slot that the proofs are generated against is computed correctly. This is important to test as any changes that break the proof would also break this test. The fuzz runs were used to generate test cases in #3469 for off chain computation of the storage slots for withdrawals.

* contracts-bedrock: fuzz L2ToL1MessagePasser Fuzzes the L2ToL1MessagePasser for state changes to its storage. This specifically tests that the storage slot that the proofs are generated against is computed correctly. This is important to test as any changes that break the proof would also break this test. The fuzz runs were used to generate test cases in #3469 for off chain computation of the storage slots for withdrawals. * contracts-bedrock: regenerate snapshot * bindings: regenerate Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Matthew Slipper <me@matthewslipper.com>

tynes force-pushed the feat/xdm-withdrawals branch from 1e0fb2d to 5b61f78 Compare September 20, 2022 12:13

tynes force-pushed the feat/xdm-withdrawals branch 4 times, most recently from e322f43 to fe114c0 Compare September 21, 2022 20:58

tynes marked this pull request as ready for review September 21, 2022 20:59

tynes requested review from mslipper, protolambda, smartcontracts and trianglesphere September 21, 2022 21:00

tynes commented Sep 21, 2022

View reviewed changes

op-chain-ops/crossdomain/withdrawal.go Outdated Show resolved Hide resolved

tynes commented Sep 21, 2022

View reviewed changes

op-chain-ops/crossdomain/withdrawal.go Outdated Show resolved Hide resolved

trianglesphere reviewed Sep 21, 2022

View reviewed changes

op-chain-ops/crossdomain/withdrawal_test.go Outdated Show resolved Hide resolved

mslipper removed their request for review September 21, 2022 23:33

tynes force-pushed the feat/xdm-withdrawals branch 2 times, most recently from f6f570c to 9729d90 Compare September 22, 2022 12:27

tynes force-pushed the feat/xdm-withdrawals branch from 9729d90 to 8d0c4b1 Compare September 22, 2022 12:30

tynes mentioned this pull request Sep 22, 2022

contracts-bedrock: fuzz L2ToL1MessagePasser #3527

Merged

trianglesphere self-requested a review September 22, 2022 14:17

smartcontracts reviewed Sep 22, 2022

View reviewed changes

op-chain-ops/crossdomain/message.go Show resolved Hide resolved

smartcontracts approved these changes Sep 22, 2022

View reviewed changes

trianglesphere reviewed Sep 22, 2022

View reviewed changes

op-chain-ops/crossdomain/legacy_withdrawal.go Show resolved Hide resolved

op-chain-ops/crossdomain/withdrawal.go Show resolved Hide resolved

trianglesphere suggested changes Sep 22, 2022

View reviewed changes

op-chain-ops/crossdomain/legacy_withdrawal.go Outdated Show resolved Hide resolved

tynes force-pushed the feat/xdm-withdrawals branch from 6641624 to dd27400 Compare September 23, 2022 14:20

trianglesphere approved these changes Sep 23, 2022

View reviewed changes

mergify bot added on-merge-train and removed on-merge-train labels Sep 23, 2022

tynes added 3 commits September 25, 2022 13:16

op-chain-ops: implement withdrawal hashing

c970480

Implement both the new hashing scheme and the legacy hashing for withdrawals so that the withdrawals can be migrated. Also implement encoding and decoding of the withdrawals

more tests

4f3476f

op-chain-ops: code review fixes

57dd3f9

tynes force-pushed the feat/xdm-withdrawals branch from a14b2fd to 57dd3f9 Compare September 25, 2022 20:16

op-chain-ops: fix message passer

8151bb8

Now that the old message passer is being kept in the L2 state, be sure to the use legacy address. The new message passer is at a different address.

mergify bot added the on-merge-train label Sep 25, 2022

mergify bot merged commit c3980e9 into develop Sep 25, 2022

mergify bot deleted the feat/xdm-withdrawals branch September 25, 2022 20:34

mergify bot removed the on-merge-train label Sep 25, 2022

This was referenced Sep 29, 2022

Develop -> Master #3602

Merged

Develop -> Master #3604

Merged

Conversation

tynes commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

mergify bot commented Sep 14, 2022

Uh oh!

tynes commented Sep 20, 2022

Uh oh!

Uh oh!

Uh oh!

trianglesphere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mslipper commented Sep 21, 2022

Uh oh!

tynes commented Sep 22, 2022

Uh oh!

tynes commented Sep 22, 2022

Uh oh!

Uh oh!

trianglesphere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tynes commented Sep 22, 2022

Uh oh!

trianglesphere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Sep 23, 2022

Uh oh!

tynes commented Sep 23, 2022

Uh oh!

mslipper commented Sep 23, 2022

Uh oh!

trianglesphere commented Sep 23, 2022

Uh oh!

tynes commented Sep 25, 2022

Uh oh!

mergify bot commented Sep 25, 2022

Uh oh!

mergify bot commented Sep 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tynes commented Sep 14, 2022 •

edited

Loading

changeset-bot bot commented Sep 14, 2022 •

edited

Loading