eth/catalyst: add validation error in new paylaod hash mismatch #28226
Conversation
eth/catalyst/api.go
Outdated
| @@ -513,7 +513,8 @@ func (api *ConsensusAPI) newPayload(params engine.ExecutableData, versionedHashe | |||
| block, err := engine.ExecutableDataToBlock(params, versionedHashes, beaconRoot) | |||
| if err != nil { | |||
| log.Warn("Invalid NewPayload params", "params", params, "error", err) | |||
| return engine.PayloadStatusV1{Status: engine.INVALID}, nil | |||
| errMsg := err.Error() | |||
| return engine.PayloadStatusV1{Status: engine.INVALID, LatestValidHash: nil, ValidationError: &errMsg}, nil | |||
There was a problem hiding this comment.
Why not return api.invalid(err, nil), nil ?
There was a problem hiding this comment.
This would set LatestValidHash non-nil. Currently the engine API specifies that if the block hash verification fails, the EL must return nil. See point 5 here: https://github.com/ethereum/execution-apis/blob/main/src/engine/paris.md#specification.
--
With that said, I don't really understand the purpose of returning nil when we actually know the latest valid hash.
There was a problem hiding this comment.
@karalabe I tried refactoring api.invalid(..) a bit so that it behaves as you requested. It doesn't seem like we are using the functionality where you pass in nil to invalid(..) and it actually grabs the head of the chain. Let me know if you prefer this or the original way.
There was a problem hiding this comment.
What's teh purpose of returning nil for latest valid? Shouldn't it be... you know... the "latest valid"?
There was a problem hiding this comment.
I was also wondering this. But I'm not sure what the thinking there that caused it to be written into the engine api like it is.
There was a problem hiding this comment.
Could we perhaps have a clarification and maybe fix the spec if it's wrong?
There was a problem hiding this comment.
I looked a bit more into this and it seems to have come from the SYNCING response. latestValidHash is supposed to return the latest valid hash from the ancestors of the provided payload. If the payload references an unknown parent, of course we must return nil since we don't know the ancestor chain.
The block hash check was added (I think) to protect the CL while the EL is syncing. So it would make sense to simply return nil for latest valid hash. I guess the case of a new payload request while not syncing wasn't as closely considered and so it also returns nil even when we can return the latest valid block hash.
… hash if none provided
…reum#28226) * eth/catalyst: add validation error in new paylaod hash mismatch * eth/catalyst/api: refactor api.invalid(..) to return nil latest valid hash if none provided
…reum#28226) * eth/catalyst: add validation error in new paylaod hash mismatch * eth/catalyst/api: refactor api.invalid(..) to return nil latest valid hash if none provided
…reum#28226) * eth/catalyst: add validation error in new paylaod hash mismatch * eth/catalyst/api: refactor api.invalid(..) to return nil latest valid hash if none provided
…reum#28226) * eth/catalyst: add validation error in new paylaod hash mismatch * eth/catalyst/api: refactor api.invalid(..) to return nil latest valid hash if none provided
…ch (ethereum#28226)" This reverts commit aeff118.
…ch (ethereum#28226)" This reverts commit aeff118.
Small change which now returns the error message from
ExecutableDataToBlock(..).