remove eip6110flag for validator index cache #14173
Conversation
| @@ -179,7 +179,7 @@ func (b *BeaconState) ValidatorIndexByPubkey(key [fieldparams.BLSPubkeyLength]by | |||
| b.lock.RLock() | |||
| defer b.lock.RUnlock() | |||
|
|
|||
| if features.Get().EIP6110ValidatorIndexCache { | |||
| if b.Version() >= version.Electra { | |||
There was a problem hiding this comment.
what do we do about validatorMultiValue and the valMapHandler?
james-prysm
added
Electra
| @@ -761,6 +762,8 @@ func InitializeFromProtoUnsafeElectra(st *ethpb.BeaconStateElectra) (state.Beaco | |||
| valMapHandler: stateutil.NewValMapHandler(st.Validators), | |||
| } | |||
|
|
|||
| b.SaveValidatorIndices() | |||
There was a problem hiding this comment.
is this appropriate for initialization?
|
Do you need the feature flag for testing? Consider renaming the flag if the justification is that other eip features use ValidatorByPubkey |
I believe we actually need it now not behind a feature flag, asking the team for more thoughts and review |
| stateFieldLeaves: make(map[types.FieldIndex]*fieldtrie.FieldTrie, fieldCount), | ||
| rebuildTrie: make(map[types.FieldIndex]bool, fieldCount), | ||
| valMapHandler: stateutil.NewValMapHandler(st.Validators), | ||
| validatorIndexCache: newFinalizedValidatorIndexCache(), //only populates when finalizing, otherwise it falls back to validator set |
There was a problem hiding this comment.
Shouldn't you copy the comment to all other state types?
There was a problem hiding this comment.
Is this an electra specific behaviour?
There was a problem hiding this comment.
it's electra specific but i was afraid there might be panics if not initialized
There was a problem hiding this comment.
it's not Electra-specific, this field is present in all states
| @@ -110,7 +109,7 @@ func (b *BeaconState) SetHistoricalRoots(val [][]byte) error { | |||
|
|
|||
| // SaveValidatorIndices save validator indices of beacon chain to cache | |||
| func (b *BeaconState) SaveValidatorIndices() { | |||
| if !features.Get().EIP6110ValidatorIndexCache { | |||
| if b != nil && b.Version() < version.Electra { | |||
There was a problem hiding this comment.
If b is nil, the function will continue and panic at the next line. It should be b == nil || instead. Either way I think nil-checking receivers is too paranoid and actually harmful here. You should never call this method on a nil beacon state, so it's better just to panic and inform the developer that there is a bug here.
What type of PR is this?
Other
What does this PR do? Why is it needed?
because this cache is used for not only EIP6110 but also some others like 7521 we would like a way to use it out of the box without the need for a feature flag. longer term we need to revisit if we should solely depend on the cache or utilize the existing state features.
continuation of #14146
Which issues(s) does this PR fix?
Fixes #
Other notes for review