Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Fix testeth VMTest false account creations and deletions #4731

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Matthalp-zz
Copy link

It seems like the VMTest filler currently creates and deletes accounts in situations that deviate from the Ethereum specification:

  • The BALANCE operation should not be able to create accounts. If the account whose balance is being looked up does not exist, the operation should simply push 0 onto the stack and not modify world state.
  • The EXTCODECOPY operation has a similar issue as BALANCE in the sense that a default value should be returned when the account subject to the lookup does not exist.
  • The EXTCODESIZE operation has a similar issue as BALANCE and EXTCODECOPY in the sense that a default value should be returned when the account subject to the lookup does not exist.
  • The SELFDESTRUCT (formerly SUICIDE) operation should not be able to delete accounts. It should only mark the corresponding account for deletion (by adding it to the self-destruct set held within the transaction substate).

At the crux of the account creation issue in BALANCE, EXTCODECOPY, and EXTCODESIZE is that the std::map that holds the address-to-account mapping is not checked to see whether or not the address exists before performing the lookup. As a result, per std::map's operation[] semantics the entry is default constructed for these non-existent addresses -- adding them to the world state.

@winsvega
Copy link
Contributor

do we really need a VM test suite by now?
I suggest we convert VM tests into general state tests.
@pirapira

@Matthalp-zz
Copy link
Author

This is a good discussion point. The VMTests are out-of-date. They do not seem to support milestones beyond Frontier. I have actually have a local branch that begins to address this issue, which I can push if you are interested. Thinking out loud:

The case for VMTests: The EVM is the most complex part of the Ethereum protocol (not considering the networking portion). Its behavior is also the focus of most of the changes that occur between milestones. A set of tests isolating the EVM functionality from transaction semantics may make sense. While this would impose clients to have a separate EVM module for these tests, this is already the case in reality. Migrating these tests to the GeneralStateTests does have some additional implementation overhead to create the transaction that invokes the EVM for all of these different cases -- and there are a lot of them!

The case against VMTests: The EVM is only invoked when transactions are processed and the current VMTests do not fully exercise the VM functionality. To the latter point the various *CALL operations are only tested to the extent that they are placed in the callcreates mock. The GeneralStateTests are already needed to fully test their functionality. Similarly logs are tested via the resultant bloom in the VMTest which would be better suited here. Many of the EVM enhancements since Frontier require full transaction processing capabilities to be fulfilled (e.g. STATICCALL).

Proposal: Based on weighing the pros and cons of each situation, I agree with your line of thinking to migrate the VMTests to GeneralStateTests. The current VMTest directories would move into the GeneralStateTests directories and are ported to being spawned by a transaction. Additionally, it would be nice to enhance the GeneralStateTests to also check the EVM's output byte array and resultant transaction receipt. Checking the EVM output will be useful for the JSON-RPC calls that check utilize the output and exposing the transaction receipt helps prepare implementors for block processing.

Let me know what you think. I am more than happy to help with this porting effort if we agree it's the right thing to do.

@Matthalp-zz Matthalp-zz changed the title Fix testeth VmTest false account creations and deletions Fix testeth VMTest false account creations and deletions Jan 1, 2018
@pirapira
Copy link
Member

pirapira commented Jan 2, 2018

@winsvega I'm still using VM tests.

@Matthalp-zz
Copy link
Author

How do we want to proceed?

@winsvega
Copy link
Contributor

winsvega commented Jan 8, 2018

looks like we keep VMTests for now. so if @pirapira approves this PR should be merged.

@pirapira
Copy link
Member

pirapira commented Jan 8, 2018

I took this week off. Coming back here next week.

@pirapira
Copy link
Member

@matthalp will you modify the test cases in ethereum/tests so that the tests pass? Or shall I?

@Matthalp-zz
Copy link
Author

I am happy to give it a shot.

@pirapira
Copy link
Member

@matthalp thank you. I'm ready to answer any questions.

@chfast
Copy link
Member

chfast commented May 16, 2018

How is this going?

@pirapira
Copy link
Member

@matthalp meanwhile I removed these problematic instructions from VM tests. Do you still see the problem in ethereum/tests:develop?

@chfast chfast changed the base branch from develop to master August 2, 2018 08:38
@axic axic added the testeth label Aug 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants