From d23437f726f88249ff735215e2a2f3767ddb5d6f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 2 Aug 2022 00:58:25 +0000 Subject: [PATCH] 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 --- consensus/fork_choice/src/fork_choice.rs | 19 ++++++++++++------- consensus/proto_array/src/error.rs | 3 ++- consensus/proto_array/src/proto_array.rs | 1 + 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 5438aaf62..c8d119a99 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -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> { - 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>( &mut self, - current_slot: Slot, + system_time_current_slot: Slot, block: BeaconBlockRef, block_root: Hash256, block_delay: Duration, @@ -648,7 +651,10 @@ where spec: &ChainSpec, count_unrealized: CountUnrealized, ) -> Result<(), 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, is_from_block: AttestationFromBlock, spec: &ChainSpec, ) -> Result<(), 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. // diff --git a/consensus/proto_array/src/error.rs b/consensus/proto_array/src/error.rs index 79b4cb2d8..826bf6c3a 100644 --- a/consensus/proto_array/src/error.rs +++ b/consensus/proto_array/src/error.rs @@ -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, diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 962408513..390eb902a 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -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::(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,