Ensure FC uses the current slot from the store (#3402)

## Issue Addressed

NA

## Proposed Changes

Ensure that we read the current slot from the `fc_store` rather than the slot clock. This is because the `fc_store` will never allow the slot to go backwards, even if the system clock does. The `ProtoArray::find_head` function assumes a non-decreasing slot.

This issue can cause logs like this:

```
ERRO Error whist recomputing head, error: ForkChoiceError(ProtoArrayError("find_head failed: InvalidBestNode(InvalidBestNodeInfo { start_root: 0xb22655aa2ae23075a60bd40797b3ba220db33d6fb86fa7910f0ed48e34bda72f, justified_checkpoint: Checkpoint { epoch: Epoch(111569), root: 0xb22655aa2ae23075a60bd40797b3ba220db33d6fb86fa7910f0ed48e34bda72f }, finalized_checkpoint: Checkpoint { epoch: Epoch(111568), root: 0x6140797e40c587b0d3f159483bbc603accb7b3af69891979d63efac437f9896f }, head_root: 0xb22655aa2ae23075a60bd40797b3ba220db33d6fb86fa7910f0ed48e34bda72f, head_justified_checkpoint: Some(Checkpoint { epoch: Epoch(111568), root: 0x6140797e40c587b0d3f159483bbc603accb7b3af69891979d63efac437f9896f }), head_finalized_checkpoint: Some(Checkpoint { epoch: Epoch(111567), root: 0x59b913d37383a158a9ea5546a572acc79e2cdfbc904c744744789d2c3814c570 }) })")), service: beacon, module: beacon_chain::canonical_head:499
```

We expect nodes to automatically recover from this issue within seconds without any major impact. However, having *any* errors in the path of fork choice is undesirable and should be avoided.

## Additional Info

NA
This commit is contained in:
Paul Hauner 2022-08-02 00:58:25 +00:00
parent 807bc8b0b3
commit d23437f726
3 changed files with 15 additions and 8 deletions

View File

@ -485,10 +485,13 @@ where
/// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#get_head
pub fn get_head(
&mut self,
current_slot: Slot,
system_time_current_slot: Slot,
spec: &ChainSpec,
) -> Result<Hash256, Error<T::Error>> {
self.update_time(current_slot, spec)?;
// Provide the slot (as per the system clock) to the `fc_store` and then return its view of
// the current slot. The `fc_store` will ensure that the `current_slot` is never
// decreasing, a property which we must maintain.
let current_slot = self.update_time(system_time_current_slot, spec)?;
let store = &mut self.fc_store;
@ -639,7 +642,7 @@ where
#[allow(clippy::too_many_arguments)]
pub fn on_block<Payload: ExecPayload<E>>(
&mut self,
current_slot: Slot,
system_time_current_slot: Slot,
block: BeaconBlockRef<E, Payload>,
block_root: Hash256,
block_delay: Duration,
@ -648,7 +651,10 @@ where
spec: &ChainSpec,
count_unrealized: CountUnrealized,
) -> Result<(), Error<T::Error>> {
let current_slot = self.update_time(current_slot, spec)?;
// Provide the slot (as per the system clock) to the `fc_store` and then return its view of
// the current slot. The `fc_store` will ensure that the `current_slot` is never
// decreasing, a property which we must maintain.
let current_slot = self.update_time(system_time_current_slot, spec)?;
// Parent block must be known.
let parent_block = self
@ -1051,13 +1057,12 @@ where
/// will not be run here.
pub fn on_attestation(
&mut self,
current_slot: Slot,
system_time_current_slot: Slot,
attestation: &IndexedAttestation<E>,
is_from_block: AttestationFromBlock,
spec: &ChainSpec,
) -> Result<(), Error<T::Error>> {
// Ensure the store is up-to-date.
self.update_time(current_slot, spec)?;
self.update_time(system_time_current_slot, spec)?;
// Ignore any attestations to the zero hash.
//

View File

@ -1,4 +1,4 @@
use types::{Checkpoint, Epoch, ExecutionBlockHash, Hash256};
use types::{Checkpoint, Epoch, ExecutionBlockHash, Hash256, Slot};
#[derive(Clone, PartialEq, Debug)]
pub enum Error {
@ -52,6 +52,7 @@ pub enum Error {
#[derive(Clone, PartialEq, Debug)]
pub struct InvalidBestNodeInfo {
pub current_slot: Slot,
pub start_root: Hash256,
pub justified_checkpoint: Checkpoint,
pub finalized_checkpoint: Checkpoint,

View File

@ -659,6 +659,7 @@ impl ProtoArray {
// Perform a sanity check that the node is indeed valid to be the head.
if !self.node_is_viable_for_head::<E>(best_node, current_slot) {
return Err(Error::InvalidBestNode(Box::new(InvalidBestNodeInfo {
current_slot,
start_root: *justified_root,
justified_checkpoint: self.justified_checkpoint,
finalized_checkpoint: self.finalized_checkpoint,