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

<- Merge/foundation release/1.10.1 resolved: cross-client tests and tests generators/fillers #353

Conversation

meowsbits
Copy link
Contributor

@meowsbits meowsbits commented Apr 1, 2021

This is a refactoring and improving of cross-client test generation.

Issues provoking this

  • Parity configuration (does not support Belin EIPs); I have deprecated Parity config-format support for EIPs installed after Istanbul.
    • Remove Makefile commands that do Parity stuff (but core functionality remains, just not conveniently through Makefile).

Does

  • Documents configurations used in State tests at tests/testdata/GeneralStateTests_configs.json as a map keyed on the subtest Fork names used in those tests, eg Berlin and ETC_Magneto.
    • These configurations are now core-geth format instead of parity format.

I'll make more notes inline with the code to point out important stuff.

See generated tests diff at etclabscore/tests#4

meowsbits and others added 30 commits March 31, 2021 06:45
Date: 2021-03-31 06:45:50-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 06:57:24-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 07:08:19-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 07:08:40-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 07:15:05-05:00
Signed-off-by: meows <b5c6@protonmail.com>
…tests

Stopping here because I'm realizing that
its going to be more complicated than just
writing one genesis+chain config for each
Fork.

Each StateTest defines its own genesis env,
so that each test can have its own genesis.

We have to write ONLY chain configs. Not genesis.

Date: 2021-03-31 07:32:33-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Use only chain config values (not genesis).

Date: 2021-03-31 07:39:58-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 07:41:20-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 07:56:14-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 07:57:03-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 08:28:27-05:00
Signed-off-by: meows <b5c6@protonmail.com>
… specs)

Date: 2021-03-31 08:49:56-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 08:51:03-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 08:53:41-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 08:55:02-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 08:55:58-05:00
Signed-off-by: meows <b5c6@protonmail.com>
… chainspec'd difficulty tests

Date: 2021-03-31 08:58:09-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 08:58:25-05:00
Signed-off-by: meows <b5c6@protonmail.com>
…gain

Running difficulty tests yields missing gaslimit
for genesiss type, which means the file only contains
the chain config and not the genesis values.

I assume this is because there were pre-existing files
which were not overwritten with genesis information.

Date: 2021-03-31 09:11:47-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:12:16-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:13:56-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:14:08-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:14:28-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:14:53-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:21:54-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 09:22:31-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Signed-off-by: meows <b5c6@protonmail.com>
Signed-off-by: meows <b5c6@protonmail.com>
Signed-off-by: meows <b5c6@protonmail.com>
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-03-31 19:15:50-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 03:51:44-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 04:13:50-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 04:17:59-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 04:32:24-05:00
Signed-off-by: meows <b5c6@protonmail.com>
…e transations

Notably these tests need to work for the matrix
of all configuration schemas (eg. Multigeth) which
is not always super pretty.

The special case for EIP1283 is handled specific to the
troublesome/indicative test for now; it could also
be handled at the Configurator method level
as a general case, but that's going too deep for me right
now.

Date: 2021-04-01 05:50:23-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 06:43:25-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 06:44:18-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 06:46:31-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 08:05:14-05:00
Signed-off-by: meows <b5c6@protonmail.com>
@@ -102,15 +107,17 @@ tests-generate: tests-generate-state tests-generate-difficulty ## Generate all t
tests-generate-state: ## Generate state tests.
@echo "Generating state tests."
env COREGETH_TESTS_GENERATE_STATE_TESTS=on \
env COREGETH_TESTS_CHAINCONFIG_FEATURE_EQUIVALENCE_COREGETH=on \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

COREGETH_TESTS_CHAINCONFIG_FEATURE_EQUIVALENCE_COREGETH forces all Forks values used by the tests to be confp.Converted on init to &coregeth.ChainConfig type values. These config values are dumped JSON encoded to an adjacent file.

No longer do clients' test runner have to somehow magically know what "Berlin" means; they can look it up and see exactly what config should be used.

@@ -60,6 +60,8 @@ var (
EIP145FBlock: big.NewInt(9573000),
EIP1014FBlock: big.NewInt(9573000),
EIP1052FBlock: big.NewInt(9573000),
// EIP1283FBlock: big.NewInt(9573000),
// PetersburgBlock: big.NewInt(9573000),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added these as documentation to reflect the related Constantinople config; there were issues with EIP1234 that came up in other places and this was part of my investigation.

return nil
}
}

c.ensureExistingRewardSchedule()
c.BlockRewardSchedule[*n] = vars.EIP649FBlockReward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two changes are important. The prevent a configuration from overwriting an already-configured EIP1234 value with a same-block EIP649 configuration; essentially codifying that EIP649 can not be set after EIP1234.

Date: 2021-04-01 08:25:26-05:00
Signed-off-by: meows <b5c6@protonmail.com>
@@ -313,6 +313,8 @@ func (c *ChainConfig) SetEIP1052Transition(n *uint64) error {
}

func (c *ChainConfig) GetEIP1283Transition() *uint64 {
// This is special case handling for multigeth configuration.
// It makes the tests pass.
if c.ConstantinopleBlock != nil && c.PetersburgBlock != nil {
if c.ConstantinopleBlock.Cmp(c.PetersburgBlock) == 0 {
return nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an ideal thing, and it differs from the goethereum implementation. This is due, in part, to core-geth's original inheritance of the multi-geth configuration format and need to support some old strange versions of the multi-geth config.

Copy link
Contributor Author

@meowsbits meowsbits Apr 1, 2021

Choose a reason for hiding this comment

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

I looked at generalizing the interaction of EIP1283 and EIP1283Disable as Disable-type methods at the Configurator.(Equivalent|Compatible), but decided that was too involved for now; and this case currently only applies to 1283, being the only EIP which has been explicitly disabled.

@@ -85,18 +85,17 @@ type stInfo struct {
LLLCVersion string `json:"lllcversion"`
Source string `json:"source"`
SourceHash string `json:"sourceHash"`
WrittenWith string `json:"written-with"`
Parent string `json:"parent"`
ParentSha1Sum string `json:"parentSha1Sum"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These three fields were specific to core-geth test generation metadata. I've removed them since their information be inferred with version control, and I use the FilledWith field to define the core-geth version (instead of WrittenWith).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also Chainspecs is disused since the tests generation creates the GeneralStateTests_configs.json which defines the test configurations; making references to configs by file unnecessary.

WrittenWith string `json:"written-with"`
Parent string `json:"parent"`
ParentSha1Sum string `json:"parentSha1Sum"`
Chainspecs chainspecRefsT `json:"chainspecs,omitempty"`
Labels map[string]interface{} `json:"labels,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Labels was apparently added at the tests suite.

Date: 2021-04-01 17:11:08-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 17:11:36-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 17:12:37-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 17:22:35-05:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2021-04-01 17:33:45-05:00
Signed-off-by: meows <b5c6@protonmail.com>
@meowsbits meowsbits merged commit 2eea1c4 into merge/foundation-release/1.10.1-resolved Apr 4, 2021
@meowsbits meowsbits deleted the merge/foundation-release/1.10.1-resolved-writing-chainconfigs-regenerate-again branch April 4, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants