Skip to content

Check for correct rollback of init code with side effects#844

Merged
winsvega merged 1 commit intoethereum:developfrom
ewasm:initcode-side-effects
May 17, 2021
Merged

Check for correct rollback of init code with side effects#844
winsvega merged 1 commit intoethereum:developfrom
ewasm:initcode-side-effects

Conversation

@gumb0
Copy link
Copy Markdown
Member

@gumb0 gumb0 commented May 14, 2021

This adds a test that checks that exceptional abort during init code correctly rolls back side effects.

The transaction CALLs a contract that SSTOREs 0 -> 1, then transaction ends with INVALID. Storage change in a contract is expected to be rolled back.

(In #835 I'm adding a similar test that rolls back after exceptional abort due to trying to deploy 0xef byte in London.)

@gumb0 gumb0 requested review from qbzzt and winsvega May 14, 2021 15:35
":raw 0x32ff",
":label invalid_first_byte_ef :raw 0xef"
":label invalid_first_byte_ef :raw 0xef",
":label side_effects_invalid_opcode :yul { pop(call(50000, 0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b, 0, 0, 0, 0, 0)) invalid() }"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

invalid is like revert?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

here is tricky to know that the call worked.
if its not requred to call revert directly in transaction init data field.
the way to do it is to go subcall.

transaction init -> call -> do smth. revert. | continue execution. sstore smth.

this way revert should cause only a subcall going oog and we can confirtm that execution happend by putting a sstore at the end of transaction init code.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

or you can have a control transaction with exactly the same data, but this time without invalid() instruction at the end.
so we check that the call (50000, ... worked as at some point 50000 might be not enough and it will just go OOG not testing what we wanted

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't want to test here reverts inside subcalls, becasue this file is all about init code in the creation transaction.
(Although other similar tests to check reverts inside CREATEs might be useful.)

So I prefer to add another similar transaction with the same side effect, but without revert.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added.

@gumb0 gumb0 force-pushed the initcode-side-effects branch from 69d0a7d to eac7125 Compare May 17, 2021 10:20
@gumb0 gumb0 requested a review from winsvega May 17, 2021 10:22
@winsvega winsvega merged commit 5490db3 into ethereum:develop May 17, 2021
@gumb0 gumb0 deleted the initcode-side-effects branch May 17, 2021 15:37
":raw 0x32ff",
":label invalid_first_byte_ef :raw 0xef"
":label invalid_first_byte_ef :raw 0xef",
":label side_effects :yul { pop(call(50000, 0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b, 0, 0, 0, 0, 0)) }"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was missing a comma in the end of the line, how did it work and didn't complain? @winsvega

Did it consider this a concatenation of two strings?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes apparently I can parse without ,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this PR however fails on master geth. which version did you fill it with?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this PR however fails on master geth. which version did you fill it with?

It was this commit according to generated test
ethereum/go-ethereum@addd882

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The current develop version of this test passes for me with latest geth ethereum/go-ethereum@017cf71

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.

2 participants