Skip to content
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.

Sign up for GitHub

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

Jump to bottom

eth/catalyst: add validation error in new paylaod hash mismatch #28226

Merged
merged 2 commits into from
Sep 29, 2023
Merged

eth/catalyst: add validation error in new paylaod hash mismatch #28226

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b718d5
eth/catalyst: add validation error in new paylaod hash mismatch
lightclient Sep 25, 2023
144fed4
eth/catalyst/api: refactor api.invalid(..) to return nil latest valid…
lightclient Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion eth/catalyst/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@karalabe karalabe Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return api.invalid(err, nil), nil ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's teh purpose of returning nil for latest valid? Shouldn't it be... you know... the "latest valid"?

Copy link
Member Author

@lightclient lightclient Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps have a clarification and maybe fix the spec if it's wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
// Stash away the last update to warn the user if the beacon client goes offline
api.lastNewPayloadLock.Lock()
Expand Down