t8n tool errors #773

winsvega · 2023-06-03T06:36:59Z

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..`)

The text was updated successfully, but these errors were encountered:

winsvega · 2023-06-06T15:33:24Z

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 · 2023-06-06T16:26:19Z

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 · 2023-06-07T15:21:27Z

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 · 2023-06-08T06:29:16Z

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 · 2023-06-08T07:59:47Z

{ (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 · 2023-06-08T11:33:06Z

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 · 2023-06-08T13:59:07Z

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:

Delete the storage keys at the start of the CREATE.
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 · 2023-06-09T07:28:14Z

@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.

We use the fixtures in the BlockchainTests folder to perform these tests. For example - tests/fixtures/ethereum_tests/BlockchainTests/GeneralStateTests/stTransactionTest/HighGasPrice.json
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
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 · 2023-06-09T07:34:55Z

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 · 2023-06-09T11:45:58Z

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 · 2023-06-09T15:05:11Z

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 · 2023-06-13T07:08:45Z

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 · 2023-06-14T10:36:08Z

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 · 2023-06-14T11:39:24Z

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 · 2023-06-15T15:33:27Z

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 · 2023-06-17T12:15:55Z

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.

ValueOverflow for multiple forks - I think there is some issue with the test fixture value. See here
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.
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 · 2023-06-19T14:55:35Z

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)
I expect transaction be rejected with the error message unsupported transaction type (transaction is imported as rlp)
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)

winsvega added A-tool Area: tooling C-bug Category: this is a bug, deviation, or other problem. labels Jun 3, 2023

gurukamath self-assigned this Jun 6, 2023

gurukamath mentioned this issue Jun 20, 2023

fix t8n tool errors #797

Merged

winsvega closed this as completed Oct 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t8n tool errors #773

t8n tool errors #773

winsvega commented Jun 3, 2023 •

edited

Loading

winsvega commented Jun 6, 2023 •

edited

Loading

gurukamath commented Jun 6, 2023

winsvega commented Jun 7, 2023

SamWilsn commented Jun 8, 2023

winsvega commented Jun 8, 2023 •

edited

Loading

petertdavies commented Jun 8, 2023

petertdavies commented Jun 8, 2023

gurukamath commented Jun 9, 2023 •

edited

Loading

winsvega commented Jun 9, 2023

petertdavies commented Jun 9, 2023

winsvega commented Jun 9, 2023 •

edited

Loading

gurukamath commented Jun 13, 2023 •

edited

Loading

winsvega commented Jun 14, 2023

gurukamath commented Jun 14, 2023

winsvega commented Jun 15, 2023

gurukamath commented Jun 17, 2023 •

edited

Loading

winsvega commented Jun 19, 2023 •

edited

Loading

t8n tool errors #773

t8n tool errors #773

Comments

winsvega commented Jun 3, 2023 • edited Loading

winsvega commented Jun 6, 2023 • edited Loading

gurukamath commented Jun 6, 2023

winsvega commented Jun 7, 2023

SamWilsn commented Jun 8, 2023

winsvega commented Jun 8, 2023 • edited Loading

petertdavies commented Jun 8, 2023

petertdavies commented Jun 8, 2023

gurukamath commented Jun 9, 2023 • edited Loading

winsvega commented Jun 9, 2023

petertdavies commented Jun 9, 2023

winsvega commented Jun 9, 2023 • edited Loading

gurukamath commented Jun 13, 2023 • edited Loading

winsvega commented Jun 14, 2023

gurukamath commented Jun 14, 2023

winsvega commented Jun 15, 2023

gurukamath commented Jun 17, 2023 • edited Loading

winsvega commented Jun 19, 2023 • edited Loading

winsvega commented Jun 3, 2023 •

edited

Loading

winsvega commented Jun 6, 2023 •

edited

Loading

winsvega commented Jun 8, 2023 •

edited

Loading

gurukamath commented Jun 9, 2023 •

edited

Loading

winsvega commented Jun 9, 2023 •

edited

Loading

gurukamath commented Jun 13, 2023 •

edited

Loading

gurukamath commented Jun 17, 2023 •

edited

Loading

winsvega commented Jun 19, 2023 •

edited

Loading