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

Implement SELFBALANCE opcode from EIP-1884 #24

Merged
merged 2 commits into from
Oct 31, 2019
Merged

Implement SELFBALANCE opcode from EIP-1884 #24

merged 2 commits into from
Oct 31, 2019

Conversation

axic
Copy link
Member

@axic axic commented Apr 30, 2019

Ref https://eips.ethereum.org/EIPS/eip-1884

Need to actually add all the changes to EVMC apparently.

lib/evmone/execution.cpp Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 30, 2019

Codecov Report

Merging #24 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master    #24      +/-   ##
========================================
+ Coverage   99.88%   100%   +0.11%     
========================================
  Files           5      5              
  Lines         842    845       +3     
  Branches      109    109              
========================================
+ Hits          841    845       +4     
+ Misses          1      0       -1

@axic
Copy link
Member Author

axic commented May 8, 2019

The invalid opcode test is failing because 0x46 is not defined in evm_instruction_names.

@chfast
Copy link
Member

chfast commented May 8, 2019

The invalid opcode test is failing because 0x46 is not defined in evm_instruction_names.

Add if in the test.

@axic
Copy link
Member Author

axic commented Jul 24, 2019

Will make this one depend on ethereum/evmc#372

@axic
Copy link
Member Author

axic commented Aug 26, 2019

@chfast @gumb0 any idea why this fails with segfault?

@chfast
Copy link
Member

chfast commented Aug 26, 2019

@chfast @gumb0 any idea why this fails with segfault?

evmone fails because there is not instruction table for Berlin but EVMC_MAX_REVISION points to Berlin now.

@axic
Copy link
Member Author

axic commented Aug 27, 2019

Rebased and fixed the previous issues, however now for some reason stack underflow is reported where it should report unknown instruction.

@axic axic marked this pull request as ready for review August 27, 2019 17:16
@gumb0
Copy link
Member

gumb0 commented Aug 28, 2019

It just checks stack requirements earlier (at the beginning of the block) than instruction returns unknown instruction, and the stack is empty.
I think if you add push before, it will be fixed.

@gumb0
Copy link
Member

gumb0 commented Aug 28, 2019

Rebased.

TEST_F(evm_state, selfbalance)
{
accounts[msg.destination].set_balance(0x0504030201);
auto code = bytecode{} + push(1) + OP_SELFBALANCE + mstore(0) + ret(32 - 6, 6);
Copy link
Member Author

Choose a reason for hiding this comment

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

There's no argument to selfbalanace.

Copy link
Member

Choose a reason for hiding this comment

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

But when OP_SELFBALANCE is undefined, there is an argument missing to mstore(0).

Copy link
Member Author

Choose a reason for hiding this comment

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

But shouldn't it just abort because of invalid instruction?

Copy link
Member

Choose a reason for hiding this comment

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

Only in naive implementation of EVM. Here we check stack requirements for the whole block of instructions before executing them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I get that, but the concept is wrong then. evmone fails with a strange error code on invalid input then?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure this concept you mean, but yes, it can return other error code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This suggest to me that evmone cannot handle badly formatted EVM input, which has a stack underflow.

@chfast
Copy link
Member

chfast commented Oct 31, 2019

Please add CHANGELOG entry.

@axic
Copy link
Member Author

axic commented Oct 31, 2019

@chfast updated

@chfast chfast merged commit b15dd08 into ethereum:master Oct 31, 2019
@axic axic deleted the selfbalance branch October 31, 2019 17:05
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