ethereum / consensus-spec-tests

Common tests for the Ethereum proof-of-stake consensus layer
MIT License
75 stars 24 forks source link

Make this repository self-sufficient #1

Closed zilm13 closed 5 years ago

zilm13 commented 5 years ago

There is one thing, which is missing from this repository but it's required to run these tests: configs, https://github.com/ethereum/eth2.0-specs/tree/dev/configs I propose to add them to these repository, so client implementations could run tests using only one submodule repository. Plus there will be no issues with different versions of repositories.

protolambda commented 5 years ago

Agree that it would be useful. I just copied them over to a repo of my own to submodule for now. As spec maintainers, we can either copy them to a separate repo, or copy them into the tests output. But both have a few drawbacks, and only solve part of the bigger problem.

The problem is, many clients don't fully use the configs yet, so there's not one way to get it "right". Also, some of the constants are more like configurables (e.g. MAX_TRANSFERS), while others are purely constants. I would like to clean that up before we formalize the usage of these constants. We need some tests to have more access to change individual configurables, while we also like clients to have access to make constants actually compile-time constants.

zilm13 commented 5 years ago

The problem is, many clients don't fully use the configs yet, so there's not one way to get it "right".

The main idea under my proposal to have configs next to the tests is to have same constants that were used in test generation.
That way MAX_TRANSFERS is a constant which is asserted in process_operations, it's not, say, honest validator parameter, which could be altered by any client. It's a constant which should be followed by any client.

protolambda commented 5 years ago

Please take a look at #7, we are looking into better constants/configuration options

zilm13 commented 5 years ago

@protolambda I'd prefer the configuration we are already using in our project: It's very close to your proposal, but we store configs in separate files, so mainnet.yaml

  SHARD_COUNT: 1024
  SLOTS_PER_EPOCH: 64
  DOMAIN_SHARD_PROPOSER: 128 # includes phase 1 constants, not used in phase 0
  ...

mainnet_genesis.yaml

    MAX_ATTESTATIONS: 128
    MAX_TRANSFERS: 0
    ...

Next, when configs are merged for the current slot, with forks, they are applied one by one in fork config order, so if any constant is overridden, the last value is actual. As a drawback of both solutions - it will be really not clear after, say, ~10 modifications, like we already have in eth1, what is the value of every constant. But anyway we have inheritance and composition and.. that's all.

Another idea came in the end but it looks like overengineering for me: mainnet_transfer_start.yaml

configs:
  - mainnet
  - mainnet_genesis
constants:
    MAX_ATTESTATIONS: 128
    MAX_TRANSFERS: 16
    ...
hwwhww commented 5 years ago

@protolambda

Also, some of the constants are more like configurables (e.g. MAX_TRANSFERS), while others are purely constants. I would like to clean that up before we formalize the usage of these constants.

It seems handled in the spec side with Constants and Configuration separation.

I'd vote for the "copy them into the tests output" solution. No need to add more submodule IMO. @protolambda @zilm13 what do you think?

ralexstokes commented 5 years ago

+1 on this; even just copying eth2.0-specs/configs/ into this repo (as tests are generated) would be much nicer

djrtwo commented 5 years ago

Looks like this is going to be resolved in https://github.com/ethereum/eth2.0-specs/pull/1320 by grouping tests by config and bringing in the associated config.yaml in test outputs to this dir

protolambda commented 5 years ago

Closing this issue, re-open if there is anything else to include.