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

Add initial test cases for EXTCODEHASH #484

Merged
merged 2 commits into from
Nov 9, 2018
Merged

Add initial test cases for EXTCODEHASH #484

merged 2 commits into from
Nov 9, 2018

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Aug 9, 2018

WIP

Depends on ethereum/solidity#4676 .

@jwasinger jwasinger force-pushed the extcodehash branch 2 times, most recently from a447b65 to 4f37d8f Compare August 9, 2018 02:07
"gas" : -1,
"value" : -1
},
"network" : ["<=Byzantium"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to test this on all networks. could just leave Byzantium.

"gas" : -1,
"value" : -1
},
"network" : ["<=Byzantium"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. Byzantium is enough

@zilm13
Copy link

zilm13 commented Aug 20, 2018

Why mixHash and nonce fields are missed in block's rlp? Is it new test format or whatever?

@winsvega
Copy link
Collaborator

Yes. It is NoProof blocks meaning that you dont have to check the difficulty

@zilm13
Copy link

zilm13 commented Aug 21, 2018

@winsvega aww found it, #480
Thx!

@zilm13
Copy link

zilm13 commented Aug 21, 2018

@winsvega still bad blocks for me
hash field of block and lastblockhash look calculated like sha3(all fields except mixHash and nonce), the way it's formed when used for mining, but actual hash of imported block is with them (zeroed fields included) and it doesn't match one in json

@winsvega
Copy link
Collaborator

The way hash is calculated is not touched.

@zilm13
Copy link

zilm13 commented Aug 22, 2018

            {
                "blockHeader" : {
                    "bloom" : "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
                    "coinbase" : "0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba",
                    "difficulty" : "0x020000",
                    "extraData" : "",
                    "gasLimit" : "0x0f4240",
                    "gasUsed" : "0x69f7",
                    "hash" : "0x0e3d2b038dda5f7a8e3621862232ba5c489774ac96f4acba1e3e81a5c903680d",
                    "mixHash" : "0x0000000000000000000000000000000000000000000000000000000000000000",
                    "nonce" : "0x0000000000000000",
                    "number" : "0x01",
                    "parentHash" : "0x05a4be4021502b3b9074074c873ce9df7bed888439ac6316ffca01defe371847",
                    "receiptTrie" : "0x85d079c46e813cecaed0dc37aae4cb50995a963452e7621d92124ad645b4cfa3",
                    "stateRoot" : "0x42ee20998ddf3e1b783586065624c9f90e6623fa40f0a2597591c9f2d6783cab",
                    "timestamp" : "0x03e8",
                    "transactionsTrie" : "0xf2fde0741a86c3f5b87ccee3cce6faafd15be92989188f40919cb13422672757",
                    "uncleHash" : "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347"
                },
                "rlp" : "0xf90235f901cda005a4be4021502b3b9074074c873ce9df7bed888439ac6316ffca01defe371847a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa042ee20998ddf3e1b783586065624c9f90e6623fa40f0a2597591c9f2d6783caba0f2fde0741a86c3f5b87ccee3cce6faafd15be92989188f40919cb13422672757a085d079c46e813cecaed0dc37aae4cb50995a963452e7621d92124ad645b4cfa3b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408269f78203e880f862f860800183061a8094095e7baea6a6c7c4c2dfeb977efac326af552d8701801ca0ae29a79903b187c6d075320f9834a330b5134244c7fb0476eba58cd87b175fd6a027a8f85308f5f08f4e2f6b2e23d9dcfd740108f8f96517e50c9e1dc86288a673c0",
                "transactions" : [
                    {
                        "data" : "0x",
                        "gasLimit" : "0x061a80",
                        "gasPrice" : "0x01",
                        "nonce" : "0x00",
                        "r" : "0xae29a79903b187c6d075320f9834a330b5134244c7fb0476eba58cd87b175fd6",
                        "s" : "0x27a8f85308f5f08f4e2f6b2e23d9dcfd740108f8f96517e50c9e1dc86288a673",
                        "to" : "0x095e7baea6a6c7c4c2dfeb977efac326af552d87",
                        "v" : "0x1c",
                        "value" : "0x01"
                    }
                ],
                "uncleHeaders" : [
                ]
            }

Taken from first Constantinople test here:
https://github.com/ethereum/tests/blob/extcodehash/BlockchainTests/GeneralStateTests/stExtCodeHash/extCodeHashCallCode_d0g0v0.json

Constructing block from fields provided:

f9025ff901f7a005a4be4021502b3b9074074c873ce9df7bed888439ac6316ffca01defe371847a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa042ee20998ddf3e1b783586065624c9f90e6623fa40f0a2597591c9f2d6783caba0f2fde0741a86c3f5b87ccee3cce6faafd15be92989188f40919cb13422672757a085d079c46e813cecaed0dc37aae4cb50995a963452e7621d92124ad645b4cfa3b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408269f78203e880a00000000000000000000000000000000000000000000000000000000000000000880000000000000000f862f860800183061a8094095e7baea6a6c7c4c2dfeb977efac326af552d8701801ca0ae29a79903b187c6d075320f9834a330b5134244c7fb0476eba58cd87b175fd6a027a8f85308f5f08f4e2f6b2e23d9dcfd740108f8f96517e50c9e1dc86288a673c0

Please notice the difference between 03e880 (timestamp and null extraData) and f862 (transactions start).

block header's rlp:
0xf901f7a005a4be4021502b3b9074074c873ce9df7bed888439ac6316ffca01defe371847a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa042ee20998ddf3e1b783586065624c9f90e6623fa40f0a2597591c9f2d6783caba0f2fde0741a86c3f5b87ccee3cce6faafd15be92989188f40919cb13422672757a085d079c46e813cecaed0dc37aae4cb50995a963452e7621d92124ad645b4cfa3b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408269f78203e880a00000000000000000000000000000000000000000000000000000000000000000880000000000000000
keccak-256 of block header's rlp (full): 0xb3dc58129434b0c20bbed833cb9ef8249d9646af33c2b5c153af3c640aee6715

block header's rlp (nonce and mixHash omitted): 0xf901cda005a4be4021502b3b9074074c873ce9df7bed888439ac6316ffca01defe371847a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa042ee20998ddf3e1b783586065624c9f90e6623fa40f0a2597591c9f2d6783caba0f2fde0741a86c3f5b87ccee3cce6faafd15be92989188f40919cb13422672757a085d079c46e813cecaed0dc37aae4cb50995a963452e7621d92124ad645b4cfa3b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408269f78203e880
keccak-256 of block header's rlp (nonce and mixHash omitted): 0x0e3d2b038dda5f7a8e3621862232ba5c489774ac96f4acba1e3e81a5c903680d

From Ethereum yellow paper:
screen shot 2018-08-22 at 15 44 52
Hm and Hn in the end corresponds to mixHash and nonce respectively
screen shot 2018-08-22 at 15 51 41

So we have incorrect rlp field in tests json (no mixHash and nonce included in list, plus they doesn't match provided mixHash and nonce values in the same json) and incorrect block header's hash calculated with no mixHash and nonce in serialized header (Hp, Ho, Hc, Hr, Ht, He, Hb, Hd,
Hi, Hl, Hg, Hs).

Where I am wrong?
I understand the idea to skip PoW validation but I don't understand why to change Block's serialization format and hash formula. Or did I just miss something?

@winsvega
Copy link
Collaborator

winsvega commented Aug 22, 2018

The provided RLP is correct as long as rlp syntax is concerned.
The blockheader is basically contains no signature.
Now I am trying to get if the blockhash is calculated correctly.

but actual hash of imported block is with them (zeroed fields included) and it doesn't match one in json
what you mean by actual hash?

@jwasinger
Copy link
Contributor Author

jwasinger commented Aug 23, 2018

Side note: I've been on an ewasm sprint. Will try to add a few more test cases next week.

@zilm13
Copy link

zilm13 commented Aug 23, 2018

@winsvega

what you mean by actual hash?

Hash calculated from full header, 0xb3dc58129434b0c20bbed833cb9ef8249d9646af33c2b5c153af3c640aee6715

@zilm13
Copy link

zilm13 commented Aug 23, 2018

@winsvega

The provided RLP is correct as long as rlp syntax is concerned.
The blockheader is basically contains no signature.

But nonce and mixHash are provided above in json. You will get another block's rlp from that data

@winsvega
Copy link
Collaborator

winsvega commented Aug 23, 2018

You mean in the blockheader fields?
Would it help to make those fielda empty "" instead of 0x00000000000 ?

"mixHash" : "",
"nonce" : "",

@zilm13
Copy link

zilm13 commented Aug 23, 2018

@winsvega null fields are encoded as 80, so it will add 8080 to the rlp and change the hash

@winsvega
Copy link
Collaborator

winsvega commented Aug 23, 2018

I think BlockHeader information in json should be removed then.
We already have those field in RLP.
And decoding an RLP is not the point of a blockchain tests.

could we just agree that 0x000..00 values of nonce and mixhash means block without signature for now?

@zilm13
Copy link

zilm13 commented Aug 23, 2018

@winsvega I think it's a good idea to exclude redundant RLP serialize/deserialize testing in all tests.

could we just agree that 0x000..00 values of nonce and mixhash means block without signature for now?

Better to exclude this fields at all, so at least rlp == block in json. But still I will have difficulties to run this tests. I will need to override default hash only for this kind of tests with attention to not break anything in real run.
Best thing for us (Harmony/EthereumJ) - mixHash and nonce = "", add 8080 to rlp and calculate hash from rlp where this 8080 is included.
Any feedback about new format from Geth and Parity? If they are ok with current format we will find a way to run it too. Anyone running them already except cpp?

@winsvega
Copy link
Collaborator

No feedback from geth and Parity. they work through hive.

another solution might be following.
I change testeth to fill this fields not with empty 0x0000 but with some constant hash-looking data that is to be ignored. (rlp as well)

@winsvega
Copy link
Collaborator

created a fix for this issue.
ethereum/aleth#5219

require tests refill after the PR get merged.

@zilm13
Copy link

zilm13 commented Aug 24, 2018

@winsvega cool! Thank you!

@holiman
Copy link
Contributor

holiman commented Aug 24, 2018

Sorry if I'm late to the discussion. I think filling nonce and mixHash with zeroes is a better solution thatn leaving them out -- there may be clients which expects them to be a certain size, and even if that client can read the RLP, it will probably output zeroes when serializing it for hashing later on.

EDIT: Doesn't have to be zeroes, can be anything, for example 0x000...000deadc0de which IMO is a lot better than 'hash-looking data', since anyone debugging those tests can clearly see that there's something fishy and ask about it.

EDIT2: Just checked the PR, 42 works for me :)

@winsvega
Copy link
Collaborator

I take this PR over

@rmeissner
Copy link

@winsvega is there any update on this? :)

@holgerd77 holgerd77 changed the title Add new test cases for EXTCODEHASH Add initial test cases for EXTCODEHASH Nov 8, 2018
@winsvega
Copy link
Collaborator

winsvega commented Nov 8, 2018

this PR covers first 6 cases from this list

https://docs.google.com/spreadsheets/d/1xat7UI8GtB4ZGVdlK5_XQSHJZaMThi4SrlcL8XMZb5Q/edit?pli=1#gid=1811198384

and the test fillers here could be used as a templates for creating the following test cases from that list.
for any newcomers this fillers might be templates. the discussion on this PR is too big. better continue with a new PR.

@winsvega winsvega merged commit 080c419 into develop Nov 9, 2018
@winsvega winsvega deleted the extcodehash branch September 5, 2019 10:11
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