Cleaner genesis state population #11809
Conversation
previous set of functions was a copypasta mess
kasey
requested review from
rauljordan,
prestonvanloon,
terencechain,
nisdas and
rkapka
| FinalizedCheckpoint: ðpb.Checkpoint{ | ||
| Epoch: 0, | ||
| Root: params.BeaconConfig().ZeroHash[:], | ||
| }, |
There was a problem hiding this comment.
I stopped using this function in favor of the .empty method on the premineGenesisConfig type, so I can revert this change, but it is probably a good idea to initialize these empty values.
| @@ -227,6 +227,8 @@ type WriteOnlyCheckpoint interface { | |||
| type WriteOnlyAttestations interface { | |||
| AppendCurrentEpochAttestations(val *ethpb.PendingAttestation) error | |||
| AppendPreviousEpochAttestations(val *ethpb.PendingAttestation) error | |||
| SetPreviousEpochAttestations([]*ethpb.PendingAttestation) error | |||
| SetCurrentEpochAttestations([]*ethpb.PendingAttestation) error | |||
There was a problem hiding this comment.
I added these setters so that the .empty method of premineGenesisConfig could purely use the state.BeaconState interface to prepare the BeaconState.
| } | ||
|
|
||
| b.previousEpochAttestations = al | ||
| b.markFieldAsDirty(nativetypes.PreviousEpochAttestations) |
There was a problem hiding this comment.
I wasn't sure if I also need to call addDirtyIndices like the Append methods do?
| t := e2e.TestParams.CLGenesisTime | ||
| nvals := params.BeaconConfig().MinGenesisActiveValidatorCount | ||
| version := e2etypes.GenesisFork() | ||
| return spectypes.NewPreminedGenesis(ctx, t, nvals, version, gb) |
There was a problem hiding this comment.
NewPreminedGenesis does most of the heavy lifting now so this component gets much simpler.
| @@ -0,0 +1,28 @@ | |||
| load("@prysm//tools/go:def.bzl", "go_library") | |||
There was a problem hiding this comment.
I felt a little meh about the spectypes package name, open to suggestions.
There was a problem hiding this comment.
Curious, is there any reason the core package couldn't be used ? A lot of the previous state related functions were added in there in your previous PRs
There was a problem hiding this comment.
An out-of-band discussion led us to moving this file to testing/utils for now, because a number of similar "populate a struct for testing" functions live there.
testing/spectypes/premine-state.go
Outdated
| var body rooter | ||
| switch s.Version { | ||
| case version.Phase0: | ||
| body = ðpb.BeaconBlockBody{ |
There was a problem hiding this comment.
maybe it would be nice to factor this into a method that NewGenesisBlockForState could share. Note the only difference is that NewGenesisBlockForState wants to actually set the StateRoot attribute in the body, but it could do that after calling the shared method. I didn't look to see if the beacon block interface has comprehensive setters like BeaconState does. If so I could rewrite this to use the interface setters as well.
testing/spectypes/premine-state.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| wh, err := blocks.WrappedExecutionPayloadHeader(eph) |
There was a problem hiding this comment.
The back and forth between wrapped and concrete types is rather awkward. In particular I wondered why PayloadToHeader gives the concrete type rather than interfaces.ExecutionData? Maybe obvious if I look at how it's used elsewhere but I haven't done that yet.
testing/spectypes/premine-state.go
Outdated
| } | ||
|
|
||
| // NewPreminedGenesis creates a genesis BeaconState at the given fork version, suitable for using as an e2e genesis. | ||
| func NewPreminedGenesis(ctx context.Context, t, nvals uint64, version int, gb *types.Block) (state.BeaconState, error) { |
There was a problem hiding this comment.
I am a bit surprised by the location of this file. Is there any reason it is in our Just realised its a new packagespectests folder ?
| @@ -0,0 +1,28 @@ | |||
| load("@prysm//tools/go:def.bzl", "go_library") | |||
There was a problem hiding this comment.
Curious, is there any reason the core package couldn't be used ? A lot of the previous state related functions were added in there in your previous PRs
What type of PR is this?
Other
What does this PR do? Why is it needed?
#11746 introduced some super nasty copypasta for populating BeaconState values in order to generate the Bellatrix genesis state. This PR cleans that up, adding a new beacon state generator that attempts to reduce boilerplate across forks by using the
state.BeaconStatesetters.Other notes for review
Shout out to @rkapka for all the hard work on native beacon state and moving everything to using the interface - this wouldn't be possible without all that effort.