optimism icon indicating copy to clipboard operation
optimism copied to clipboard

op-chain-ops: implement withdrawal hashing

Open tynes opened this issue 3 years ago • 6 comments

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

tynes avatar Sep 14 '22 16:09 tynes

⚠️ No Changeset found

Latest commit: 8151bb888f0e21f9271a52f2e2b744e0f20aaaf7

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

changeset-bot[bot] avatar Sep 14 '22 16:09 changeset-bot[bot]

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

mergify[bot] avatar Sep 14 '22 16:09 mergify[bot]

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

tynes avatar Sep 20 '22 20:09 tynes

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

mslipper avatar Sep 21 '22 23:09 mslipper

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

tynes avatar Sep 22 '22 12:09 tynes

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

tynes avatar Sep 22 '22 13:09 tynes

i have addressed the issues, thanks for the review

tynes avatar Sep 22 '22 21:09 tynes

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

mergify[bot] avatar Sep 23 '22 17:09 mergify[bot]

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

tynes avatar Sep 23 '22 18:09 tynes

It fails for me locally as well.

mslipper avatar Sep 23 '22 20:09 mslipper

@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?

trianglesphere avatar Sep 23 '22 20:09 trianglesphere

@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.

tynes avatar Sep 25 '22 20:09 tynes

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

mergify[bot] avatar Sep 25 '22 20:09 mergify[bot]

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

mergify[bot] avatar Sep 25 '22 20:09 mergify[bot]