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

Simplify Altair "genesis" #2323

Merged
merged 12 commits into from
Apr 27, 2021
Merged

Simplify Altair "genesis" #2323

merged 12 commits into from
Apr 27, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Apr 9, 2021

Issue

  1. In the pyspec test suite, we get Altair state with upgrade_to_altair:
    https://github.com/ethereum/eth2.0-specs/blob/6f095fc6916dcb7803b6cded68c0367f72a14f06/tests/core/pyspec/eth2spec/test/context.py#L76-L89
    It assumes that ALTAIR_FORK_SLOT is 0 in the test cases. Therefore, after upgrade_to_altair, the result state.latest_block_header is from Phase 0 latest_block_header.
    • -> That’s also what the first block block_1.parent_root points to.
  2. In fork choice spec on_block, the given block.parent_root must be known in store. (assert block.parent_root in store.block_states)

Now think about the Altair-only testnets, where ALTAIR_FORK_SLOT == 0. With the current spec, the initialization is like:

  1. Use initialize_beacon_state_from_eth1 to get genesis_state: phase0.BeaconState.
  2. Get genesis_block = phase0.BeaconBlock(state_root=hash_tree_root(genesis_state))
  3. Call upgrade_to_altair to get state: altair.BeaconState
  4. Initialize fork choice store with get_forkchoice_store(state, genesis_block)
    • Note that they are state: altair.BeaconState and genesis_block: phase0.BeaconState

It could be simplified if not using upgrade_to_altair:

  1. Use new initialize_beacon_state_from_eth1 to get genesis_state: phase1.BeaconState.
  2. Get genesis_block = altair.BeaconBlock(state_root=hash_tree_root(genesis_state))
  3. Initialize fork choice store with get_forkchoice_store(genesis_state, genesis_block)
    • Note that they are state: altair.BeaconState and genesis_block: altair.BeaconState

Proposed solutions

  1. [context.py] We can just use create_genesis_state helper to generate the “initial state” for Altair or other versions.
def _prepare_state(balances_fn: Callable[[Any], Sequence[int]], threshold_fn: Callable[[Any], int],
                   spec: Spec, phases: SpecForks):
    phase = phases[spec.fork]
    balances = balances_fn(phase)
    activation_threshold = threshold_fn(phase)
    state = create_genesis_state(spec=phase, validator_balances=balances,
                                 activation_threshold=activation_threshold)
    return state
  1. [Fork choice tests] Change get_genesis_forkchoice_store_and_block helper to use spec.BeaconBlock instead of phase0_spec.BeaconBlock(state_root=genesis_state.hash_tree_root())
  2. [beacon-chain.md] Add initialize_beacon_state_from_eth1 in Altair specs, which is only used in the test suite and for testnets.

Pros:

  • Simpler testnet initialization.

Cons:

  • DRY: it introduces some redundant code in initialize_beacon_state_from_eth1 and upgrade_to_altair. But I think we can refactor it to be acceptable.
  • Much less tests cover upgrade_to_altair: after this change, only test_fork.py tests will cover upgrade_to_altair.

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 9, 2021

/cc @ajsutton @benjaminion

I think the anchor_block deserialization issue you saw may be caused by phase0.BeaconBlock v.s. altair.BeaconBlock?

@hwwhww hwwhww added the Altair aka HF1 label Apr 9, 2021
@hwwhww hwwhww changed the title Test vectors for Altair "genesis" Simplify Altair "genesis" Apr 9, 2021
@ajsutton
Copy link
Contributor

/cc @ajsutton @benjaminion

I think the anchor_block deserialization issue you saw may be caused by phase0.BeaconBlock v.s. altair.BeaconBlock?

Yes you're right - I thought I'd tried that in the past but apparently got it wrong somehow. If I forcibly create a phase0 "spec" to parse that initial block then the Altair fork choice tests pass for me.

Note that the Altair genesis reference tests do not run upgrade_to_altair and the sync committees in the state wind up filled with zeroed out public keys. I logged #2318 about that. The Altair initialize_beacon_state_from_eth1 should also calculate the current and next sync committees.

I'm a big fan of being able to start from a pure Altair genesis and not need any phase0 code because that leaves the possibility of eventually dropping phase0 support from codebases entirely and just using a later anchor state to sync from. Not sure if we'll ever actually do that but it would be really nice if it were at least possible so new clients can just implement the latest spec instead of having to implement every historical phase as well.

@ethereum ethereum deleted a comment Apr 12, 2021
@hwwhww hwwhww marked this pull request as ready for review April 16, 2021 03:45
@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 16, 2021

Thanks @ajsutton for the reply! Right, so this PR also fixes #2318.

I think we need to add other more fork choice rule tests around the fork boundary. (not in this PR)
#2333 is the first issue that popped up.

@djrtwo @protolambda
I’m thinking about if we can:

  1. Set ALTAIR_FORK_SLOT = 0 in the minimal config
  2. Add minimal_fork config, which is as same as the minimal config except for ALTAIR_FORK_SLOT = 64 (some number that is not too large but have passed epochs) for testing the boundaries.

@ajsutton
Copy link
Contributor

Agree that we should be able to test both Altair at genesis and Altair at some point after Phase0. I think we also want a config where Altair doesn't kick in though (ie ALTAIR_FORK_SLOT = FAR_FUTURE_EPOCH).

It sounds like it would be really useful to be able to say "minimal spec but with altair fork at epoch X". Otherwise we're going to get a bit of an explosion of configs as we add more forks.

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 20, 2021

@ajsutton I updated the docs in 666f847:

  1. When handling test vector:
    • By default, use the given version to deserialize the binary data.
    • We might add some meta fields we have test cases cross fork boundary. Right now, only the forks tests cross the fork boundary and the test format described it.
    • Use new initialize_beacon_state_from_eth1 to run the genenis tests.
  2. When handling the pure Altair testnet:
    • Use new initialize_beacon_state_from_eth1 to generate the genesis.
    • Do not apply upgrade_to_altair.

Keeping ALTAIR_FORK_SLOT = FAR_FUTURE_EPOCH for the minimal case now. We can add a new config once we have new test cases that cross the fork boundary.

/cc @djrtwo

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

A few minor comments. I'm okay with this approach but am concerned about being able to more dynamically set fork slots for testing

ALTAIR_FORK_SLOT = FAR_FUTURE_EPOCH -- Why are we setting it to far future epoch instead of GENESIS_SLOT? Don't we want the fork to by default happen for all of the tests today?

EDIT: I remember now that we did this for safety

Should fork slots be a part of the core config? Maybe these should live in a separate file. Core configs are often used for compile targets and are expected to be few and far between, but we'll likely want fork slots to be highly configurable.

specs/altair/beacon-chain.md Show resolved Hide resolved
specs/altair/beacon-chain.md Outdated Show resolved Hide resolved
specs/altair/fork.md Outdated Show resolved Hide resolved
@hwwhww hwwhww requested a review from djrtwo April 27, 2021 14:09
specs/altair/beacon-chain.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Altair aka HF1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants