Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP2930 tests #774

Merged
merged 2 commits into from
Feb 26, 2021
Merged

EIP2930 tests #774

merged 2 commits into from
Feb 26, 2021

Conversation

qbzzt
Copy link
Collaborator

@qbzzt qbzzt commented Jan 29, 2021

@jochem-brouwer
Copy link
Member

I have not checked the actual files, but based upon the list of items on your first post, I'd say that we should also verify that the access list is cleared after every transaction.

So if transaction 0 has on the access list address A and storage slot B, when we access this slot, we get a discount. If transaction 1 in the same block, which has an empty access list, also accesses address A and storage slot B, then this transaction should not get a discount and instead charge the default gas cost.

The DELEGATECALL one is an interesting one 😄

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 12, 2021

I have not checked the actual files, but based upon the list of items on your first post, I'd say that we should also verify that the access list is cleared after every transaction.

So if transaction 0 has on the access list address A and storage slot B, when we access this slot, we get a discount. If transaction 1 in the same block, which has an empty access list, also accesses address A and storage slot B, then this transaction should not get a discount and instead charge the default gas cost.

I should have updated the list - I now have one on a spreadsheet. I'll do that soon, thank you for reminding me.

I agree that we need to also run checks at the block level. We do, see https://github.com/qbzzt/tests/blob/eip2930/src/BlockchainTestsFiller/ValidBlocks/bcValidBlockTest/eip2930Filler.yml.

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 12, 2021

The DELEGATECALL one is an interesting one 😄

There are now eighteen tests of using the access list in multiple contexts: https://github.com/qbzzt/tests/blob/eip2930/src/GeneralStateTestsFiller/stEIP2930/variedContextFiller.yml

@holiman
Copy link
Contributor

holiman commented Feb 19, 2021

I'm also getting

=== RUN   TestBlockchain/GeneralStateTests/stEIP2930/addressOpcodes.json/addressOpcodes_d0g0v0_Berlin
    block_test.go:49: test without snapshotter failed: block #1 insertion into chain failed: invalid receipt root hash (remote: 11effa66837b7a5862d19efbad498ebe70fdc2bc0c6a34e5a6d778d9418a0b96 local: 456363f366e20ef1d551bee5fd49391178243b0b6a38a99b0e9342d85b699ac9)

I think the blockchain tests may need to be regenerated, there was a bug in the rlp decoding of receipts which caused certain types of failures, when importing blocks from the network. This may have caused the reciept roots to become bad somwhere in the retesteth <-> geth generation flow

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 19, 2021 via email

@holiman
Copy link
Contributor

holiman commented Feb 19, 2021

Thanks -- but it wasn't my intention to dump this on you on a Friday afternoon and make you work over the weekend, so take the time you need :)

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 19, 2021 via email

@holiman
Copy link
Contributor

holiman commented Feb 21, 2021 via email

@winsvega
Copy link
Collaborator

winsvega commented Feb 21, 2021

Empty array [] is for empty list.
Empty object {} is for null

#772

@holiman
Copy link
Contributor

holiman commented Feb 21, 2021 via email

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 22, 2021

I'm also getting

=== RUN   TestBlockchain/GeneralStateTests/stEIP2930/addressOpcodes.json/addressOpcodes_d0g0v0_Berlin
    block_test.go:49: test without snapshotter failed: block #1 insertion into chain failed: invalid receipt root hash (remote: 11effa66837b7a5862d19efbad498ebe70fdc2bc0c6a34e5a6d778d9418a0b96 local: 456363f366e20ef1d551bee5fd49391178243b0b6a38a99b0e9342d85b699ac9)

I think the blockchain tests may need to be regenerated, there was a bug in the rlp decoding of receipts which caused certain types of failures, when importing blocks from the network. This may have caused the reciept roots to become bad somwhere in the retesteth <-> geth generation flow

I regenerated the tests. Can you verify they work now?

@qbzzt qbzzt closed this Feb 22, 2021
@qbzzt qbzzt reopened this Feb 22, 2021
@rakita
Copy link

rakita commented Feb 22, 2021

Hello, I had run these tests in OpenEthereum and they all fail with receiptRoot mismatch. @holiman are you getting the same?
Here is the output dump that I am getting, if needed you can find receiptRoot:
https://gist.github.com/rakita/f4496693c92275d382c9b9c13a07a31f

@holiman
Copy link
Contributor

holiman commented Feb 22, 2021

Hello, I had run these tests in OpenEthereum and they all fail with receiptRoot mismatch. @holiman are you getting the same?
Here is the output dump that I am getting, if needed you can find receiptRoot:
https://gist.github.com/rakita/f4496693c92275d382c9b9c13a07a31f

Haven't gotten to the block tests yet, but as far as the berlin statetests goes, it seems alright so far

@holiman
Copy link
Contributor

holiman commented Feb 22, 2021

Both geth and OE is ok with transactionCosts_d0g0v0_Berlin but rejects transactionCosts_d1g0v0_Berlin.

Your

Bad block detected: Error(Block(InvalidReceiptsRoot(Mismatch { expected: 0xa016a3e03d190d68ed522721348ab73e351de580cfe6cc0599c1ef3ce4e970ee, found: 0x485bb85f3a7e7fcf258228d475d08c3e835dc101052212a25a83afd061b2fd6e })), State { next_error: None, backtrace: InternalBacktrace { backtrace: None } })

Geth:

=== RUN   TestBlockchain/GeneralStateTests/stEIP2930/transactionCosts.json/transactionCosts_d1g0v0_Berlin
    block_test.go:54: test without snapshotter failed: block #1 insertion into chain failed: invalid receipt root hash (remote: a016a3e03d190d68ed522721348ab73e351de580cfe6cc0599c1ef3ce4e970ee local: 485bb85f3a7e7fcf258228d475d08c3e835dc101052212a25a83afd061b2fd6e)

So both geth and OE thinks the root should be 0x485bb85f3a7e7fcf258228d475d08c3e835dc101052212a25a83afd061b2fd6e

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 22, 2021 via email

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 22, 2021

== RUN TestBlockchain/GeneralStateTests/stEIP2930/transactionCosts.json/transactionCosts_d1g0v0_Berlin
block_test.go:54: test without snapshotter failed: block #1 insertion into chain failed: invalid receipt root hash (remote: a016a3e03d190d68ed522721348ab73e351de580cfe6cc0599c1ef3ce4e970ee local: 485bb85f3a7e7fcf258228d475d08c3e835dc101052212a25a83afd061b2fd6e)

Which branch of geth did you use? There was an issue with receipt hashes.

@holiman
Copy link
Contributor

holiman commented Feb 23, 2021

I'm running with the very latest. This is definitely an issue with how retesteth creates the receipt root hash. I can provide more info, but I'm not sure what info is most useful to you. I could provide all the key-value pairs going into the receipt trie if you want?

@holiman
Copy link
Contributor

holiman commented Feb 23, 2021

Actually, the problem may be in the evm t8n tool, which spits out an erroneous receiptHash, and retesteth probably just uses that. I'll push a fix

@holiman
Copy link
Contributor

holiman commented Feb 23, 2021

The t8n tool problem should be fixed by this commit: ethereum/go-ethereum@6304c7b . Please check if that solves the problem

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 23, 2021 via email

@holiman
Copy link
Contributor

holiman commented Feb 23, 2021

you should just be able to git pull after you have checked out the branch from @lightclient .

git remote add lightclient [email protected]:lightclient/go-ethereum.git
git fetch lightclient impl-eip-2718
git checkout impl-eip-2718

@holiman
Copy link
Contributor

holiman commented Feb 24, 2021

@qbzzt it appears that retesteth doesn't like the receipt encoding, particularly:

Error: Unexpected field 'type' in config: ToolResponseReceipt 
{
    "type" : 1,
    "root" : "0x",
    "status" : "0x1",
    "cumulativeGasUsed" : "0x520c",
    "logsBloom" : "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
    "logs" : null,
    "transactionHash" : "0x4116b4b6e8fee930d6f49d728dd76ee879063848ef8e50c9568d61a687323643",
    "contractAddress" : "0x0000000000000000000000000000000000000000",
    "gasUsed" : "0x520c",
    "blockHash" : "0x0000000000000000000000000000000000000000000000000000000000000000",
    "transactionIndex" : "0x0"

It rejects the type. It looks a bit odd to me aswell, if we have "0x1" on transactions, but have numeric 1 on receipts. cc @fjl ?

@holiman
Copy link
Contributor

holiman commented Feb 24, 2021

I pushed a commit to the geth-PR which now makes the receipt type a 0x-prefixed string

 ./evm t8n --input.alloc=./testdata/8/alloc.json --input.txs=./testdata/8/txs.json --input.env=./testdata/8/env.json --output.result=stdout --state.fork=YOLOv3
INFO [02-24|13:40:43.743] Wrote file                               file=alloc.json
{
 "result": {
  "stateRoot": "0x45ca8ccf93a870405c821cae61cd21f96e2df2755153267e7e2cc912133404a5",
  "txRoot": "0xe42c488908c04b9f7d4d39614ed4093a33ff16353299672e1770b786c28a5e6f",
  "receiptRoot": "0xb207f384195fb6fb7ee7105ba963cc19e1614ce0e75809999289c6c82e7a8d97",
  "logsHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
  "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
  "receipts": [
   {
    "type": "0x1",
    "root": "0x",
    "status": "0x1",
    "cumulativeGasUsed": "0x7aae",
    "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
    "logs": null,
    "transactionHash": "0x26c8c6e23fa3b246f44fba53e7b5fcb55f01f1e075f2de3db9b982afd4bd3901",
    "contractAddress": "0x0000000000000000000000000000000000000000",
    "gasUsed": "0x7aae",
    "blockHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
    "transactionIndex": "0x0"
   },
   {
    "root": "0x",
    "status": "0x1",
    "cumulativeGasUsed": "0xdd24",
    "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
    "logs": null,
    "transactionHash": "0x26ea003b1188334eced68a720dbe89886cd6a477cccdf924cf1d392e2281c01b",
    "contractAddress": "0x0000000000000000000000000000000000000000",
    "gasUsed": "0x6276",
    "blockHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
    "transactionIndex": "0x1"
   },
   {
    "type": "0x1",
    "root": "0x",
    "status": "0x1",
    "cumulativeGasUsed": "0x14832",
    "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
    "logs": null,
    "transactionHash": "0x6997569ed85f1d810bc61d969cbbae12f34ce88d314ff5ef2629bc741466fca6",
    "contractAddress": "0x0000000000000000000000000000000000000000",
    "gasUsed": "0x6b0e",
    "blockHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
    "transactionIndex": "0x2"
   }
  ]
 }
}

@winsvega
Copy link
Collaborator

winsvega commented Feb 24, 2021

hm. now when the format is changed I need to change the parser check of the response files.

the receipt or state root hash I get from evm t8n tool.

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 24, 2021 via email

@winsvega
Copy link
Collaborator

Now. But the head of light client branch doesn't compile. Use head - 1

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 25, 2021

So both geth and OE thinks the root should be 0x485bb85f3a7e7fcf258228d475d08c3e835dc101052212a25a83afd061b2fd6e

Phew, I finally got it! I'm going to commit and push another update, hopefully the last one.

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

According to @MariusVanDerWijden , these now seem fine! Please merge, and we'll pull the latest and greatest into our go-ethereum CI test harness

@rakita
Copy link

rakita commented Feb 25, 2021

For OE we have failed eip2930 state tests, with state root mismatch: https://gist.github.com/rakita/7418931666fe5ed54ab3144d88579352

edit: it is OE problem i need to add support for access list in state tests.

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

@rakita as long as the tests pass in the form of block-tests, I think we can be confident that the tests are fine. They do, right?

@rakita
Copy link

rakita commented Feb 25, 2021

@holiman yes they do pass, I think we are okay to merge this.

@qbzzt I have a question about indexing of access_list:

"indexes" : {
   "data" : 32,
    "gas" : 0,
    "value" : 0
},

do I reuse data as access_list index?

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

We had some back-and-forth about this with @winsvega . In the end, yes, the data index is re-used for index to accesslist.

@winsvega
Copy link
Collaborator

How about we include transaction rlps into generated state test as well? Let's say in post section near to state root.

@jochem-brouwer
Copy link
Member

I have two problems with the current format.

(1) The EIP2930 tests do not have a chain id field. There should be one, we cannot assume that if none exists, then the chain id is 1 (if the field is empty, then we would normally encode this as the empty buffer)
(2) There is a field for which data/gas/value index we should use. AccessLists works similar. Therefore there should also be an access list field.

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

(1) The EIP2930 tests do not have a chain id field. There should be one, we cannot assume that if none exists, then the chain id is 1 (if the field is empty, then we would normally encode this as the empty buffer)

Actually, every single test requires a chainID, at least since we introduced the CHAINID opcode. It's not new for 2930. So the assumption 1 has always been there. That said, I wouldn't mind having it explicit in the env portion.

(2) There is a field for which data/gas/value index we should use. AccessLists works similar. Therefore there should also be an access list field.

I kind of agree. I think the idea of tying it with an existing index is that we then don't have to regenerate all existing tests. Or, it might also be that the combinatorics explode too much if we permute on four different indices..? Not sure. I'm kind of ok with how it is, but I would also have been ok with not having access lists indexed at all.

My personal preference is to just use what we have now, and move on..

@jochem-brouwer
Copy link
Member

I kind of agree. I think the idea of tying it with an existing index is that we then don't have to regenerate all existing tests. Or, it might also be that the combinatorics explode too much if we permute on four different indices..? Not sure. I'm kind of ok with how it is, but I would also have been ok with not having access lists indexed at all.

The problem is that currently it does not work if we have a test where we have both AccessList transactions and Legacy transactions.

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

The problem is that currently it does not work if we have a test where we have both AccessList transactions and Legacy transactions.

It's an accesslist-transaction if the field is non-null. It's a legacy if null.

@jochem-brouwer
Copy link
Member

It's an accesslist-transaction if the field is non-null. It's a legacy if null.

So this is a LegacyTransaction?

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

So this is a LegacyTransaction?

Yes. But let's check. It's at index 0 , transactionCosts, Berlin, so any transactionCosts_Berlin_d0 should show us. We have one here: https://github.com/ethereum/tests/pull/774/files#diff-4499e014502f7dca537b9a0e511b4c330d9aafa04483b3732e9493dcc513b842R2

Transaction here: https://github.com/ethereum/tests/pull/774/files#diff-4499e014502f7dca537b9a0e511b4c330d9aafa04483b3732e9493dcc513b842R93-R104

                "transactions" : [
                    {
                        "data" : "0x00",
                        "gasLimit" : "0x061a80",
                        "gasPrice" : "0x01",
                        "nonce" : "0x00",
                        "to" : "0xcccccccccccccccccccccccccccccccccccccccc",
                        "value" : "0x0186a0",
                        "v" : "0x1c",
                        "r" : "0xc721886959d3e92c88a23c492055a2e5168f4b954851d79309299f5b09bdbc3a",
                        "s" : "0x4a1d65ab446ae5e2f39ddd2b297452ccf32f24f874042cf38f5ec349096fe026"
                    }

Yep, indeed

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

And yes, we discussed it a bit further up: #774 (comment)

@jochem-brouwer
Copy link
Member

Ah great, thanks for the explanation @holiman! 😄

@winsvega
Copy link
Collaborator

Yes its getting quite complicated.
There is no indexing for accessLists as we don't need combinatorics here. I was even thinking to remove it since this complexity issues pop up.

To make things clear I want to introduce transaction rlps into gstate tests so there would be no need to build and sign the transaction on clients side, just take the rlp. And no indexing issue.
Such logic of building up the transaction should be the test generator only.

@holiman
Copy link
Contributor

holiman commented Feb 26, 2021

To make things clear I want to introduce transaction rlps

You keep mentioning transaction rlp -- I think we need to get rid of the notion that transactions are rlp. Transactions are a binary format which is not necessarily valid rlp, as they can start with the type-byte. So please don't name the field rlp, but instead maybe txdata or txbinary.

@winsvega
Copy link
Collaborator

ok. since the lightclient branch is merged. we merge this to develop.

@winsvega winsvega merged commit 86aaf60 into ethereum:develop Feb 26, 2021
@winsvega
Copy link
Collaborator

winsvega commented Feb 26, 2021

To make things clear I want to introduce transaction rlps

You keep mentioning transaction rlp -- I think we need to get rid of the notion that transactions are rlp. Transactions are a binary format which is not necessarily valid rlp, as they can start with the type-byte. So please don't name the field rlp, but instead maybe txdata or txbinary.

it is still rlp. because when you encode a block rlp transactions are encoded into that rlp. there for a list element in transaction list is a valid rlp.

like (data encoded as byte array) "0xb8c601f8c301800183061a8094095e7baea6a6c7c4c2dfeb977efac326af552d87830186a0821122f85bf859940000000000000000000000000000000000001337f842a00000000000000000000000000000000000000000000000000000000000000001a0000000000000000000000000000000000000000000000000000000000000000201a03bba0c93d55b783032ad80495ef18110a2e48a5c48d4ced9fc8d2e280848bf89a07c515b4c3c87c8b14cf2f9e2904b39b49501e7b6ec32209f8ce65714d68388ca",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants