diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 50daac83be..6599bb69fa 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4032,7 +4032,25 @@ impl BeaconChain { 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. diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 191a157a93..e1c082a880 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/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) -> 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(); diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 61038f40af..772ac3c866 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -100,7 +100,9 @@ pub async fn handle_rpc( let forkchoice_state: JsonForkChoiceStateV1 = get_param(params, 0)?; let payload_attributes: Option = 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( 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!( diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index 99adfa6558..afa6afb28c 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -68,6 +68,7 @@ impl MockServer { previous_request: <_>::default(), preloaded_responses, static_new_payload_response: <_>::default(), + static_forkchoice_updated_response: <_>::default(), _phantom: PhantomData, }); @@ -134,6 +135,7 @@ impl MockServer { }, 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 MockServer { }, 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 MockServer { }, 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 { pub preloaded_responses: Arc>>, pub previous_request: Arc>>, pub static_new_payload_response: Arc>>, + pub static_forkchoice_updated_response: Arc>>, pub _phantom: PhantomData, } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index dbcb0e336a..d71565fc19 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -18,6 +18,7 @@ pub enum Error { 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> { + 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, diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 7c76fdf8e5..5b0a46e952 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/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; } } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 007f262fdd..8f3f5ed798 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/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,