Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

New statetest execution model #9431

Closed
holiman opened this issue Aug 29, 2018 · 4 comments
Closed

New statetest execution model #9431

holiman opened this issue Aug 29, 2018 · 4 comments
Labels
M4-core ⛓ Core client code / Rust. Z1-question 🙋‍♀️ Issue is a question. Closer should answer.
Milestone

Comments

@holiman
Copy link
Contributor

holiman commented Aug 29, 2018

A couple of months ago, the stateTest execution model was slightly changed, which I noticed when updating the tests within the geth repo.

Some gitter discussion on the topic: https://gitter.im/ethereum/tests?at=5b85004aff445156164b685f
Changes is to add a 0-value mining reward (touch). Normally, the coinbase gets txfees during a statetest -- making it exist already. So this change only makes a difference in the cases
where

  • the coinbase suicided, or
  • there are only 'bad' transactions, which aren't executed. In those cases, the coinbase gets no txfee, so isn't created, and thus needs to be touched

Here are the relevant changes to Geth that was needed in order to make it work: https://github.com/ethereum/go-ethereum/pull/17538/files/548127685648625d8acb3e6ea1b479a2f2a167a9#diff-f53696be8527ac422b8d4de7c8e945c1R149 .

It would be good to know if parity will implement this, or not.

  • If parity does not implement this, then geth shouldn't either, since the execution model of statetests is at the heart of the evmlab-based fuzzing engine, and differences here will yield tons of false positives.

CC @winsvega to answer any questions about the reason for change.

@folsen
Copy link
Contributor

folsen commented Aug 29, 2018

cc @tomusdrw @sorpaas @andresilva

@Tbaut Tbaut added Z1-question 🙋‍♀️ Issue is a question. Closer should answer. M4-core ⛓ Core client code / Rust. labels Aug 29, 2018
@Tbaut Tbaut added this to the 2.1 milestone Aug 29, 2018
@winsvega
Copy link

winsvega commented Aug 29, 2018

The reason for this change was a preparation of state tests to run via RPC.
When test is executed by using RPC commands it is basically a blockchain test. RPC methods simulate real network behaviour. So when a new block is imported it is expected to have a reward. In case of StateTests it is 0 reward.
As Yoichi said 0 reward is still considered to be a touch thats why I added this change.

Another solution would be to implement mining rewards in StateTests. Or we could just deprecate the statetests because the same information that a state test has could be retrieved from a blockchain test.
The reason not to deprecate the state test is that state test parser is implemented on clients and is used for different kind of tests.

Once again I would like to see any custom test parser to be replaced by RPC test execution methods on client side at some point. https://github.com/ethereum/retesteth/wiki/RPC-Methods
This way will be no need to care about test format changes.

@sorpaas
Copy link
Collaborator

sorpaas commented Aug 29, 2018

@winsvega: we have many practical issues to implement retesteth RPC methods. On our side, this requires a complete rework for our RPC crate to insert testing client. So this is not something we can do in short term, even if we want.

So if the reason for the change is related to RPC, then I think it doesn't really affect us? @folsen @tomusdrw @andresilva This is a rather simple change anyway, and I think either implementing this or not won't make much difference.

@holiman
Copy link
Contributor Author

holiman commented Aug 29, 2018

It's basically the same with geth, we won't implement retesteth RPC methods anytime soon. However, the change does affect the ability to actually pass the statetests, which is nice to have.

@cheme cheme self-assigned this Aug 29, 2018
@tomusdrw tomusdrw assigned tomusdrw and unassigned cheme Aug 30, 2018
@tomusdrw tomusdrw removed their assignment Aug 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
M4-core ⛓ Core client code / Rust. Z1-question 🙋‍♀️ Issue is a question. Closer should answer.
Projects
None yet
Development

No branches or pull requests

7 participants