search

ethereum / execution-specs

Specification for the Execution Layer. Tracking network upgrades.
Creative Commons Zero v1.0 Universal
810 stars 225 forks source link

t8n tool errors #773

Closed winsvega closed 8 months ago

winsvega commented 1 year ago

Here is the results of t8n execution on the state tests (skipping ConstantinopleFix as it is not declared in tool)

Fixed

--------
*** TOTAL ERRORS DETECTED: 8 errors during all test execution!
--------
info: (GeneralStateTests/stTransactionTest/ValueOverflow, fork: Berlin, TrInfo: d: 0, g: 0, v: 0, TrData: ` 0x..`)

info: (GeneralStateTests/stExample/accessListExample, fork: Berlin, TrInfo: d: 0, g: 0, v: 0, TrData: `:label declaredKeyWrite 0x00..`) (typed transaction before allowed fork)
info: (GeneralStateTests/stExample/basefeeExample, fork: Berlin, TrInfo: d: 0, g: 0, v: 0, TrData: `:label declaredKeyWrite 0x00..`) (typed transaction before allowed fork)
info: (GeneralStateTests/stExample/eip1559, fork: Berlin, TrInfo: d: 0, g: 0, v: 0, TrData: ` 0x00..`) (typed transaction before allowed fork)
info: (GeneralStateTests/stEIP1559/typeTwoBerlin, fork: Berlin, TrInfo: d: 0, g: 0, v: 0, TrData: `:label declaredKeyWrite 0x00..`)
winsvega commented 1 year ago 
WARNING:T8N:Transaction 0 failed: 
Error: Client reject transaction expected to be valid: (0xdbb728ec3bb0b37eb49274b839b556865bf27a654f91ef7787ef05e97d1fb15c) 
0xf884800a850a000000008080b630506000600e80602860003960006000f56002553d6000556020600060003e6000516001550000fe6211223360005260206000fd00001ca011b4e962eb5259b1eeb47b3aff0ce65c026d92b840511fecfbec1997ba4abb27a03970e23b1c2140165c823234ebeac740328c396bcee36ea61429a7995d721a70
Reason: Failed transaction:  (GeneralStateTests/stCreate2/RevertInCreateInInitCreate2, fork: Berlin, TrInfo: d: 0, g: 0, v: 0, TrData: ` 0x305060..`)

https://drive.google.com/file/d/1VGKDS1WkT9wlwtHiB-EAD8yWglx1XeO6/view?usp=drive_link

gurukamath commented 1 year ago

We had concluded that these test cases are not possible since they have zero nonce and balance, empty code but non-empty storage. We followed the discussion that happened some time ago on the py-evm repo. See here

We have ignored stRevertTest/RevertInCreateInInit.json, stCreate2/RevertInCreateInInitCreate2.json and stSStoreTest/InitCollision.json for the same reason. See here.

winsvega commented 1 year ago

why? such case is possible on first rules, so it could have left over on the mainnet (even if its not the case) also how do you reject this tests? do you check the fork and then reject or you just reject according to the pre state? the t8n report transaction fail instead of state pre incorrect.

in Frontier this is totally possible

SamWilsn commented 1 year ago

it could have left over on the mainnet (even if its not the case)

We're of the opinion that if something hasn't happened on mainnet, we can create a "retroactive" EIP prohibiting the behaviour, in the same way that EIP-2681 retroactively limits account nonces.

in Frontier this is totally possible

What would this look like?

winsvega commented 1 year ago

{ (CREATE 0 0 (lll { [[1]]1 } 0)) }

If your evm fails on empty contract then ot will fail on this instruction on Frontier, so technically it will be a bug

petertdavies commented 1 year ago

If your evm fails on empty contract then ot will fail on this instruction on Frontier, so technically it will be a bug

We handle empty accounts in general fine. All of these test case relate to questions of what happens when you create a smart contract at an address which already has storage. The account must have no nonce and no code, otherwise the create would fail. This is only possible if you find an address collision (costing billions of dollars in compute) and deploy an empty contract there prior to Spurious Dragon. Whether this counts as an impossible situation is debatable.

Early in the development of the specs we followed py-evm precedent in refusing to implement the behaviour required to pass these tests and declaring the tests invalid. Once I have worked out what Geth's behaviour is we can decide if we should implement it.

petertdavies commented 1 year ago

I think I have figured it out.

According to the test suite, the semantics of CREATE and CREATE2 when the target is an account with no nonce, no code, but some storage keys is as follows:

  1. Delete the storage keys at the start of the CREATE.
  2. For gas refund calculation purposes, treat the storage keys as having never existed.

We implement 1, but not 2. Due to architectural differences between us and production clients, it's not trivial for us to implement 2.

The problem is that CREATE deletes storage while bypassing the refund counter. If it SSTORE had deleted those keys, it would have given a bunch of refunds. When the new contract writes to the deleted keys the refund (which was never given) is removed turning the refund counter negative. We then crash because the refund counter is unexpectedly negative. Production clients (probably unknowingly) implement 2 to get around this problem.

gurukamath commented 1 year ago

@winsvega I looked at the remaining 38 tests in your list and they actually pass in our CI process. Let me outline the test that we perform in our CI process and do let me know if we are missing anything here.

  1. We use the fixtures in the BlockchainTests folder to perform these tests. For example - tests/fixtures/ethereum_tests/BlockchainTests/GeneralStateTests/stTransactionTest/HighGasPrice.json
  2. We then load the env, alloc and txs from the fixture for each block (see here) and perform the state transition using the t8n tool. We then use the output from the t8n tool and use the b11r tool to build a block out of it
  3. Finally, we assert that the rlp of the block from the b11r tool exactly matches the rlp given in the fixture JSON.

Please let me know in case you see any issue with this testing approach.

winsvega commented 1 year ago

Ah this particular one must be coinbase touch. Its an issue only with state tests as it has 0 reward touch. So coinbase gets touched and an empty account created on pre EIP150.

Since I assume the state transition is equivalent to the block sealing. Even with mining reward of 0. Or 0 tx (high nonce tx gets rejected)

But let me check the logs of pyt8n

petertdavies commented 1 year ago

I will make a PR to fix the issue I mentioned above. That should fix stRevertTest/RevertInCreateInInit.json, stCreate2/RevertInCreateInInitCreate2.json and stSStoreTest/InitCollision.json.

winsvega commented 1 year ago

tests/fixtures/ethereum_tests/BlockchainTests/GeneralStateTests/stTransactionTest/HighGasPrice.json

here is the thing. in blockchain test we can't check rejected transaction exceptions. I do export it in bc tests too but runners don't verify this. so when a tx get rejected in state tests -> blockchain test convertion. we get empty block. that means such test do nothing.

I still put this info into blockchain test:

"transactionSequence" : [
                    {
                        "exception" : "TR_NoFunds",
                        "rawBytes" : "0xf87e809f031eea408f8e1799cb883da2927b1336521d73c2c14accfebb70d5c5af006c82520894d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d001801ca039b081ab7094dff1b3ac79cbf8e381adc9f7a4e16290d7abc42dd006e5e062c5a033af00e26e5eb4431dcad601b2c8bf12d51eef2bd037d14681f22692ffab1ccd",
                        "valid" : "false"
                    }
                ],

to indicate that this tx was rejected from the block. the correct test would be to forge block with this transaction accepted but then we can't compute the post state as it is undefined, let's see maybe I can craft invalid block instead

we do have transaction tests for verification, this is just an attempt to check transaction rejection and its exceptions in realistic bc scenario using state tests. currently there are only a few tests like this

gurukamath commented 1 year ago

tests/fixtures/ethereum_tests/BlockchainTests/GeneralStateTests/stTransactionTest/HighGasPrice.json

here is the thing. in blockchain test we can't check rejected transaction exceptions. I do export it in bc tests too but runners don't verify this. so when a tx get rejected in state tests -> blockchain test convertion. we get empty block. that means such test do nothing.

So u recommend using the GeneralStateTests instead of the BlockChainTests to run the evm tools tests? Cause I see that the InvalidBlockTests and ValidBlockTests don't seem to be available in GeneralStateTests

winsvega commented 1 year ago

No, I am fixing those tests now. But the situation seems to be state test only. Since we assume the state is rewarded 0 and despite transaction rejected, an empty transition is calculated.

So its a question about what should happen to empty coinbase when state reward set to 0 and 0 transactions provided in txs.(pre eip158)

And as for empty accounts, the t8n must support pre state with empty accounts having storage as on firrst networks it was possible to generate those

gurukamath commented 1 year ago

And as for empty accounts, the t8n must support pre state with empty accounts having storage as on firrst networks it was possible to generate those

This should be handled post #794

winsvega commented 1 year ago

updated the errors. some errors still happen when unexpected type transaction is imported on prior forkrules. other errors I think occured due to unstability. the test has many vectors and sometimes when running 600 transactions over 5 fork rules in a raw, the tool just does not return outputs.

gurukamath commented 1 year ago

updated the errors. some errors still happen when unexpected type transaction is imported on prior forkrules. other errors I think occured due to unstability. the test has many vectors and sometimes when running 600 transactions over 5 fork rules in a raw, the tool just does not return outputs.

I have checked the last remaining failing tests on the list and the following seems to be true.

  1. ValueOverflow for multiple forks - I think there is some issue with the test fixture value. See here
  2. accessListExample, basefeeExample, eip1559, typeTwoBerlin for Berlin - These are just tests where the transaction type is not supported and the t8n tool is expected to throw an error when attempting to sign unsupported transactions. See here. So in my opinion, this behaviour is as expected.
  3. intrinsic for Merge and precompsEIP2929 for Shanghai - These tests pass on my end. Could you add more context to exactly what is incorrect here?
winsvega commented 1 year ago
  1. yes this is experemental test providing a malicious rlp/ fields. it crashed the t8n instead of it handling malicious input. (in generated bc test I expect hive or client try to parse block rlp and handle the error. so client must not crash)
  2. I expect transaction be rejected with the error message unsupported transaction type (transaction is imported as rlp)
  3. this test just fails because pyt8n is unstable. (so it passes 99% of the time, but if you run many tests in raw some will start to fail, maybe also because run multithread)

we are down to 2 issues: unsupported transactions types being imported in txs.rlp with conflicting state.fork flag bizzare values in tx fields crash t8n (expected: transaction rejected with exception string in out.json file)