Fix withdrawals check

[somnathb1]

No description provided.

[yperbasis]

What was the problem with the previous logic?

[somnathb1]

What was the problem with the previous logic?

The logic was to set the withdrawals from req for capella and up. No need to do the checks if withdrawals aren't expected, as it would be nil anyways.
This was hitting a bump with Teku during gnosis devnet, for some reason it was sending newPayloadV1 and getting stuck with "no withdrawals".

[yperbasis]

What was the problem with the previous logic?

The logic was to set the withdrawals from req for capella and up. No need to do the checks if withdrawals aren't expected, as it would be nil anyways. This was hitting a bump with Teku during gnosis devnet, for some reason it was sending newPayloadV1 and getting stuck with "no withdrawals".

Hmm. The change doesn't look right to me. If header.time >= shanghai and CL is sending us newPayloadV1 instead of newPayloadV3, that's a bug on the CL side and we shouldn't accept that. See https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#update-the-methods-of-previous-forks

[somnathb1]

What was the problem with the previous logic?

The logic was to set the withdrawals from req for capella and up. No need to do the checks if withdrawals aren't expected, as it would be nil anyways. This was hitting a bump with Teku during gnosis devnet, for some reason it was sending newPayloadV1 and getting stuck with "no withdrawals".

Hmm. The change doesn't look right to me. If header.time >= shanghai and CL is sending us newPayloadV1 instead of newPayloadV3, that's a bug on the CL side and we shouldn't accept that. See https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#update-the-methods-of-previous-forks

Either way, there is no need for checking withdrawals presence without setting it first. And it was only setting it for capella+, then checking for nil on that, then checking again.

This change is not a logical change to handle newPayloadV1 against the specs, but simply to avoid 2 more unnecessary if conditions, with the rest of the logic as is.

[yperbasis]

Either way, there is no need for checking withdrawals presence without setting it first. And it was only setting it for capella+, then checking for nil on that, then checking again.

This change is not a logical change to handle newPayloadV1 against the specs, but simply to avoid 2 more unnecessary if conditions, with the rest of the logic as is.

OK. So with this change UnsupportedForkError rather than InvalidParamsError will be returned if newPayloadV1 is used after Dencun, right? To be honest I'm not sure which is better/more conformant with the spec in that case. But theoretically with your change we're now accepting the case when newPayloadV1 is used after Shapella but before Dencun, which is bad.

[somnathb1]

You are right, and

Either way, there is no need for checking withdrawals presence without setting it first. And it was only setting it for capella+, then checking for nil on that, then checking again.
This change is not a logical change to handle newPayloadV1 against the specs, but simply to avoid 2 more unnecessary if conditions, with the rest of the logic as is.

OK. So with this change UnsupportedForkError rather than InvalidParamsError will be returned if newPayloadV1 is used after Dencun, right? To be honest I'm not sure which is better/more conformant with the spec in that case. But theoretically with your change we're now accepting the case when newPayloadV1 is used after Shapella but before Dencun, which is bad.

You are right, and there is some refactoring needed in the existing code. I don't understand why checkWithdrawalsPresence with nil withdrawals for less than capella