Fix edge-case when finding the finalized descendant (#3924)

## Issue Addressed

NA

## Description

We were missing an edge case when checking to see if a block is a descendant of the finalized checkpoint. This edge case is described for one of the tests in this PR:

a119edc739/consensus/proto_array/src/proto_array_fork_choice.rs (L1018-L1047)

This bug presented itself in the following mainnet log:

```
Jan 26 15:12:42.841 ERRO Unable to validate attestation error: MissingBeaconState(0x7c30cb80ec3d4ec624133abfa70e4c6cfecfca456bfbbbff3393e14e5b20bf25), peer_id: 16Uiu2HAm8RPRciXJYtYc5c3qtCRdrZwkHn2BXN3XP1nSi1gxHYit, type: "unaggregated", slot: Slot(5660161), beacon_block_root: 0x4a45e59da7cb9487f4836c83bdd1b741b4f31c67010c7ae343fa6771b3330489
```

Here the BN is rejecting an attestation because of a "missing beacon state". Whilst it was correct to reject the attestation, it should have rejected it because it attests to a block that conflicts with finality rather than claiming that the database is inconsistent.

The block that this attestation points to (`0x4a45`) is block `C` in the above diagram. It is a non-canonical block in the first slot of an epoch that conflicts with the finalized checkpoint. Due to our lazy pruning of proto array, `0x4a45` was still present in proto-array. Our missed edge-case in [`ForkChoice::is_descendant_of_finalized`](38514c07f2/consensus/fork_choice/src/fork_choice.rs (L1375-L1379)) would have indicated to us that the block is a descendant of the finalized block. Therefore, we would have accepted the attestation thinking that it attests to a descendant of the finalized *checkpoint*.

Since we didn't have the shuffling for this erroneously processed block, we attempted to read its state from the database. This failed because we prune states from the database by keeping track of the tips of the chain and iterating back until we find a finalized block. This would have deleted `C` from the database, hence the `MissingBeaconState` error.
This commit is contained in:
Paul Hauner
2023-02-09 23:51:18 +00:00
parent 2b735a9e8b
commit 5276dd0cb0
6 changed files with 276 additions and 20 deletions

View File

@@ -451,7 +451,7 @@ impl ProtoArray {
/// Invalidate zero or more blocks, as specified by the `InvalidationOperation`.
///
/// See the documentation of `InvalidationOperation` for usage.
pub fn propagate_execution_payload_invalidation(
pub fn propagate_execution_payload_invalidation<E: EthSpec>(
&mut self,
op: &InvalidationOperation,
) -> Result<(), Error> {
@@ -482,7 +482,7 @@ impl ProtoArray {
let latest_valid_ancestor_is_descendant =
latest_valid_ancestor_root.map_or(false, |ancestor_root| {
self.is_descendant(ancestor_root, head_block_root)
&& self.is_descendant(self.finalized_checkpoint.root, ancestor_root)
&& self.is_finalized_checkpoint_or_descendant::<E>(ancestor_root)
});
// Collect all *ancestors* which were declared invalid since they reside between the
@@ -977,6 +977,12 @@ impl ProtoArray {
/// ## Notes
///
/// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`.
///
/// ## Warning
///
/// Do not use this function to check if a block is a descendant of the
/// finalized checkpoint. Use `Self::is_finalized_checkpoint_or_descendant`
/// instead.
pub fn is_descendant(&self, ancestor_root: Hash256, descendant_root: Hash256) -> bool {
self.indices
.get(&ancestor_root)
@@ -990,6 +996,70 @@ impl ProtoArray {
.unwrap_or(false)
}
/// Returns `true` if `root` is equal to or a descendant of
/// `self.finalized_checkpoint`.
///
/// Notably, this function is checking ancestory of the finalized
/// *checkpoint* not the finalized *block*.
pub fn is_finalized_checkpoint_or_descendant<E: EthSpec>(&self, root: Hash256) -> bool {
let finalized_root = self.finalized_checkpoint.root;
let finalized_slot = self
.finalized_checkpoint
.epoch
.start_slot(E::slots_per_epoch());
let mut node = if let Some(node) = self
.indices
.get(&root)
.and_then(|index| self.nodes.get(*index))
{
node
} else {
// An unknown root is not a finalized descendant. This line can only
// be reached if the user supplies a root that is not known to fork
// choice.
return false;
};
// The finalized and justified checkpoints represent a list of known
// ancestors of `node` that are likely to coincide with the store's
// finalized checkpoint.
//
// Run this check once, outside of the loop rather than inside the loop.
// If the conditions don't match for this node then they're unlikely to
// start matching for its ancestors.
for checkpoint in &[
node.finalized_checkpoint,
node.justified_checkpoint,
node.unrealized_finalized_checkpoint,
node.unrealized_justified_checkpoint,
] {
if checkpoint.map_or(false, |cp| cp == self.finalized_checkpoint) {
return true;
}
}
loop {
// If `node` is less than or equal to the finalized slot then `node`
// must be the finalized block.
if node.slot <= finalized_slot {
return node.root == finalized_root;
}
// Since `node` is from a higher slot that the finalized checkpoint,
// replace `node` with the parent of `node`.
if let Some(parent) = node.parent.and_then(|index| self.nodes.get(index)) {
node = parent
} else {
// If `node` is not the finalized block and its parent does not
// exist in fork choice, then the parent must have been pruned.
// Proto-array only prunes blocks prior to the finalized block,
// so this means the parent conflicts with finality.
return false;
};
}
}
/// Returns the first *beacon block root* which contains an execution payload with the given
/// `block_hash`, if any.
pub fn execution_block_hash_to_beacon_block_root(