-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use slot to determine fork version #14653
Conversation
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.
Looks like a big improvement to avoid reading from the state. I am a bit concerned that there are no unit tests added or changed. Could you add some tests for this logic change?
httputil.HandleError(w, "Could not get head state: "+err.Error(), http.StatusInternalServerError) | ||
return | ||
v := slots.ToForkVersion(primitives.Slot(slot)) | ||
if slot == 0 { |
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.
shouldn't this be based on rawSlot instead of slot?
030b97b
to
bd59ada
Compare
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.
LGTM
* Use slot to determine version * gaz * solve cyclic dependency * Radek' review * unit test * gaz * use require instead of assert * fix test * fix test * fix TestGetAggregateAttestation * fix ListAttestations test * James' review * Radek' review * add extra checks to GetAttesterSlashingsV2 * fix matchingAtts * improve tests + fix * fix * stop appending all non electra atts * more tests * changelog * revert TestProduceSyncCommitteeContribution changes --------- Co-authored-by: Radosław Kapka <[email protected]> Co-authored-by: rkapka <[email protected]>
What type of PR is this?
Other
What does this PR do? Why is it needed?
This PR updates the logic to use the slot instead of the head state version to determine the fork version.
This approach reduces the risk of errors in cases where the block for the first slot of a new fork is delayed or not proposed, which could result in the head state still reflecting the previous fork.
Which issues(s) does this PR fix?
N/A
Other notes for review
Acknowledgements