Skip to content

op-node: fix withdrawals#3326

Merged
mslipper merged 3 commits intodevelopfrom
bugfix/withdrawals
Aug 26, 2022
Merged

op-node: fix withdrawals#3326
mslipper merged 3 commits intodevelopfrom
bugfix/withdrawals

Conversation

@mslipper
Copy link
Contributor

Fixes withdrawals by specifying the correct storage slot. Note that this does not fix the op-e2e tests, which were incorrectly passing. @protolambda is working on this.

Fixes withdrawals by specifying the correct storage slot. Note that this does not fix the op-e2e tests, which were incorrectly passing. @protolambda is working on this.
@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2022

⚠️ No Changeset found

Latest commit: 8251fd9

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
Copy link
Contributor

mergify bot commented Aug 26, 2022

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

Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

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

Do we need a changeset?

@mslipper
Copy link
Contributor Author

Do we need a changeset?

No - op-* are versioned independently from changesets. I'll release a new tag once this is merged.

@mslipper mslipper merged commit 0308fe4 into develop Aug 26, 2022
@mslipper mslipper deleted the bugfix/withdrawals branch August 26, 2022 23:20
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

op-e2e passing in both cases was caused by 1/16 chance, due to the common nibble between two storage slots:

# the old Go code, bad: withdrawal hash key in storage slot 1
slot: 0x0fc5f1f8a00bac48be88365a30af798f877e907941a92407d9f4b15fef77eea1
# the new Go code, good: withdrawal hash key in storage slot 0
alt slot: 0x1d69b929d80ac435a0c3f908abfc4dbd08435ee119482f8a7b85dda6bece3002

keccak256(slot): 0xa87fcbdb8ebcbf82a237325aca345384f00f3a51e410ef428d99a5cb1fea87bd
keccak256(altSlot): 0xa21ecb2855d5e9adc14b2c0adeb21b98e3a617790c460c82bd5d5665a1cf5259

A proof for the bad 0x0fc5f1f8a00bac48be88365a30af798f877e907941a92407d9f4b15fef77eea1 resulted in a proof for the good 0x1d69b929d80ac435a0c3f908abfc4dbd08435ee119482f8a7b85dda6bece3002 because the storage trie was small, and the prefix of the hashes matched.
And then other tests failed, because they didn't have the lucky prefix match to accidentally fix the bad proof generation in Go. The proof itself is correct however, so no vulns, just outdated Go util code that didn't match the new storage layout + needed more tests to catch it.

Nice team work @mslipper @smartcontracts :)

@@ -319,6 +319,5 @@ func StorageSlotOfWithdrawalHash(hash common.Hash) common.Hash {
// Where p is the 32 byte value of the storage slot and ++ is concatenation
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the above comment still correct?

maurelian pushed a commit that referenced this pull request Sep 15, 2022
Fixes withdrawals by specifying the correct storage slot. Note that this does not fix the op-e2e tests, which were incorrectly passing. @protolambda is working on this.
sam-goldman pushed a commit that referenced this pull request Sep 15, 2022
Fixes withdrawals by specifying the correct storage slot. Note that this does not fix the op-e2e tests, which were incorrectly passing. @protolambda is working on this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants