Genesis schema #469

ehildenb · 2018-06-01T20:25:44Z

Initial attempt at a Genesis test schema. The example from ethereum/EIPs#1085 is at GenesisTests/dummy.json, and is passing the schema at JSONSchema/genesis-schema.json.

@winsvega and @arnetheduck

pirapira · 2018-06-05T10:23:50Z

@ehildenb is this not blocked anymore?

ehildenb · 2018-06-06T14:02:30Z

@pirapira I was hoping for some feedback from @winsvega and @arnetheduck on this issue, as they proposed having the schema in the AllCoreDevs meeting. If we want to merge anyway and fix the schema later, that's also fine with me.

ehildenb · 2018-06-06T14:39:15Z

@pirapira I need to make a few more changes before this is ready, will let you know when.

winsvega · 2018-06-06T16:03:26Z

JSONSchema/genesis-schema.json

+                    "additionalProperties": false,
+                    "properties": {
+                        "author": {
+                            "$ref": "#/definitions/HexData"


author is a hex address representation it hsouldbe a valid address.

No comment. Only say happy new year Github. Thanks you very much.

winsvega · 2018-06-06T16:04:32Z

should this script also check the possible range of the field or just the type of the field?
like extradata is a limited field
difficulty field also has limits. and so on.

ehildenb · 2018-06-06T17:13:46Z

Blocking this for #472.

ehildenb · 2018-06-06T19:24:15Z

@winsvega I've changed the field "author" to be a AddressMaybePrefixOrEmpty, which other addresses seem to be binned as as well.

Also, I've moved the file to JSONSchema/genesis-filler-schema.json, as I assume this is the schema for the Genesis fillers, and not the tests themselves. The dummy sample file I've moved to src/GenesisTestFillers/dummy.json.

I've updated the Makefile to include the genesis test filler validation, and the README also documents this. Still blocked on #472

ehildenb · 2018-06-07T13:38:40Z

Unblocking @pirapira and @winsvega.

arnetheduck · 2018-06-10T15:32:11Z

Just for the record, I'm not really involved with this issue, other than as a happy consumer (great to see this work!)

holiman

Good start! A few comments ...

holiman · 2018-06-12T19:33:53Z

JSONSchema/genesis-filler-schema.json

+                "balance",
+                "code",
+                "nonce",
+                "storage"


Not sure I think storage should be required. Actually, I would prefer if all these four are optional, and defaults to 0 (balance, nonce) or empty (code, storage).

I don't think we can assert anything about the actual value held, just the JSON structure. I could be wrong though, have not read on JSON schemas, just learned by examples.

I'll make these not required.

holiman · 2018-06-12T19:37:38Z

JSONSchema/genesis-filler-schema.json

+                        "byzantiumForkBlock",
+                        "chainID",
+                        "constantinopleForkBlock",
+                        "homesteadForkBlock",


Same for these -- if we make these required, we'll have to update the genesis spec to make future fork blocks also required -- otherwise it'll be inconsistent. So I would instead say that all forkblocks are implicitly maxUint64 unless otherwise specified. Don't know how to express that in json spec though...
A wild idea would be to have a separate section for those: params/forks/xxx.

I guess I'll make these not required for now...

holiman · 2018-06-12T19:40:49Z

JSONSchema/genesis-filler-schema.json

+                        "constantinopleForkBlock": {
+                            "$ref": "#/definitions/IntegerOrConfusedHex"
+                        },
+                        "homesteadForkBlock": {


We also have the daoForkBlock

holiman · 2018-06-12T19:42:16Z

JSONSchema/genesis-filler-schema.json

+                        "nonce": {
+                            "$ref": "#/definitions/IntegerOrConfusedHex"
+                        },
+                        "timestamp": {


Might make sense to add an optional number or blockNumber to the genesis. Particularly if we want to go with EIP 210, blockhash, it will be good to run tests on high numbered blocks.

ehildenb · 2018-06-13T04:34:26Z

@holiman I've incorporated your suggestions. One thing I wasn't sure about was whether you meant for chainID and miningMethod to also be optional?

Let me know what else should be changed. If you can provide an example of a filled test as well, I can write the schema for that.

holiman · 2018-06-13T07:52:39Z

One thing I wasn't sure about was whether you meant for chainID and miningMethod to also be optional?

Well, I don't have any clear opinion about those.

winsvega · 2018-06-13T12:22:55Z

JSONSchema/definitions.json

@@ -59,6 +59,20 @@
            },
            "type": "object"
        },
+        "FillerInfo": {


is this a test filler _info or a genesis json _info ?

a filled test _info has more fields that are required.
a filler test _info has only comment.

Do you think we should version the fillers? I think it would be a good idea to start doing that (so that we can add a test like "all the fillers have been updated to the newest version").

you mean filler format version?
stateTest -> GeneralStateTest

not so many people used fillers. there has been no need to track test filler format as only me and Yoichi used it. ideally yes. test fillers could be versioned and we could have a documentation stating what are the changes.

Yeah, I think for new formats (ie. Genesis) we should try for the "ideal" case. So I think the fillers should be versioned, and the filled should have hashes for the sources.

It will just make it easier to identify which fillers/filled need updated.

filled tests already has hash of the filler.
if there is going to be a vesrion of the filler format. that version would be different from genesis format version. I think we need different scheme validators here. if it is possible to split json validation into separate groups that would be great.

winsvega · 2018-06-13T12:28:26Z

JSONSchema/genesis-filler-schema.json

+                }
+            ]
+        },
+        "PreStateAccount": {


if its a preState account in tests then all fields are required.
if its a prestate account in Genesis then it has different scheme:

if (_obj.count(c_precompiled)) { // A precompiled contract requireJsonFields(_obj, "validateAccountObj", {{c_precompiled, {{js::obj_type}, JsonFieldPresence::Required}}, {c_wei, {{js::str_type}, JsonFieldPresence::Optional}}}); } else if (_obj.size() == 1) { // A genesis account with only balance set if (_obj.count(c_balance)) requireJsonFields(_obj, "validateAccountObj", {{c_balance, {{js::str_type}, JsonFieldPresence::Required}}}); else requireJsonFields(_obj, "validateAccountObj", {{c_wei, {{js::str_type}, JsonFieldPresence::Required}}}); }

so it could have only balance defined.

@winsvega not sure what you're suggesting here. Currently none of the fields in PreStateAccount are required, are you saying we should require balance?

hm. thats a good question. I think we should require balance.
account nonce is default. account code empty by default. same with storage.
but if we not declare any fields then what is the meaning of putting account into genesis?

winsvega · 2018-06-13T12:29:01Z

JSONSchema/genesis-filler-schema.json

+        "^.*$": {
+            "properties": {
+                "_info": {
+                    "$ref": "#/definitions/FillerInfo"


not a filler info then. its an info section of genesis.json
_ is added because json lib put _ first in a json.
the section could have version field so we have an idea what sheme version we are dealing with

winsvega · 2018-06-13T12:31:30Z

JSONSchema/genesis-filler-schema.json

+                    "additionalProperties": false,
+                    "patternProperties": {
+                        "^(0x)?[0-9a-f]*": {
+                            "$ref": "#/definitions/PreStateAccount",


again a genesis pre state account are different from tests pre state accounts.
genesis account could have only one field - balance. or all 4 standart fields. or if we change the format in the future it might change.

winsvega · 2018-06-13T12:32:31Z

JSONSchema/genesis-filler-schema.json

+                    "additionalProperties": false,
+                    "properties": {
+                        "author": {
+                            "$ref": "#/definitions/AddressMaybePrefixOrEmpty"


this is a question.
author field in genesis should not be empty I think.
and we should agree whether we use 0x prefix or not.

ehildenb · 2018-06-13T15:58:25Z

@winsvega I've tried incorporating your comments, but may have messed some things up, so take another look.

I would recommend looking through the changes with git log --patch on this branch, the commits marked sss at the beginning are the ones which address your comments.

I think we should require a version in the FillerInfo section in addition to the comment, so that it's easy to see when we update the format which tests need updated. This will also allow us to slowly migrate the other test-suites to be more standardized.

winsvega · 2018-06-13T18:45:21Z

changing the test fillers would require to regenerate all the tests at the current model. as tests has the hash of the test filler. or we could just copy the hash with a tool without generating a tests. but this should be done in a way that we promise that test itself has not been touched. I think filler _info section is another issue.

here I see the mess around json scheme validators. it is easy to mix a validator with some other test field. like _info at genesis and _info at test filler and _info at filled test.

winsvega · 2018-06-13T18:47:19Z

src/GenesisTestsFiller/dummy.json

@@ -1,6 +1,7 @@
 {
-    "dummy_test": {
+    "GenesisFillerTemplate": {


GenesisExampleTemplate

winsvega · 2018-06-13T18:50:55Z

src/GenesisTestsFiller/dummy.json

@@ -33,6 +31,7 @@
            "byzantiumForkBlock": "0x00",
            "chainID": "0x01",
            "constantinopleForkBlock": "0x00",
+            "daoForkBlock": "0x00",


if this fields are not required, what is the default value? thats important cause some client has it set to 0x00 and other to infinity.

also the names of the fields are important issue here. Parity has EIP155.
are devs agreed to use homesteadForkBlock, byzantiumForkBlock, EIP158ForkBlock for the fork names?

ehildenb · 2018-06-13T19:40:42Z

With the version number, it's more about being able to have a script which cycles through the test fillers and makes sure they are all bigger than a certain version number. That way, if we update the format at some point, it's easy to enforce that all the test fillers have been updated (either programmatically or by hand).

Either way, I think we should focus on "best practices" or the "ideal case" with any new test formats, so we don't end up in the same mess again. I can slowly work on making the old test-sets more conformant, so we have less of the stuff like ...OrEmpty in the validators.

winsvega · 2018-06-13T22:12:36Z

It is not that we change the test filler format so often...

ehildenb · 2018-06-13T22:39:27Z

It's more about best practices and future proofing than anything. I would like to begin evolving the VMTests format to make them simpler, for example, and it would be easier with versioning in place.

It won't always be us in charge of this repository, eventually someone will have to inherit it, and having versioning in place makes that process easier (as they will inevitably want to change things).

winsvega · 2018-06-13T22:46:44Z

VMTests you could take and experement with.

holiman · 2018-06-14T11:19:34Z

Regarding fork numbers and names. I think we could come to agreement about their names, if it's ByzantiumFork or EIPXXFork etc.
I still think we should have a separate section forks which contains hardfork numbers. Everything in that section should default to infinity unless specified otherwise (which is outside of the scope for the specification to dictate). That would make it more future-proof, imo, since other config params can be required, whereas more forks can be added later without having to rewrite old tests which lack later forks.

ehildenb · 2018-06-14T14:38:41Z

@winsvega I have removed "version" from being required (which I still think is a mistake).

@holiman I think I have implemented the change you are indicating with the hardfork numbers. The last commit (marked sss) makes the change to the file src/GenesisTestsFiller/template.json so that it passes the validator.

Anything else that should be changed? Can someone supply me with an example Filled GenesisTest so I can write the schema for that?

ehildenb · 2018-06-14T14:46:48Z

I've also pushed a new commit (latest one marked sss) which "tightens" the allowed things to show up in each field. Basically, anywhere possible I put HexData instead of IntegerOrConfusedHex.

That way the clients can have more limited code for reading in the test-sets instead of having to have lots of branches for the potential different ways that keys/values can be passed in.

winsvega · 2018-06-14T16:20:44Z

version field being required in genesis schema is ok
version field being required in test fillers is not ready yet. it will make all PRs to this repo red.
lets first make all tests with version field then add this rule.

…ler schema

…r testing Genesis Filler

ehildenb · 2018-06-17T20:24:16Z

I have added some initial genesis test templates at ./GenesisTests supplied by @folsen, as well as an initial genesis test validator at ./JSONSchema/genesis-schema.json. The last commit demonstrates how to enable said validator in the Makefile test-runner.

Someone should look over and let me know what needs to change, I'll adopt the templates in there to conform to the schema/update the schema a bit more this week.

winsvega · 2018-07-11T11:35:12Z

travis check fails.
what about fork block names?

winsvega · 2018-07-19T13:42:15Z

https://github.com/ehildenb/ethereum-tests/blob/4e24d411d67cde54bd1343501b6468a3d4f1489e/GenesisTests/basic_genesis_tests.json
I would like to create this tests with retesteth.
if any client dev is to implement genesis standart they could run
./retesteth -t compatibility/genesis (example)
so if they have test_setChainParams method the tool would try to send different genesis config requests to the client and see if it manage to parse it in response from the client via RPC.

ehildenb force-pushed the genesis-schema branch from 71f6140 to dd2061d Compare June 2, 2018 21:22

ehildenb mentioned this pull request Jun 2, 2018

[BLOCKED] Vmtests 2 generalstatetests #470

Closed

ehildenb force-pushed the genesis-schema branch from dd2061d to 73c96e0 Compare June 6, 2018 13:58

ehildenb changed the title ~~[BLOCKED] Genesis schema~~ Genesis schema Jun 6, 2018

winsvega reviewed Jun 6, 2018

View reviewed changes

ehildenb changed the title ~~Genesis schema~~ [BLOCKED] Genesis schema Jun 6, 2018

ehildenb force-pushed the genesis-schema branch from 73c96e0 to 67deb5a Compare June 6, 2018 19:21

ehildenb force-pushed the genesis-schema branch from 67deb5a to 3dd6765 Compare June 7, 2018 13:36

ehildenb changed the title ~~[BLOCKED] Genesis schema~~ Genesis schema Jun 7, 2018

holiman reviewed Jun 12, 2018

View reviewed changes

ehildenb force-pushed the genesis-schema branch from 3dd6765 to ab82ca6 Compare June 13, 2018 04:32

winsvega reviewed Jun 13, 2018

View reviewed changes

ehildenb force-pushed the genesis-schema branch from 9aab154 to 50e4b5d Compare June 14, 2018 14:36

JSONSchema/definitions: add definitions of Address and FillerInfo

02d0792

winsvega mentioned this pull request Jun 15, 2018

Ethereum Core Devs Meeting 40 Agenda ethereum/pm#44

Closed

Everett Hildenbrandt added 6 commits June 17, 2018 14:22

JSONSchema/genesis-filler-schema: inital attempt at a GenesisTestsFil…

aaf6bce

…ler schema

src/GenesisTestsFiller/template.json: template file as example and fo…

9968cb5

…r testing Genesis Filler

test.py: enable checking GenesisTest Schemas

e6afdeb

Makefile, README: enable GenesisTestsFiller sanitation

f5dd6ae

JSONSchema/genesis-schema: initial genesis test schema

80608fe

GenesisTests/: format/add two template tests

ed71073

ehildenb force-pushed the genesis-schema branch from a745fa2 to 4e24d41 Compare June 17, 2018 20:22

Makefile: enable failing genesis test checkers

4e24d41

winsvega mentioned this pull request Jul 26, 2018

[after Constantinople] test rpc ethereum/aleth#4859

Closed

winsvega referenced this pull request in ethereum/hive Aug 10, 2018

aleth: fix some remaining snags

049bbc0

winsvega mentioned this pull request Aug 10, 2018

Config files: aleth doesn't like it when precompiles have code/nonce/storage ethereum/aleth#5177

Open

winsvega added the expire soon outdated PR/Issue needs to be resolve or closed label Nov 29, 2018

winsvega closed this Mar 25, 2019

meowsbits mentioned this pull request Dec 5, 2019

info: references etclabscore/eth-x-chainspec#4

Open

Genesis schema #469

Genesis schema #469

Conversation

ehildenb commented Jun 1, 2018 • edited Loading

pirapira commented Jun 5, 2018

ehildenb commented Jun 6, 2018

ehildenb commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega commented Jun 6, 2018

ehildenb commented Jun 6, 2018

ehildenb commented Jun 6, 2018

ehildenb commented Jun 7, 2018

arnetheduck commented Jun 10, 2018

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehildenb commented Jun 13, 2018

holiman commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehildenb commented Jun 13, 2018

winsvega commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehildenb commented Jun 13, 2018

winsvega commented Jun 13, 2018

ehildenb commented Jun 13, 2018

winsvega commented Jun 13, 2018

holiman commented Jun 14, 2018

ehildenb commented Jun 14, 2018

ehildenb commented Jun 14, 2018

winsvega commented Jun 14, 2018 • edited Loading

ehildenb commented Jun 17, 2018

winsvega commented Jul 11, 2018

winsvega commented Jul 19, 2018 • edited Loading

ehildenb commented Jun 1, 2018 •

edited

Loading

winsvega Jun 13, 2018 •

edited

Loading

winsvega commented Jun 14, 2018 •

edited

Loading

winsvega commented Jul 19, 2018 •

edited

Loading