hive: fcu fields not validated

[rkrasiuk]

Description

The chain formed by forkchoice update fields (finalized -> safe -> head) is not validated by the consensus engine and it erroneously responds with FCU status Syncing

Hive

At the time of writing the test covering this behavior is named Inconsistent Head in ForkchoiceState (Paris) under engine-api test suite.

Test breakdown

The simulation sends following Engine API requests to the client:

FCU head: 0xd462b6793c2895fd61cd63e098874774d3e03556e14ec40fb9861956e39eb8b0 - Valid

New Payload: 0xab4eb0d9f2a392ccf1edf43ffd0c9d6a197d1215a94f7294d032e962812a175b (parent: 0xd462b6793c2895fd61cd63e098874774d3e03556e14ec40fb9861956e39eb8b0) - Valid

New Payload: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 (parent: 0xd462b6793c2895fd61cd63e098874774d3e03556e14ec40fb9861956e39eb8b0) - Valid

FCU head: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 - Valid

FCU head: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 - Valid

New Payload: 0xb307f97d770d75d8f61e996162a063cbe3ed3c6a9b8b2392e237c3364225a585 (parent: 0xab4eb0d9f2a392ccf1edf43ffd0c9d6a197d1215a94f7294d032e962812a175b) - Valid

New Payload: 0x444680403ae5c7e1d3eb67d2e7d77bdf6492bdf0f108cca77f7d9097dfd97652 (parent: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7) - Valid

FCU head: 0x444680403ae5c7e1d3eb67d2e7d77bdf6492bdf0f108cca77f7d9097dfd97652 safe: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 - Valid

FCU head: 0x444680403ae5c7e1d3eb67d2e7d77bdf6492bdf0f108cca77f7d9097dfd97652 safe: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 - Valid

New Payload: 0xcd8924ad6d3e6075f7548297a35a46a85e384b3520ed1abdf1c168cae381ac71 (parent: 0xb307f97d770d75d8f61e996162a063cbe3ed3c6a9b8b2392e237c3364225a585) - Valid

New Payload: 0xd6d53c45ebebd35fe4abaf030eeaec52e830a328e0cc0d15b93a9fe687f408de (parent: 0x444680403ae5c7e1d3eb67d2e7d77bdf6492bdf0f108cca77f7d9097dfd97652) - Valid

FCU head: 0xd6d53c45ebebd35fe4abaf030eeaec52e830a328e0cc0d15b93a9fe687f408de safe: 0x444680403ae5c7e1d3eb67d2e7d77bdf6492bdf0f108cca77f7d9097dfd97652 finalized: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 - Valid

FCU head: 0xcd8924ad6d3e6075f7548297a35a46a85e384b3520ed1abdf1c168cae381ac71 safe: 0x444680403ae5c7e1d3eb67d2e7d77bdf6492bdf0f108cca77f7d9097dfd97652 finalized: 0x2e656d340bf43036bf48c6fd966497414ee79d8cf6c3bc86c15c47ed907e51f7 - Syncing (wrong)

After analyzing the chains formed by these blocks, we can see that head 0xcd89..ac71 can never extend chain formed by 0x4446..7652 and 0x2e65..51f7.

Solution

After validating the existence of the head block in the blockchain tree, we should validate that it forms a valid chain by extending safe and finalized blocks.

[Rjected]

To add some more context here, we do perform a similar check here

reth/crates/consensus/beacon/src/engine/mod.rs

Lines 827 to 868 in 720815a

	/// Ensures that the given forkchoice state is consistent, assuming the head block has been
	/// made canonical.
	///
	/// If the forkchoice state is consistent, this will return Ok(None). Otherwise, this will
	/// return an instance of [OnForkChoiceUpdated] that is INVALID.
	///
	/// This also updates the safe and finalized blocks in the [CanonChainTracker], if they are
	/// consistent with the head block.
	fn ensure_consistent_state(
	&mut self,
	state: ForkchoiceState,
	) -> RethResult<Option<OnForkChoiceUpdated>> {
	// Ensure that the finalized block, if not zero, is known and in the canonical chain
	// after the head block is canonicalized.
	//
	// This ensures that the finalized block is consistent with the head block, i.e. the
	// finalized block is an ancestor of the head block.
	if !state.finalized_block_hash.is_zero() &&
	!self.blockchain.is_canonical(state.finalized_block_hash)?
	{
	return Ok(Some(OnForkChoiceUpdated::invalid_state()))
	}
	// Finalized block is consistent, so update it in the canon chain tracker.
	self.update_finalized_block(state.finalized_block_hash)?;
	// Also ensure that the safe block, if not zero, is known and in the canonical chain
	// after the head block is canonicalized.
	//
	// This ensures that the safe block is consistent with the head block, i.e. the safe
	// block is an ancestor of the head block.
	if !state.safe_block_hash.is_zero() &&
	!self.blockchain.is_canonical(state.safe_block_hash)?
	{
	return Ok(Some(OnForkChoiceUpdated::invalid_state()))
	}
	// Safe block is consistent, so update it in the canon chain tracker.
	self.update_safe_block(state.safe_block_hash)?;
	Ok(None)
	}

This happens after make_canonical and the check is only performed if make_canonical returns a VALID response. Given that cd89 and all its ancestors should exist in the blockchain tree, SYNCING is definitely the wrong response here

[github-actions]

This issue is stale because it has been open for 21 days with no activity.

[mattsse]

iirc merge tests were retired