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

Fix EVM tests #127

Merged
merged 1 commit into from
Jun 4, 2018
Merged

Fix EVM tests #127

merged 1 commit into from
Jun 4, 2018

Conversation

vyorkin
Copy link
Contributor

@vyorkin vyorkin commented Jun 1, 2018

  • Rename suicide op to selfdestruct according to EIP-6.

  • Transfer the balance of the self-destructed account to the "refund account" (its address is the top 20-bytes on VM's stack). Refund account is "shadow-created" if it doesn't exist.

  • Refactor and fix the evm_test.exs:

  • Update EVM.Address.new to use only the last @size of bytes for address construction:
    this is important because in VM operations we're getting addresses from the VM stack as integers that can be of any size.

  • Replace the doctest for SELFDESTRUCT operation in EVM.System module with a regular ExUnit test because it's getting quite complex.

  • Added :sender condition to the EVM.Debugger.Breakpoint for convenience.

  • Slightly refactor and fix the new_account fn (add empty code field) in the EVM.Interface.Mock.MockAccountInterface module.

  • Fix vm_test.exs (add empty code field).

resolves #126, resolves #109

@vyorkin vyorkin self-assigned this Jun 1, 2018
@ghost ghost added the status: in progress label Jun 1, 2018
@vyorkin vyorkin requested review from ayrat555, masonforest, germsvel and cernivec and removed request for ayrat555 and masonforest June 1, 2018 17:01
{gas, sub_state, exec_env, _} = run_test(test)
assert_state(test, exec_env.account_interface)

context = %{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this whole test needs to be refactored, just maybe not as part of this PR.

@vyorkin vyorkin added this to the Simple node (MVP) milestone Jun 1, 2018
Copy link
Member

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

Looks good. The only concern I have is that many evm tests moved to general tests group in ethereum common tests. So we are not running them all

@vyorkin
Copy link
Contributor Author

vyorkin commented Jun 1, 2018

Thanks! I'll double check if I'm running all the EVM tests from the ethereum common tests & fix that.

@vyorkin
Copy link
Contributor Author

vyorkin commented Jun 1, 2018

Ah, I see that @masonforest have created a separate issue to track this #124 so we're good.

Rename suicide op to selfdetstruct according to EIP-6.
Don't ignore empty state values in EVM tests.

Transfer the balance of the self-destructed account to the "refund account" (its address is the top 20-bytes on VM's stack).
Refund account is "shadow-created" if it doesn't exist.

Refactor and fix the `evm_test.exs`:
* Remove the `code` field from the caller's account (see http://ethereum-tests.readthedocs.io/en/latest/test_types/vm_tests.html#the-exec-section).
* Exclude caller's account from the actual state if it isn't in the "pre" or "post" addresses.

Update `EVM.Address.new` to use only the last `@size` of bytes for address construction:
this is important because in VM operations we're getting addresses from the VM stack as integers that can be of any size.

Replace the doctest for `SELFDESTRUCT` operation in `EVM.System` module with a regular ExUnit test.
Slightly refactor and fix the `new_account` fn (add empty `code` field) in `EVM.Interface.Mock.MockAccountInterface` module.
Fix `vm_test.exs` (add empty `code` field).
@vyorkin
Copy link
Contributor Author

vyorkin commented Jun 4, 2018

ok, rebased

Copy link
Contributor

@masonforest masonforest left a comment

Choose a reason for hiding this comment

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

The Yellow Paper says SELFDESTRUCT should Halt execution and register account for later deletion.

I think eventually we want SELFDESTRUCT to add the contracts address to the suicide_list and actually delete them when we're processing transactions. Does that sound right?

This PR looks good to go though. I'd rather have a passing test suite and refactor from there as needed.

@vyorkin
Copy link
Contributor Author

vyorkin commented Jun 4, 2018

@masonforest yes! its by mistake, thanks for pointing this out

@vyorkin
Copy link
Contributor Author

vyorkin commented Jun 4, 2018

Ok, I'll merge this and refactor the AccountInterface to add the account to the selfdestruct_list instead of deleting it right away as a next step

@vyorkin vyorkin merged commit 1441f0b into master Jun 4, 2018
@ghost ghost removed the status: in progress label Jun 4, 2018
@vyorkin vyorkin deleted the bugfix/evm-tests branch June 4, 2018 14:33
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.

Fix refund on selfdestruct Fix EVM tests
3 participants