From c25934956b30dc7beb915b860f02eb7f68ad3a24 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 10 Aug 2022 07:52:58 +0000 Subject: [PATCH] Remove INVALID_TERMINAL_BLOCK (#3385) ## Issue Addressed Resolves #3379 ## Proposed Changes Remove instances of `InvalidTerminalBlock` in lighthouse and use `Invalid {latest_valid_hash: "0x0000000000000000000000000000000000000000000000000000000000000000"}` to represent that status. --- beacon_node/beacon_chain/src/beacon_chain.rs | 35 ++++++++----- .../beacon_chain/src/execution_payload.rs | 7 ++- .../tests/payload_invalidation.rs | 52 ++++++++++--------- beacon_node/execution_layer/src/engine_api.rs | 1 - .../src/engine_api/json_structures.rs | 7 --- .../execution_layer/src/payload_status.rs | 19 ------- .../execution_layer/src/test_utils/mod.rs | 4 +- 7 files changed, 58 insertions(+), 67 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d6503d687..54c961e34 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4084,21 +4084,32 @@ impl BeaconChain { "Fork choice update invalidated payload"; "status" => ?status ); - // The execution engine has stated that all blocks between the - // `head_execution_block_hash` and `latest_valid_hash` are invalid. - self.process_invalid_execution_payload( - &InvalidationOperation::InvalidateMany { - head_block_root, - always_invalidate_head: true, - latest_valid_ancestor: latest_valid_hash, - }, - ) - .await?; + + // This implies that the terminal block was invalid. We are being explicit in + // invalidating only the head block in this case. + if latest_valid_hash == ExecutionBlockHash::zero() { + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateOne { + block_root: head_block_root, + }, + ) + .await?; + } else { + // The execution engine has stated that all blocks between the + // `head_execution_block_hash` and `latest_valid_hash` are invalid. + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateMany { + head_block_root, + always_invalidate_head: true, + latest_valid_ancestor: latest_valid_hash, + }, + ) + .await?; + } Err(BeaconChainError::ExecutionForkChoiceUpdateInvalid { status }) } - PayloadStatus::InvalidTerminalBlock { .. } - | PayloadStatus::InvalidBlockHash { .. } => { + PayloadStatus::InvalidBlockHash { .. } => { warn!( self.log, "Fork choice update invalidated payload"; diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 3c530aaac..7af171b79 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -114,6 +114,11 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( PayloadStatus::Invalid { latest_valid_hash, .. } => { + // latest_valid_hash == 0 implies that this was the terminal block + // Hence, we don't need to run `BeaconChain::process_invalid_execution_payload`. + if latest_valid_hash == ExecutionBlockHash::zero() { + return Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into()); + } // This block has not yet been applied to fork choice, so the latest block that was // imported to fork choice was the parent. let latest_root = block.parent_root(); @@ -127,7 +132,7 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into()) } - PayloadStatus::InvalidTerminalBlock { .. } | PayloadStatus::InvalidBlockHash { .. } => { + PayloadStatus::InvalidBlockHash { .. } => { // Returning an error here should be sufficient to invalidate the block. We have no // information to indicate its parent is invalid, so no need to run // `BeaconChain::process_invalid_execution_payload`. diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 7728b319d..027a708cf 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -40,7 +40,6 @@ enum Payload { }, Syncing, InvalidBlockHash, - InvalidTerminalBlock, } struct InvalidPayloadRig { @@ -231,16 +230,20 @@ impl InvalidPayloadRig { Payload::Invalid { latest_valid_hash } => { let latest_valid_hash = latest_valid_hash .unwrap_or_else(|| self.block_hash(block.message().parent_root())); - mock_execution_layer - .server - .all_payloads_invalid_on_new_payload(latest_valid_hash) + if latest_valid_hash == ExecutionBlockHash::zero() { + mock_execution_layer + .server + .all_payloads_invalid_terminal_block_on_new_payload() + } else { + mock_execution_layer + .server + .all_payloads_invalid_on_new_payload(latest_valid_hash) + } } + Payload::InvalidBlockHash => mock_execution_layer .server .all_payloads_invalid_block_hash_on_new_payload(), - Payload::InvalidTerminalBlock => mock_execution_layer - .server - .all_payloads_invalid_terminal_block_on_new_payload(), }; let set_forkchoice_updated = |payload: Payload| match payload { Payload::Valid => mock_execution_layer @@ -252,16 +255,20 @@ impl InvalidPayloadRig { Payload::Invalid { latest_valid_hash } => { let latest_valid_hash = latest_valid_hash .unwrap_or_else(|| self.block_hash(block.message().parent_root())); - mock_execution_layer - .server - .all_payloads_invalid_on_forkchoice_updated(latest_valid_hash) + if latest_valid_hash == ExecutionBlockHash::zero() { + mock_execution_layer + .server + .all_payloads_invalid_terminal_block_on_forkchoice_updated() + } else { + mock_execution_layer + .server + .all_payloads_invalid_on_forkchoice_updated(latest_valid_hash) + } } + Payload::InvalidBlockHash => mock_execution_layer .server .all_payloads_invalid_block_hash_on_forkchoice_updated(), - Payload::InvalidTerminalBlock => mock_execution_layer - .server - .all_payloads_invalid_terminal_block_on_forkchoice_updated(), }; match (new_payload_response, forkchoice_response) { @@ -294,9 +301,7 @@ impl InvalidPayloadRig { match forkchoice_response { Payload::Syncing => assert!(execution_status.is_strictly_optimistic()), Payload::Valid => assert!(execution_status.is_valid_and_post_bellatrix()), - Payload::Invalid { .. } - | Payload::InvalidBlockHash - | Payload::InvalidTerminalBlock => unreachable!(), + Payload::Invalid { .. } | Payload::InvalidBlockHash => unreachable!(), } assert_eq!( @@ -310,14 +315,8 @@ impl InvalidPayloadRig { "block from db must match block imported" ); } - ( - Payload::Invalid { .. } | Payload::InvalidBlockHash | Payload::InvalidTerminalBlock, - _, - ) - | ( - _, - Payload::Invalid { .. } | Payload::InvalidBlockHash | Payload::InvalidTerminalBlock, - ) => { + (Payload::Invalid { .. } | Payload::InvalidBlockHash, _) + | (_, Payload::Invalid { .. } | Payload::InvalidBlockHash) => { set_new_payload(new_payload_response); set_forkchoice_updated(forkchoice_response); @@ -473,7 +472,10 @@ async fn immediate_forkchoice_update_payload_invalid_block_hash() { #[tokio::test] async fn immediate_forkchoice_update_payload_invalid_terminal_block() { - immediate_forkchoice_update_invalid_test(|_| Payload::InvalidTerminalBlock).await + immediate_forkchoice_update_invalid_test(|_| Payload::Invalid { + latest_valid_hash: Some(ExecutionBlockHash::zero()), + }) + .await } /// Ensure the client tries to exit when the justified checkpoint is invalidated. diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 4f957d638..c370985ec 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -78,7 +78,6 @@ pub enum PayloadStatusV1Status { Syncing, Accepted, InvalidBlockHash, - InvalidTerminalBlock, } #[derive(Clone, Debug, PartialEq)] diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 9ed38b61b..31aa79f05 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -319,7 +319,6 @@ pub enum JsonPayloadStatusV1Status { Syncing, Accepted, InvalidBlockHash, - InvalidTerminalBlock, } #[derive(Debug, PartialEq, Serialize, Deserialize)] @@ -338,9 +337,6 @@ impl From for JsonPayloadStatusV1Status { PayloadStatusV1Status::Syncing => JsonPayloadStatusV1Status::Syncing, PayloadStatusV1Status::Accepted => JsonPayloadStatusV1Status::Accepted, PayloadStatusV1Status::InvalidBlockHash => JsonPayloadStatusV1Status::InvalidBlockHash, - PayloadStatusV1Status::InvalidTerminalBlock => { - JsonPayloadStatusV1Status::InvalidTerminalBlock - } } } } @@ -352,9 +348,6 @@ impl From for PayloadStatusV1Status { JsonPayloadStatusV1Status::Syncing => PayloadStatusV1Status::Syncing, JsonPayloadStatusV1Status::Accepted => PayloadStatusV1Status::Accepted, JsonPayloadStatusV1Status::InvalidBlockHash => PayloadStatusV1Status::InvalidBlockHash, - JsonPayloadStatusV1Status::InvalidTerminalBlock => { - PayloadStatusV1Status::InvalidTerminalBlock - } } } } diff --git a/beacon_node/execution_layer/src/payload_status.rs b/beacon_node/execution_layer/src/payload_status.rs index 46917a0aa..7db8e234d 100644 --- a/beacon_node/execution_layer/src/payload_status.rs +++ b/beacon_node/execution_layer/src/payload_status.rs @@ -18,9 +18,6 @@ pub enum PayloadStatus { InvalidBlockHash { validation_error: Option, }, - InvalidTerminalBlock { - validation_error: Option, - }, } /// Processes the response from the execution engine. @@ -90,22 +87,6 @@ pub fn process_payload_status( validation_error: response.validation_error.clone(), }) } - PayloadStatusV1Status::InvalidTerminalBlock => { - // In the interests of being liberal with what we accept, only raise a - // warning here. - if response.latest_valid_hash.is_some() { - warn!( - log, - "Malformed response from execution engine"; - "msg" => "expected a null latest_valid_hash", - "status" => ?response.status - ) - } - - Ok(PayloadStatus::InvalidTerminalBlock { - validation_error: response.validation_error.clone(), - }) - } PayloadStatusV1Status::Syncing => { // In the interests of being liberal with what we accept, only raise a // warning here. diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index 462e34e91..18612bf30 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -245,8 +245,8 @@ impl MockServer { fn invalid_terminal_block_status() -> PayloadStatusV1 { PayloadStatusV1 { - status: PayloadStatusV1Status::InvalidTerminalBlock, - latest_valid_hash: None, + status: PayloadStatusV1Status::Invalid, + latest_valid_hash: Some(ExecutionBlockHash::zero()), validation_error: Some("static response".into()), } }