fix: perform forkchoice update consistency checks (#3730)

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
This commit is contained in:
Dan Cline
2023-07-12 10:12:33 -04:00
committed by GitHub
parent 6799fc3600
commit 99240906a8
4 changed files with 119 additions and 5 deletions

View File

@@ -158,6 +158,11 @@ impl<DB: Database, C: Consensus, EF: ExecutorFactory> BlockchainTreeViewer
self.tree.read().block_indices().canonical_tip()
}
fn is_canonical(&self, hash: BlockHash) -> Result<bool, Error> {
trace!(target: "blockchain_tree", ?hash, "Checking if block is canonical");
self.tree.read().is_block_hash_canonical(&hash)
}
fn pending_blocks(&self) -> (BlockNumber, Vec<BlockHash>) {
trace!(target: "blockchain_tree", "Returning all pending blocks");
self.tree.read().block_indices().pending_blocks()

View File

@@ -631,12 +631,20 @@ where
debug!(target: "consensus::engine", hash=?state.head_block_hash, number=outcome.header().number, "canonicalized new head");
// new VALID update that moved the canonical chain forward
let _ = self.update_canon_chain(outcome.header().clone(), &state);
let _ = self.update_head(outcome.header().clone());
} else {
debug!(target: "consensus::engine", fcu_head_num=?outcome.header().number, current_head_num=?self.blockchain.canonical_tip().number, "Ignoring beacon update to old head");
}
if let Some(attrs) = attrs {
// if we return early then we wouldn't perform these consistency checks, so we
// need to do them here, and should do them before we process any payload
// attributes
if let Some(invalid_fcu_response) = self.ensure_consistent_state(state)? {
trace!(target: "consensus::engine", ?state, head=?state.head_block_hash, "Forkchoice state is inconsistent, returning invalid response");
return Ok(invalid_fcu_response)
}
// the CL requested to build a new payload on top of this new VALID head
let payload_response = self.process_payload_attributes(
attrs,
@@ -662,22 +670,119 @@ where
}
};
if let Some(invalid_fcu_response) =
self.ensure_consistent_state_with_status(state, &status)?
{
trace!(target: "consensus::engine", ?status, ?state, "Forkchoice state is inconsistent, returning invalid response");
return Ok(invalid_fcu_response)
}
trace!(target: "consensus::engine", ?status, ?state, "Returning forkchoice status");
Ok(OnForkChoiceUpdated::valid(status))
}
/// Ensures that the given forkchoice state is consistent, assuming the head block has been
/// made canonical. This takes a status as input, and will only perform consistency checks if
/// the input status is VALID.
///
/// 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_with_status(
&mut self,
state: ForkchoiceState,
status: &PayloadStatus,
) -> Result<Option<OnForkChoiceUpdated>, reth_interfaces::Error> {
// We only perform consistency checks if the status is VALID because if the status is
// INVALID, we want to return the correct _type_ of error to the CL so we can properly
// describe the reason it is invalid. For example, it's possible that the status is invalid
// because the safe block has an invalid state root. In that case, we want to preserve the
// correct `latestValidHash`, instead of returning a generic "invalid state" error that
// does not contain a `latestValidHash`.
//
// We also should not perform these checks if the status is SYNCING, because in that case
// we likely do not have the finalized or safe blocks, and would return an incorrect
// INVALID status instead.
if status.is_valid() {
return self.ensure_consistent_state(state)
}
Ok(None)
}
/// 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,
) -> Result<Option<OnForkChoiceUpdated>, reth_interfaces::Error> {
// 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)
}
/// Sets the state of the canon chain tracker based to the given head.
///
/// This expects the given head to be the new canonical head.
///
/// Additionally, updates the head used for p2p handshakes.
///
/// This should be called before issuing a VALID forkchoice update.
/// This also updates the tracked safe and finalized blocks, and should be called before
/// returning a VALID forkchoice update response
fn update_canon_chain(
&self,
head: SealedHeader,
update: &ForkchoiceState,
) -> Result<(), Error> {
self.update_head(head)?;
self.update_finalized_block(update.finalized_block_hash)?;
self.update_safe_block(update.safe_block_hash)?;
Ok(())
}
/// Updates the state of the canon chain tracker based on the given head.
///
/// This expects the given head to be the new canonical head.
/// Additionally, updates the head used for p2p handshakes.
///
/// This should be called before returning a VALID forkchoice update response
#[inline]
fn update_head(&self, head: SealedHeader) -> Result<(), reth_interfaces::Error> {
let mut head_block = Head {
number: head.number,
hash: head.hash,
@@ -690,9 +795,6 @@ where
// we update the the tracked header first
self.blockchain.set_canonical_head(head);
self.update_finalized_block(update.finalized_block_hash)?;
self.update_safe_block(update.safe_block_hash)?;
head_block.total_difficulty =
self.blockchain.header_td_by_number(head_block.number)?.ok_or_else(|| {
Error::Provider(ProviderError::TotalDifficultyNotFound {

View File

@@ -217,6 +217,9 @@ pub trait BlockchainTreeViewer: Send + Sync {
/// Note: this could be the given `parent_hash` if it's already canonical.
fn find_canonical_ancestor(&self, parent_hash: BlockHash) -> Option<BlockHash>;
/// Return whether or not the block is known and in the canonical chain.
fn is_canonical(&self, hash: BlockHash) -> Result<bool, Error>;
/// Given the hash of a block, this checks the buffered blocks for the lowest ancestor in the
/// buffer.
///

View File

@@ -655,6 +655,10 @@ where
self.tree.canonical_tip()
}
fn is_canonical(&self, hash: BlockHash) -> std::result::Result<bool, Error> {
self.tree.is_canonical(hash)
}
fn pending_blocks(&self) -> (BlockNumber, Vec<BlockHash>) {
self.tree.pending_blocks()
}