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

EXTCODECOPY should not create an account in testeth #4281

Open
pirapira opened this issue Jul 26, 2017 · 14 comments
Open

EXTCODECOPY should not create an account in testeth #4281

pirapira opened this issue Jul 26, 2017 · 14 comments

Comments

@pirapira
Copy link
Member

pirapira commented Jul 26, 2017

VMTests contain a field of created accounts

    "callcreates" : [
    ],

as well as the pre-state. When post field contains an account not mentioned in any of these two fields, testeth should fail because at least one of these fields is wrong.

When post field contains an account not existent in the final state calculated, testeth should fail even when it's running a VM Test. Also, testeth should not create an account for EXTCODECOPY.

This should have detected ethereum/tests#235.

@rtkaczyk
Copy link

rtkaczyk commented Aug 1, 2017

The reason for the unexpected account appearing in the post section is that EXTCODECOPY, when targeting a non-existent account, will create a new empty one. This is not normal behaviour for the main chain, even before EIP-161. It seems be an artifact of the test VM implementation.

I have confirmed that with Parity - on a custom chain without EIP-161 specified, EXTCODECOPY from a non-existent address does not create a new account. Yet Parity passes the VMTests, because its test VM also mimics this strange behaviour of EXTCODECOPY.

I have not checked whether EXTCODESIZE is prone to this problem.

As for the fix, looking at callcreates is probably insufficient because that would not catch accounts created by SELFDESTRUCT, would it?

@pirapira
Copy link
Member Author

pirapira commented Aug 2, 2017

Your both points are interesting.

  • yes, we need to fix the EXTCODECOPY behavior in the test VM.
  • right, callcreates is not sufficient. Now, I think testeth should compare the accounts in the final state against the accounts in the post field.

@pirapira pirapira changed the title testeth should fail when a VMTest's post section contains an unexpected new account EXTCODECOPY should not create an account in testeth Aug 2, 2017
@winsvega
Copy link
Contributor

winsvega commented Aug 9, 2017

In a what way testeth should compare the accounts in the final state?

@pirapira
Copy link
Member Author

pirapira commented Aug 9, 2017

Compare the state root hash before the block reward distribution and the state root hash after the GeneralStateTest.

@winsvega
Copy link
Contributor

winsvega commented Aug 9, 2017

block reward only applies in blockchain tests. not in GeneralStateTests

@pirapira
Copy link
Member Author

pirapira commented Aug 9, 2017

@winsvega I know. That's why the state root hash needs to be taken before the block reward distribution.

@winsvega
Copy link
Contributor

winsvega commented Aug 9, 2017

You mean we should check the state hash of general state tests and the hash of a corresponding blockchain test (befor the reward) to confirm that the test convertation was successfull ?

we already have expect section check. this will be like additional level of protection

@pirapira
Copy link
Member Author

pirapira commented Aug 9, 2017

Expect section check is not powerful when the expect section is empty, or the difference occurs somewhere not interesting.

@winsvega winsvega added the bug label Sep 13, 2017
@winsvega
Copy link
Contributor

winsvega commented Sep 13, 2017

@pirapira
how could this case happen in the first place?
When post field contains an account not existent in the final state calculated, testeth should fail
final state calculated is a post state

@pirapira
Copy link
Member Author

pirapira commented Sep 13, 2017

@winsvega I can removeadd accounts fromto the "post" field in the filled test JSON file.

@winsvega
Copy link
Contributor

If you do this. that it will trigger post state mismatch when running a test

@pirapira
Copy link
Member Author

@winsvega really? Are you sure post state mismatch is detected during VMTests? Why was ethereum/tests#235 not detected by testeth?

@winsvega
Copy link
Contributor

winsvega commented Sep 13, 2017

that is vm test. you know better as you work with vm tests.

in blockchain tests it is checked
https://github.com/ethereum/cpp-ethereum/blob/develop/test/tools/jsontests/BlockChainTests.cpp#L582

state tests has hashes of the final state to compare with the client's

@pirapira
Copy link
Member Author

@winsvega I edited the description.

@axic axic added the testeth label Jan 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants