-
Notifications
You must be signed in to change notification settings - Fork 922
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
More feature flags removal #7526
Conversation
state, err = executeStateTransitionStateGen(ctx, state, signed[i]) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong.
Previous implementation with the flag on was this
state, err := transition.ExecuteStateTransition(ctx, state, signed[i])
But now you are using executeStateTransitionStateGen
? Why?
Why is this feature not being released, but removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline https://discordapp.com/channels/476244492043812875/483017808658169866/765740051450691604
This is OK to remove, the flag was never used and should have been in the form of DisableStateGenSigVerify. It was a safety net in case the state gen feature was not working properly.
if err != nil { | ||
return nil, err | ||
} | ||
state, err = processSlotsStateGen(ctx, state, targetSlot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
…o ff-cleanup-part6
Part of #7509
I removed the feature flags
disableStrictAttestationPubsubVerificationFlag
,enableStateGenSigVerify
,disableReduceAttesterStateCopy
,disableBatchBlockVerify
, anddisableDomainDataCacheFlag
. The rationale for removal are outlined in #7509These are grouped in PR for ease to merge and resolve conflicts