Fix deserialization of Electra JSON responses from builders

[michaelsproul]

I think our implementation of ForkVersionDeserialize is wrong:

lighthouse/common/eth2/src/types.rs

Lines 2014 to 2033 in 091e292

	impl<E: EthSpec> ForkVersionDeserialize for FullPayloadContents<E> {
	fn deserialize_by_fork<'de, D: Deserializer<'de>>(
	value: Value,
	fork_name: ForkName,
	) -> Result<Self, D::Error> {
	if fork_name.deneb_enabled() {
	serde_json::from_value(value)
	.map(Self::PayloadAndBlobs)
	.map_err(serde::de::Error::custom)
	} else if fork_name.bellatrix_enabled() {
	serde_json::from_value(value)
	.map(Self::Payload)
	.map_err(serde::de::Error::custom)
	} else {
	Err(serde::de::Error::custom(format!(
	"FullPayloadContents deserialization for {fork_name} not implemented"
	)))
	}
	}
	}

We are relying on serde_json::from_value(value) to produce a ExecutionPayloadAndBlobs<E> without any fork hinting, so it is liable to deserialize the wrong variant (e.g. Deneb instead of Electra).

I think we should remove the Deserialize impl for ExecutionPayloadAndBlobs<E>, implement ForkVersionDeserialize instead, and use that fork-aware deserialization here.

[michaelsproul]

Fix should be targeted at the release-v7.0.0 branch.

[eserilev]

im on it

[pawanjay176]

Sorry for screwing this up.
In my testing with the mock builder, the builder eventually ends up publishing the correct block and so we don't see issues with missed slots (although the logs show the same error).
Should probably make the default behaviour to not publish the payload in the mock builder so that we can catch publishing bugs.

Thanks for testing and raising the issue @will-luban

[michaelsproul]

No worries @pawanjay176, this one was pretty easy to miss. I think the new tests and the refactor proposed here should help prevent issues with new forks:

• Refactor ForkVersionedResponse and ForkVersionDeserialize #7286

[michaelsproul]

Closed by:

• Fix builder API electra json response #7285