Block sanity transfer test is impossible because the transfer limit is 0

michaelsproul commented 5 years ago

In the v0.6.3 block sanity tests, there's a transfer test that can't be passed using the default config because MAX_TRANSFERS is 0. We should either get rid of the test, or change the minimal config to use greater transfer limit.

protolambda commented 5 years ago

I'm having the same problem in ZRNT (go executable spec). Just ignoring the test case for now. The problem with changing the minimal config is that we also want to test the transfer rejection when MAX_TRANSFERS = 0.

I think we could split the preset into two:

Longer term non-changing constants, for compile-time array sizes etc.
A few configurable numbers, passed to the transition function, e.g. all MAX_<OPERATION_TYPE>, maybe some others. TBD.

Also see discussion around the initial constants setup. Here's some reasoning for constant constants:

For some things, we need both the old as the new constant to be present for a transition, and after the transition (to sync from older state before transition). Which makes it more tempting to just use a differently named constant in the rare cases of a change, for the old data type. And then have it all be compile time, without managing configurations. This is especially good for things like slots-per-epoch, where we need both the old value and the new value (and genesis time) to calculate a slot number based on time. Or to calculate the a amount of epochs since some time, for justification logic. In such cases a change involves more work than just loading a new value.

It breaks for cases like this however, where we want to be flexible in a "constant".

What I'd like to preserve:

constants used in data types are compile time, consistent. Change in a constant means its a different type, avoid bugs with invalid array lookups etc, and some efficiency + clarity in the code (no configuration overhead/obfuscation)
the values are easily distributed in one format, with one name, e.g. "mainnet". For a new phase, we append to the configuration (different name / maybe in some structure per-phase). Since it is time-based, we shouldn't need to re-define the whole thing for small fork / testnet change.

Idea for new configuration format:

constants:
  SHARD_COUNT: 1024
  SLOTS_PER_EPOCH: 64
  DOMAIN_SHARD_PROPOSER: 128 # includes phase 1 constants, not used in phase 0
  ...
forks: # variables, changing per fork
  genesis:
    MAX_ATTESTATIONS: 128
    MAX_TRANSFERS: 0
    ...
  transferstart: # can't think of a cool fork name
    MAX_TRANSFERS: 16  # overrides variables declared in earlier phases
  phase1:
    SHARD_GENESIS: 12345 # can also add new variables
    ...

And then some fork timeline configuration (name -> slot), separately:

genesis: 0
transferstart: 1234
phase1: 5678
...

And in a test we either set the pre-state slot to something after the transferstart fork, or make the test suite run with a timeline that starts transferstart from slot 0.

feedback/suggestions welcome.

djrtwo commented 5 years ago

We are excluding this test in v0.7.0. We'll want to add it back in when we add tests for forks (config changes)

ethereum / consensus-spec-tests

Block sanity transfer test is impossible because the transfer limit is 0 #7