engine_forkchoiceUpdatedV1/V2/V3 incorrectly only check for correct engine API call version when payload attributes present

[tersec]

It does these checks in validateVersion, but this is only called when attrsOpt.isSome:

nimbus-eth1/nimbus/beacon/api_handler/api_forkchoice.nim

Lines 188 to 193 in bda760f

	# If payload generation was requested, create a new block to be potentially
	# sealed by the beacon client. The payload will be requested later, and we
	# might replace it arbitrarilly many times in between.
	if attrsOpt.isSome:
	let attrs = attrsOpt.get()
	validateVersion(attrs, com, apiVersion)

whereas https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.4/src/engine/shanghai.md#specification-1 requires:

2. Consensus layer client MUST call this method instead of engine_forkchoiceUpdatedV1 under any of the following conditions:
   • headBlockHash references a block which timestamp is greater or equal to the Shanghai timestamp,
   • payloadAttributes is not null and payloadAttributes.timestamp is greater or equal to the Shanghai timestamp.

There's no check for the headBlockHash block timestamp mismatching the fork case.

[jangko]

nimbus-eth1/nimbus/beacon/api_handler/api_forkchoice.nim

Line 138 in cfbbcda

chain.setCanonical(header).isOkOr:

nimbus-eth1/nimbus/beacon/api_handler/api_forkchoice.nim

Line 195 in cfbbcda

let bundle = ben.generatePayload(attrs).valueOr:

Both of the above calls implicitly validate the timestamp of the related block inside their block validation routine.

[tersec]

nimbus-eth1/nimbus/beacon/api_handler/api_forkchoice.nim

Line 138 in cfbbcda

chain.setCanonical(header).isOkOr:

nimbus-eth1/nimbus/beacon/api_handler/api_forkchoice.nim

Line 195 in cfbbcda

let bundle = ben.generatePayload(attrs).valueOr:

Both of the above calls implicitly validate the timestamp of the related block inside their block validation routine.

Line 195 is also within the payload attributes-only section of the function:

nimbus-eth1/nimbus/beacon/api_handler/api_forkchoice.nim

Lines 188 to 197 in cfbbcda

	# If payload generation was requested, create a new block to be potentially
	# sealed by the beacon client. The payload will be requested later, and we
	# might replace it arbitrarilly many times in between.
	if attrsOpt.isSome:
	let attrs = attrsOpt.get()
	validateVersion(attrs, com, apiVersion)
	let bundle = ben.generatePayload(attrs).valueOr:
	error "Failed to create sealing payload", err = error
	raise invalidAttr(error)

so it doesn't address the no-payload-attributes case.

I don't see how setCanonical can make this decision, because it doesn't know the apiVersion of the fcU call. It's possible, e.g., on a hardfork boundary, for a CL to validly send oscillating fcUs of

• fcUV2(Shapella block S in one fork, null payload attributes)
• fcUV3(Dencun block D in another fork, null payload attributes)
• fcUV2(Shapella block S in one fork, null payload attributes)
• fcUV3(Dencun block D in another fork, null payload attributes)
  etc in a loop.

This would typically indicate, of course, a CL bug, but the engine API behavior per se is not incorrect.

Invalid engine API behavior which would look identical to setCanonical:

• fcUV3(Shapella block S in one fork, null payload attributes)
• fcUV2(Dencun block D in another fork, null payload attributes)
• fcUV3(Shapella block S in one fork, null payload attributes)
• fcUV2(Dencun block D in another fork, null payload attributes)

(or any other non-matching fcUV2 vs fcUV3 vs Shapella vs Dencun block, but that's one arbitrary example)

How can setCanonical flag this?