Unify Engine API failure responses

[rubo]

Changes

Implemented ethereum/execution-apis#337 that unifies Engine API failure responses.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a fix or a feature that would cause existing functionality not to work as expected)
• Documentation update
• Code style update (formatting, renaming)
• Refactoring (no functional or API changes)
• Build-related changes
• Other: description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Withdrawals Hive tests passing.

[LukaszRozmej]

Maybe we should do class hierarchy for ExecutionPayload?
Or at least a clear way of validating them, for example, extension methods like: IsValidV1, IsValidV2 or GetVersion.

[rubo]

Maybe we should do class hierarchy for ExecutionPayload?
Or at least a clear way of validating them, for example, extension methods like: IsValidV1, IsValidV2 or GetVersion.

That was exactly the way I went initially but it turned out to be a longer and more complicated approach. Class hierarchy won't work in this case either. The only way to distinguish versions, for now, is to check whether the Withdrawals property is null based on the timestamp and block number combination.
Given this check is only needed at the very beginning of this particular method, moving it to an extension method doesn't bring any value here. If we're going to reuse it somewhere else as well, then it would make sense, indeed.

Also, because this check is an input parameter validation, I'd like to have it before any other checks for FCU as well. However, the timestamp and block number logic are a bit complicated there, so I left it there.

Things may change with future version checks though.

[LukaszRozmej]

Maybe we should do class hierarchy for ExecutionPayload?
Or at least a clear way of validating them, for example, extension methods like: IsValidV1, IsValidV2 or GetVersion.

That was exactly the way I went initially but it turned out to be a longer and more complicated approach. Class hierarchy won't work in this case either. The only way to distinguish versions, for now, is to check whether the Withdrawals property is null based on the timestamp and block number combination. Given this check is only needed at the very beginning of this particular method, moving it to an extension method doesn't bring any value here. If we're going to reuse it somewhere else as well, then it would make sense, indeed.

Also, because this check is an input parameter validation, I'd like to have it before any other checks for FCU as well. However, the timestamp and block number logic are a bit complicated there, so I left it there.

Things may change with future version checks though.

My whole point is on semantics and readability lets name it V1/V2 and make validation based on that. Will be easier to read and translate from specs, let me try to provide example

[rubo]

My whole point is on semantics and readability lets name it V1/V2 and make validation based on that. Will be easier to read and translate from specs, let me try to provide example

If we go with versions, then we will have engine_newPayloadV2(ExecutionPayloadV2). So, the JSON always will deserialize to V2. Then we need to check whether this V2 is really a V2 or a V1 based on timestamp and withdrawals presence. Thus, no difference.

[LukaszRozmej]

created versioning PR: #5157