Fix parent_beacon_block_root during proposer prep

[michaelsproul]

Issue Addressed

Fix a bug whereby Lighthouse would send the wrong parent_beacon_block_root in the payload attributes when preparing a payload.

We were sending the root of the parent of the canonical head block in all cases, which is wrong most of the time when we want to build upon the canonical head. In this case, we should send the root of the canonical head block itself (this is specced in EIP-4788). Using the parent root happened to be correct in the case of a single-slot re-org, although this was just a happy accident.

The bug only has low impact because when we actually request a payload we use the correct parent block root, here:

lighthouse/beacon_node/beacon_chain/src/execution_payload.rs

Lines 428 to 429 in 1360653

	let parent_beacon_block_root = match state {
	&BeaconState::Deneb(_) => Some(state.latest_block_header().canonical_root()),

So we get a payload ID cache miss almost every slot, and force the EL to build a payload last-minute off the new (correct) payload attributes.

Proposed changes

• Fix the issue.
• Add a test (TODO).

Additional Info

Thanks to @pinges for bringing this to our attention. Besu (correctly) assumed that the headBlockHash should determine the parentBeaconBlockRoot, and was failing to produce blocks with Lighthouse as a result. Besu merged a workaround (which is probably a good defensive measure) in hyperledger/besu#5843.

[jimmygchen]

Nice find, I reviewed the 4788 changes but missed this one!
Looks good to me 👍

[jimmygchen]

Nice find, I reviewed the 4788 changes but missed this one! Looks good to me 👍

Looks like we have the exact same logic in mock_builder, probably explains why I was getting block hash mismatch! 🎉

lighthouse/beacon_node/execution_layer/src/test_utils/mock_builder.rs

Lines 405 to 425 in 7d468cb

	let parent_root = head_state.latest_block_header().parent_root;
	let payload_attributes = match fork {
	ForkName::Merge => {
	PayloadAttributes::new(timestamp, *prev_randao, fee_recipient, None, None)
	}
	// the withdrawals root is filled in by operations
	ForkName::Capella => {
	PayloadAttributes::new(timestamp, *prev_randao, fee_recipient, Some(vec![]), None)
	}
	ForkName::Deneb => PayloadAttributes::new(
	timestamp,
	*prev_randao,
	fee_recipient,
	Some(vec![]),
	Some(parent_root),
	),
	ForkName::Base | ForkName::Altair => {
	return Err(MevError::InvalidFork);
	}
	};

[michaelsproul]

Ah, that's probably why the builder Deneb test is failing. Although weirdly it succeeds when I run it locally. I'll push that change tomorrow with a test

[realbigsean]

Ah, awesome! Was struggling to figure this one out yesterday. Thanks Michael!

[jimmygchen]

Ah, that's probably why the builder Deneb test is failing. Although weirdly it succeeds when I run it locally. I'll push that change tomorrow with a test

That failing test (builder_works_post_deneb) is a bit weird and flaky, I'm guessing it could be related to one of these issues I had when I added this test here.

• Use RUST_MIN_STACK=8388608: 4mb thread stack limit wasnt enough, looks like we really need to get @ethDreamer's Initial Attempt to Eliminate Blob Clones #4593 in.
• Increase get_header timeout: 1 second timeout wasn't enough when running in debug - I think we might even want to override this in tests via env vars, as it's likely that mock builder requires more time to return a response.

[jimmygchen]

I've re-reviewed the changes, looks good!

[jimmygchen]

Oh, there seems to be some issues with expected withdrawals amount mismatch in the failing tests.

[michaelsproul]

Yeah, looking into that now

[michaelsproul]

Ok, I reverted the check that I added to the execution block hash generator, as it wasn't correct in the presence of slashable blocks.

The check I added was trying to verify that all payload attributes with the same head_block_hash, fee_recipient and timestamp were identical. This isn't the case when there are two chains of slashable blocks, e.g. at slot 9 & 10. The block roots of the blocks at slot 9 are distinct, but they both blocks have the same execution payload (and therefore identical execution block hash). Then when building the 2nd block at slot 10, the fork choice update has the same block hash as the first slot 10 block, but with a different parent root.