Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EthereumRuntime: Use EXTCODECOPY to retrieve bytecode #51

Merged
merged 8 commits into from
Jan 19, 2019
Merged

EthereumRuntime: Use EXTCODECOPY to retrieve bytecode #51

merged 8 commits into from
Jan 19, 2019

Conversation

pinkiebell
Copy link
Contributor

Introduces the EVMCode helper library

Related #18

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #51 into master will increase coverage by 0.87%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #51      +/-   ##
=========================================
+ Coverage   64.62%   65.5%   +0.87%     
=========================================
  Files           9       9              
  Lines         212     200      -12     
  Branches       58      55       -3     
=========================================
- Hits          137     131       -6     
+ Misses         41      37       -4     
+ Partials       34      32       -2
Impacted Files Coverage Δ
contracts/Verifier.sol 46.34% <50%> (-0.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af7faa8...bbebe41. Read the comment docs.

Copy link
Member

@peara peara left a comment

Choose a reason for hiding this comment

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

The code is great.
I just have a few small questions/discussion

contracts/EVMCode.slb Show resolved Hide resolved
contracts/EVMCode.slb Show resolved Hide resolved

if ((dispute.state & END_OF_EXECUTION) != 0) {
if (uint8(code[_executionState.pc]) != OP_REVERT && uint8(code[_executionState.pc]) != OP_RETURN) {
if (opcode != OP_REVERT && opcode != OP_RETURN) {
Copy link
Member

Choose a reason for hiding this comment

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

This question is unrelated to the PR.
Now I think more about this, I wonder if the operator in plasma chain would submit reverted transactions to main net?
Is this will open a kind of DOS attack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be challenged. This just makes sure that a challenger can proof that the solvers solution was too 'long'

Copy link
Member

Choose a reason for hiding this comment

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

I see, this is just for the case solver didn't stop after hitting REVERT. My question was totally unrelated.
But given that, then stateHash is already enough to cover this case, isn't it?
Because if opcode is REVERT, EVMRuntime will return some error?

Copy link
Member

Choose a reason for hiding this comment

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

Also there is opcode invalid. Should it be included here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, stateHash is enough because they have to proof the execution and also agree on prioer execution steps.

Also there is opcode invalid. Should it be included here?

Hmm, no. Every valid execution ends with either RETURN or REVERT.
Or most of the time: Out Of Gas xD

Copy link
Member

@peara peara Jan 17, 2019

Choose a reason for hiding this comment

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

I think so too. When something invalid happen it will REVERT.
But here: https://solidity.readthedocs.io/en/v0.5.2/assembly.html#opcodes
there is an invalid which also said to end execution with invalid instruction
Do I miss anything...

Copy link
Member

Choose a reason for hiding this comment

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

I think assert will end with invalid but haven't found something conclusive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peara
If we want to allow this, we might also include SELFDESTRUCT. But that wasn't at the table.
I think we don't want to allow that for PLASMA @johannbarbie ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peara But still pointer, STOP is missing 😎

@pinkiebell
Copy link
Contributor Author

Yay or Nay ? 🙂

@peara
Copy link
Member

peara commented Jan 19, 2019

Yay 😄

@pinkiebell pinkiebell merged commit fa4eecb into leapdao:master Jan 19, 2019
@pinkiebell pinkiebell deleted the plg branch January 19, 2019 13:49
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