fix: Parse blocks sent by builder with fork version.#63
fix: Parse blocks sent by builder with fork version.#63bharath-123 merged 9 commits intobharath/fulufrom
Conversation
| func (api *RelayAPI) getForkFromSlot(slot uint64) spec.DataVersion { | ||
| if api.isFulu(slot) { | ||
| return spec.DataVersionFulu | ||
| } else if api.isElectra(slot) { | ||
| return spec.DataVersionElectra | ||
| } else if api.isDeneb(slot) { | ||
| return spec.DataVersionDeneb | ||
| } else if api.isCapella(slot) { | ||
| return spec.DataVersionCapella | ||
| } | ||
| return spec.DataVersionPhase0 | ||
| } |
There was a problem hiding this comment.
This would be a little cleaner as a switch:
func (api *RelayAPI) getForkFromSlot(slot uint64) spec.DataVersion {
switch {
case api.isFulu(slot):
return spec.DataVersionFulu
case api.isElectra(slot):
return spec.DataVersionElectra
case api.isDeneb(slot):
return spec.DataVersionDeneb
case api.isCapella(slot):
return spec.DataVersionCapella
default:
return spec.DataVersionPhase0
}
}
services/api/service.go
Outdated
| if contentType == ApplicationOctetStream { | ||
| if contentType == "application/octet-stream" { |
| builderEthConsensusVersion := req.Header.Get(HeaderEthConsensusVersion) | ||
| if builderEthConsensusVersion == "" { | ||
| // don't reject a builder submission if the Eth-Consensus-Version header is not present | ||
| builderEthConsensusVersion = api.getForkFromSlot(latestSlot).String() | ||
| log.Warnf("builder did not send the Eth-Consensus-Version header, using latestSlot %d to infer the fork version: %s", latestSlot, builderEthConsensusVersion) | ||
| } |
There was a problem hiding this comment.
Instead of using the latest slot, I think we should try to use SubmitBlockRequest.message.slot (where message is BidTrace). This should solve this fork boundary issue; where a builder submits a Fulu block while latestSlot is still in Electra. This idea would require partial decoding, should be possible though.
There was a problem hiding this comment.
ya ideally latestSlot would never be in electra, since payload attributes ideally always reflects the latest slot. But partially decoding the payload could be much more accurate.
6e87b09 to
9a51a31
Compare
| // SSZ decoding failed. try JSON as fallback (some builders used octet-stream for json before) | ||
| if err2 := json.Unmarshal(requestPayloadBytes, payload); err2 != nil { | ||
| log.WithError(fmt.Errorf("%w / %w", err, err2)).Warn("could not decode payload - SSZ or JSON") | ||
| builderEthConsensusVersion := req.Header.Get(HeaderEthConsensusVersion) |
There was a problem hiding this comment.
I am contemplating whether we should even have this header since we already have the fallback logic?
There was a problem hiding this comment.
Yeah honestly might not even be necessary for submitNewBlock... Might prevent a bug though so I'd be in favor of keeping it. Eg a builder forgets to update their client and continues to build electra payloads in fulu.
|
Tests are failing because the new submitBlock json payloads which are added can't be found in the CI run for some reason? And the lint is failing due to some dynamic return types. will look into them. |
services/api/service.go
Outdated
| case api.isCapella(slot): | ||
| return spec.DataVersionCapella | ||
| default: | ||
| return spec.DataVersionPhase0 |
There was a problem hiding this comment.
I think I missed this before. This should be bellatrix.
b2fb3a0 to
d06635e
Compare
common/types_spec.go
Outdated
| } | ||
|
|
||
| func (r *VersionedSubmitBlockRequest) UnmarshalWithVersion(input []byte, contentType, ethConsensusVersion string) error { | ||
| if contentType == "application/octet-stream" { |
There was a problem hiding this comment.
i think it will be better to replace it with ApplicationOctetStream variable? We can move it from service to constants inside the common package so it resolves in other places as well
services/api/utils.go
Outdated
| } | ||
|
|
||
| func getSlotFromBuilderSSZPayload(input []byte) (uint64, error) { | ||
| if len(input) < 4 { |
There was a problem hiding this comment.
shouldn't this be if len(input) < 8 since it might cause a panic when accessing input[4:8]
There was a problem hiding this comment.
ya you are right. the check should be there.
|
|
||
| messageOffset := binary.LittleEndian.Uint32(input[4:8]) | ||
|
|
||
| slot := binary.LittleEndian.Uint64(input[messageOffset : messageOffset+8]) |
There was a problem hiding this comment.
probably also need to add a check here before accessing
if messageOffset + 8 > len(input) throw error
services/api/service.go
Outdated
| return | ||
| slot, err := getSlotFromBuilderJSONPayload(requestPayloadBytes) | ||
| if err != nil { | ||
| log.WithError(err).Warn("could not get slot from builder ssz payload") |
There was a problem hiding this comment.
These error messages should be swapped right?
There was a problem hiding this comment.
good catch! will do it
jtraglia
left a comment
There was a problem hiding this comment.
LGTM but you need to fix the CI checks.
064e0ec to
a2ad020
Compare

📝 Summary
Make parsing of blocks sent via
SubmitNewBlockstoVersionedSubmitBlockRequestdependent on fork version.⛱ Motivation and Context
When receiving blocks from the builder via
SubmitNewBlocksAPI, we parse the block to its corresponding fork specificVersionedSubmitBlockRequest. With the Fusaka hard fork, theVersionedSubmitBlockRequestfor Fulu and Electra are the same since the only change is the addition of cell_proofs which are just extended blob proofs. The current way of unmarshalling the payload unmarshalls an ElectraVersionedSubmitBlockRequestto a Fulu one.To avoid such incidents, we want to parse the incoming block specifc to the fork.
The builder doesn't send the
Eth-Consensus-Versionlike the proposers do, so we need to infer the fork version to parse theVersionedSubmitBlockRequest. To do this, we parse the value of the slot from the raw payload bytes. Based on the slot, we parse the payload according to the corresponding fork version.✅ I have run these commands
make lintmake test-racego mod tidyCONTRIBUTING.md