Ensure VALID response from fcU updates protoarray (#3126)

## Issue Addressed

NA

## Proposed Changes

Ensures that a `VALID` response from a `forkchoiceUpdate` call will update that block in `ProtoArray`.

I also had to modify the mock execution engine so it wouldn't return valid when all payloads were supposed to be some other static value.

## Additional Info

NA

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -4032,7 +4032,25 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         match forkchoice_updated_response {
             Ok(status) => match &status {
-                PayloadStatus::Valid | PayloadStatus::Syncing => Ok(()),
+                PayloadStatus::Valid => {
+                    // Ensure that fork choice knows that the block is no longer optimistic.
+                    if let Err(e) = self
+                        .fork_choice
+                        .write()
+                        .on_valid_execution_payload(head_block_root)
+                    {
+                        error!(
+                            self.log,
+                            "Failed to validate payload";
+                            "error" => ?e
+                        )
+                    };
+                    Ok(())
+                }
+                // There's nothing to be done for a syncing response. If the block is already
+                // `SYNCING` in fork choice, there's nothing to do. If already known to be `VALID`
+                // or `INVALID` then we don't want to change it to syncing.
+                PayloadStatus::Syncing => Ok(()),
                 // The specification doesn't list `ACCEPTED` as a valid response to a fork choice
                 // update. This response *seems* innocent enough, so we won't return early with an
                 // error. However, we create a log to bring attention to the issue.

beacon_node/beacon_chain/tests/payload_invalidation.rs

@@ -149,6 +149,15 @@ impl InvalidPayloadRig {
             .unwrap()
     }
 
+    fn validate_manually(&self, block_root: Hash256) {
+        self.harness
+            .chain
+            .fork_choice
+            .write()
+            .on_valid_execution_payload(block_root)
+            .unwrap();
+    }
+
     fn import_block_parametric<F: Fn(&BlockError<E>) -> bool>(
         &mut self,
         is_valid: Payload,
@@ -651,6 +660,42 @@ fn invalid_after_optimistic_sync() {
     assert_eq!(head.block_root, roots[1]);
 }
 
+#[test]
+fn manually_validate_child() {
+    let mut rig = InvalidPayloadRig::new().enable_attestations();
+    rig.move_to_terminal_block();
+    rig.import_block(Payload::Valid); // Import a valid transition block.
+
+    let parent = rig.import_block(Payload::Syncing);
+    let child = rig.import_block(Payload::Syncing);
+
+    assert!(rig.execution_status(parent).is_not_verified());
+    assert!(rig.execution_status(child).is_not_verified());
+
+    rig.validate_manually(child);
+
+    assert!(rig.execution_status(parent).is_valid());
+    assert!(rig.execution_status(child).is_valid());
+}
+
+#[test]
+fn manually_validate_parent() {
+    let mut rig = InvalidPayloadRig::new().enable_attestations();
+    rig.move_to_terminal_block();
+    rig.import_block(Payload::Valid); // Import a valid transition block.
+
+    let parent = rig.import_block(Payload::Syncing);
+    let child = rig.import_block(Payload::Syncing);
+
+    assert!(rig.execution_status(parent).is_not_verified());
+    assert!(rig.execution_status(child).is_not_verified());
+
+    rig.validate_manually(parent);
+
+    assert!(rig.execution_status(parent).is_valid());
+    assert!(rig.execution_status(child).is_not_verified());
+}
+
 #[test]
 fn payload_preparation() {
     let mut rig = InvalidPayloadRig::new();

beacon_node/execution_layer/src/test_utils/handle_rpc.rs

@@ -100,7 +100,9 @@ pub async fn handle_rpc<T: EthSpec>(
             let forkchoice_state: JsonForkChoiceStateV1 = get_param(params, 0)?;
             let payload_attributes: Option<JsonPayloadAttributesV1> = get_param(params, 1)?;
 
-            let response = ctx
+            let head_block_hash = forkchoice_state.head_block_hash;
+
+            let mut response = ctx
                 .execution_block_generator
                 .write()
                 .forkchoice_updated_v1(
@@ -108,6 +110,14 @@ pub async fn handle_rpc<T: EthSpec>(
                     payload_attributes.map(|json| json.into()),
                 )?;
 
+            if let Some(mut status) = ctx.static_forkchoice_updated_response.lock().clone() {
+                if status.status == PayloadStatusV1Status::Valid {
+                    status.latest_valid_hash = Some(head_block_hash)
+                }
+
+                response.payload_status = status.into();
+            }
+
             Ok(serde_json::to_value(response).unwrap())
         }
         other => Err(format!(

beacon_node/execution_layer/src/test_utils/mod.rs

@@ -68,6 +68,7 @@ impl<T: EthSpec> MockServer<T> {
             previous_request: <_>::default(),
             preloaded_responses,
             static_new_payload_response: <_>::default(),
+            static_forkchoice_updated_response: <_>::default(),
             _phantom: PhantomData,
         });
 
@@ -134,6 +135,7 @@ impl<T: EthSpec> MockServer<T> {
             },
             should_import: true,
         };
+        *self.ctx.static_forkchoice_updated_response.lock() = Some(response.status.clone());
         *self.ctx.static_new_payload_response.lock() = Some(response)
     }
 
@@ -148,6 +150,7 @@ impl<T: EthSpec> MockServer<T> {
             },
             should_import,
         };
+        *self.ctx.static_forkchoice_updated_response.lock() = Some(response.status.clone());
         *self.ctx.static_new_payload_response.lock() = Some(response)
     }
 
@@ -160,6 +163,7 @@ impl<T: EthSpec> MockServer<T> {
             },
             should_import: true,
         };
+        *self.ctx.static_forkchoice_updated_response.lock() = Some(response.status.clone());
         *self.ctx.static_new_payload_response.lock() = Some(response)
     }
 
@@ -248,6 +252,7 @@ pub struct Context<T: EthSpec> {
     pub preloaded_responses: Arc<Mutex<Vec<serde_json::Value>>>,
     pub previous_request: Arc<Mutex<Option<serde_json::Value>>>,
     pub static_new_payload_response: Arc<Mutex<Option<StaticNewPayloadResponse>>>,
+    pub static_forkchoice_updated_response: Arc<Mutex<Option<PayloadStatusV1>>>,
     pub _phantom: PhantomData<T>,
 }
 

consensus/fork_choice/src/fork_choice.rs

@@ -18,6 +18,7 @@ pub enum Error<T> {
     InvalidProtoArrayBytes(String),
     InvalidLegacyProtoArrayBytes(String),
     FailedToProcessInvalidExecutionPayload(String),
+    FailedToProcessValidExecutionPayload(String),
     MissingProtoArrayBlock(Hash256),
     UnknownAncestor {
         ancestor_slot: Slot,
@@ -512,6 +513,16 @@ where
         Ok(true)
     }
 
+    /// See `ProtoArrayForkChoice::process_execution_payload_validation` for documentation.
+    pub fn on_valid_execution_payload(
+        &mut self,
+        block_root: Hash256,
+    ) -> Result<(), Error<T::Error>> {
+        self.proto_array
+            .process_execution_payload_validation(block_root)
+            .map_err(Error::FailedToProcessValidExecutionPayload)
+    }
+
     /// See `ProtoArrayForkChoice::process_execution_payload_invalidation` for documentation.
     pub fn on_invalid_execution_payload(
         &mut self,

consensus/proto_array/src/proto_array.rs

@@ -337,20 +337,37 @@ impl ProtoArray {
             self.maybe_update_best_child_and_descendant(parent_index, node_index)?;
 
             if matches!(block.execution_status, ExecutionStatus::Valid(_)) {
-                self.propagate_execution_payload_validation(parent_index)?;
+                self.propagate_execution_payload_validation_by_index(parent_index)?;
             }
         }
 
         Ok(())
     }
 
+    /// Updates the `block_root` and all ancestors to have validated execution payloads.
+    ///
+    /// Returns an error if:
+    ///
+    /// - The `block-root` is unknown.
+    /// - Any of the to-be-validated payloads are already invalid.
+    pub fn propagate_execution_payload_validation(
+        &mut self,
+        block_root: Hash256,
+    ) -> Result<(), Error> {
+        let index = *self
+            .indices
+            .get(&block_root)
+            .ok_or(Error::NodeUnknown(block_root))?;
+        self.propagate_execution_payload_validation_by_index(index)
+    }
+
     /// Updates the `verified_node_index` and all ancestors to have validated execution payloads.
     ///
     /// Returns an error if:
     ///
     /// - The `verified_node_index` is unknown.
     /// - Any of the to-be-validated payloads are already invalid.
-    pub fn propagate_execution_payload_validation(
+    fn propagate_execution_payload_validation_by_index(
         &mut self,
         verified_node_index: usize,
     ) -> Result<(), Error> {
@@ -475,7 +492,7 @@ impl ProtoArray {
 
                         // It might be new knowledge that this block is valid, ensure that it and all
                         // ancestors are marked as valid.
-                        self.propagate_execution_payload_validation(index)?;
+                        self.propagate_execution_payload_validation_by_index(index)?;
                         break;
                     }
                 }

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -188,6 +188,16 @@ impl ProtoArrayForkChoice {
         })
     }
 
+    /// See `ProtoArray::propagate_execution_payload_validation` for documentation.
+    pub fn process_execution_payload_validation(
+        &mut self,
+        block_root: Hash256,
+    ) -> Result<(), String> {
+        self.proto_array
+            .propagate_execution_payload_validation(block_root)
+            .map_err(|e| format!("Failed to process valid payload: {:?}", e))
+    }
+
     /// See `ProtoArray::propagate_execution_payload_invalidation` for documentation.
     pub fn process_execution_payload_invalidation(
         &mut self,