fix: /eth/v2/beacon/blocks post api to handle electra and fulu blocks correctly #14897
Conversation
james-prysm
added
Bug
| @@ -723,7 +723,27 @@ func (s *Server) PublishBlockV2(w http.ResponseWriter, r *http.Request) { | |||
| s.publishBlock(ctx, w, r, true) | |||
| } | |||
| } | |||
| func (s *Server) versionHeaderFromCurrentSlot() string { | |||
| // attempt to get the current fork information | |||
| ce := slots.ToEpoch(s.GenesisTimeFetcher.CurrentSlot()) | |||
There was a problem hiding this comment.
what do you guys think about this?
There was a problem hiding this comment.
If you want current fork information, then a slightly "cleaner" way is to use CurrentFork() from the blockchain service. There is one problem when doing it either way, but it's an edge case during the fork and clients should use v2 anyway. So I don't worry about it.
There was a problem hiding this comment.
I won't change it then 😅
| @@ -723,7 +723,27 @@ func (s *Server) PublishBlockV2(w http.ResponseWriter, r *http.Request) { | |||
| s.publishBlock(ctx, w, r, true) | |||
| } | |||
| } | |||
| func (s *Server) versionHeaderFromCurrentSlot() string { | |||
There was a problem hiding this comment.
Rename to versionHeaderFromCurrentFork in case you decide to use CurrentFork()
Co-authored-by: Radosław Kapka <[email protected]>
Co-authored-by: Radosław Kapka <[email protected]>
Co-authored-by: Radosław Kapka <[email protected]>
|
|
||
| // nolint:gocognit |
There was a problem hiding this comment.
Please do not ignore the linter. This function should be refactored to reduce complexity rather than suppressing the linter.
If you need to ignore, can you state a good cause for doing so?
There was a problem hiding this comment.
we should refactor it, I just wanted to get this pr out with the least amount of changes for the perceived urgency, we should refactor this to clean it up but I was afraid of introducing more changes with that. I need to make sure the errors are being returned correctly and exiting the function if wrapped into new functions.
if you feel I must handle it in this PR then I'll shift focus to this .
There was a problem hiding this comment.
If you want to follow up immediately after this PR, that works for me. I'm just cautious of people ignoring the linter and never fixing it
| @@ -735,9 +755,21 @@ func (s *Server) publishBlockSSZ(ctx context.Context, w http.ResponseWriter, r * | |||
| httputil.HandleError(w, api.VersionHeader+" header is required", http.StatusBadRequest) | |||
| return | |||
| } | |||
| if !versionRequired && versionHeader == "" { | |||
| versionHeader = s.versionHeaderFromCurrentSlot() | |||
There was a problem hiding this comment.
While the current slot should definitionally be equal to the slot of the published block, wouldn't it be ideal to base this on the slot of the block itself?
If the posted block is json, we can unmarshal into a narrow type just to read the slot. Then pick the version based on the block's slot.
type SlotPeeker struct {
Block struct {
Slot primitives.Slot `json:"slot"`
} `json:"message"`
}
p := &SlotPeeker{}
if err := json.Unmarshal(body, p) {
...
}
If the posted block is ssz, it can be unmarshaled to the correct type using https://github.com/prysmaticlabs/prysm/blob/develop/encoding/ssz/detect/configfork.go#L53
There was a problem hiding this comment.
I was thinking about this as well but for deneb and up it's not just a block but block contents so it's another wrapper. Do you suggest trying to unmarshal the subset to fit the block contents type and if that fails try to do the subset of the block? The whole reason for this fork thing is to support this deprecated endpoint v1, in v2 this versioned head is required so we always know how to unmarshall
There was a problem hiding this comment.
Yeah sure, add another wrapper for "block contents" which specifies the block field as json.RawMessage. If the deneb wrapper type is published, you'll have len() > 0 for that field; then unmarshal the json.RawMessage value into the SlotPeeker. If the field has len() == 0, try to unmarshal the original body into the SlotPeeker.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
The following error was reported by Chris Berry on the vouch team on POST request endpoint
/eth/v2/beacon/blocksthis error is caused by how we have handled blocks in our publish block function. a new fork block type ( FULU) was added which has the same structure as the electra block type, the existing code believed the block was a fulu type.
This fix handles the API in 2 different ways.
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements