Skip to content

Calling a contract which has an undefined opcode as first byte of code#836

Merged
winsvega merged 3 commits intoethereum:developfrom
ewasm:undefined-first-byte
May 10, 2021
Merged

Calling a contract which has an undefined opcode as first byte of code#836
winsvega merged 3 commits intoethereum:developfrom
ewasm:undefined-first-byte

Conversation

@gumb0
Copy link
Copy Markdown
Member

@gumb0 gumb0 commented May 6, 2021

Pulled out of #835

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented May 6, 2021

I realize now that it should be changed for London, because 0x48 is going to be defined as BASEFEE.

@winsvega Can we merge this first as is? Do you already merge changes for London?

@gumb0 gumb0 requested a review from winsvega May 6, 2021 17:06
@winsvega
Copy link
Copy Markdown

winsvega commented May 6, 2021

Not yet. Eip1559 is in development

@gumb0 gumb0 force-pushed the undefined-first-byte branch from ff32460 to 209774a Compare May 6, 2021 17:27
@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented May 6, 2021

Not yet. Eip1559 is in development

Then I guess this should be fine to merge now. I've added blockchain tests.

Please double check the list of undefined opcodes.

@axic
Copy link
Copy Markdown
Member

axic commented May 7, 2021

Please double check the list of undefined opcodes.

I looked at undefinedOpcodeFirstByteFiller.yml and compared against the opcode table in evmeone and it looked correct to me.

@axic
Copy link
Copy Markdown
Member

axic commented May 7, 2021

@winsvega @qbzzt could this be merged?

@axic axic requested a review from qbzzt May 7, 2021 12:56
Copy link
Copy Markdown

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

It is good to have a separate test, but now we have so much data copied for every transaction.
Each blockchain test has a copy of the whole state, but executes just one contract from that state.

Perhaps its better to design the call loop in one transaction otherwise we have 100k+ of test lines?

also should have all possible opcodes as the first byte, so we have a map of which opcodes are supposed to halt the execution, and make sure that others are fine.
then we change this map over forks.

@gumb0 gumb0 force-pushed the undefined-first-byte branch from 4ba9b33 to d3ea193 Compare May 7, 2021 14:01
@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented May 7, 2021

In the second commit I changed it to be the single transaction calling all contracts in a loop and saving those that succeed (expected none to succeed)

In the third commit I changed it to test all possible bytes as a first byte, but then expected section contains the list of those that don't require values on stack and execution is successful.

Perhaps final version is better, but then it's better to rename the test (it's not about undefined opcodes anymore)

@winsvega any thoughts?

@winsvega
Copy link
Copy Markdown

winsvega commented May 7, 2021

yes this looks much better about space.
to stay safe you can add one more sstore after the loop so to be sure that test didn't run oog while going through the loop and all opcodes are called

@gumb0 gumb0 force-pushed the undefined-first-byte branch from 5cb3a48 to 46c0669 Compare May 7, 2021 15:50
Copy link
Copy Markdown

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

Ori? @qbzzt

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.

3 participants