From 44fae52cd7eb0b41908d2f8fb288cf859f5c5b7e Mon Sep 17 00:00:00 2001 From: Mac L Date: Wed, 27 Jul 2022 00:51:05 +0000 Subject: [PATCH] Refuse to sign sync committee messages when head is optimistic (#3191) ## Issue Addressed Resolves #3151 ## Proposed Changes When fetching duties for sync committee contributions, check the value of `execution_optimistic` of the head block from the BN and refuse to sign any sync committee messages `if execution_optimistic == true`. ## Additional Info - Is backwards compatible with older BNs - Finding a way to add test coverage for this would be prudent. Open to suggestions. --- beacon_node/beacon_chain/src/beacon_chain.rs | 35 +++++++++++++++++-- beacon_node/beacon_chain/src/errors.rs | 3 ++ beacon_node/http_api/src/lib.rs | 6 ++++ .../src/sync_committee_service.rs | 34 ++++++++++++++---- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index b9f9727e4c..2f35253058 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1380,10 +1380,41 @@ impl BeaconChain { pub fn get_aggregated_sync_committee_contribution( &self, sync_contribution_data: &SyncContributionData, - ) -> Option> { - self.naive_sync_aggregation_pool + ) -> Result>, Error> { + if let Some(contribution) = self + .naive_sync_aggregation_pool .read() .get(sync_contribution_data) + { + self.filter_optimistic_sync_committee_contribution(contribution) + .map(Option::Some) + } else { + Ok(None) + } + } + + fn filter_optimistic_sync_committee_contribution( + &self, + contribution: SyncCommitteeContribution, + ) -> Result, Error> { + let beacon_block_root = contribution.beacon_block_root; + match self + .canonical_head + .fork_choice_read_lock() + .get_block_execution_status(&beacon_block_root) + { + // The contribution references a block that is not in fork choice, it must be + // pre-finalization. + None => Err(Error::SyncContributionDataReferencesFinalizedBlock { beacon_block_root }), + // The contribution references a fully valid `beacon_block_root`. + Some(execution_status) if execution_status.is_valid_or_irrelevant() => Ok(contribution), + // The contribution references a block that has not been verified by an EL (i.e. it + // is optimistic or invalid). Don't return the block, return an error instead. + Some(execution_status) => Err(Error::HeadBlockNotFullyVerified { + beacon_block_root, + execution_status, + }), + } } /// Produce an unaggregated `Attestation` that is valid for the given `slot` and `index`. diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index d3337dfafe..189cb3fdea 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -184,6 +184,9 @@ pub enum BeaconChainError { CannotAttestToFinalizedBlock { beacon_block_root: Hash256, }, + SyncContributionDataReferencesFinalizedBlock { + beacon_block_root: Hash256, + }, RuntimeShutdown, TokioJoin(tokio::task::JoinError), ProcessInvalidExecutionPayload(JoinError), diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index c1980bee3d..3284f874f9 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2324,6 +2324,12 @@ pub fn serve( blocking_json_task(move || { chain .get_aggregated_sync_committee_contribution(&sync_committee_data) + .map_err(|e| { + warp_utils::reject::custom_bad_request(format!( + "unable to fetch sync contribution: {:?}", + e + )) + })? .map(api_types::GenericResponse::from) .ok_or_else(|| { warp_utils::reject::custom_not_found( diff --git a/validator_client/src/sync_committee_service.rs b/validator_client/src/sync_committee_service.rs index 105bf7d27f..73d0066f20 100644 --- a/validator_client/src/sync_committee_service.rs +++ b/validator_client/src/sync_committee_service.rs @@ -4,7 +4,7 @@ use environment::RuntimeContext; use eth2::types::BlockId; use futures::future::join_all; use futures::future::FutureExt; -use slog::{crit, debug, error, info, trace}; +use slog::{crit, debug, error, info, trace, warn}; use slot_clock::SlotClock; use std::collections::HashMap; use std::ops::Deref; @@ -174,17 +174,39 @@ impl SyncCommitteeService { return Ok(()); } - // Fetch block root for `SyncCommitteeContribution`. - let block_root = self + // Fetch `block_root` and `execution_optimistic` for `SyncCommitteeContribution`. + let response = self .beacon_nodes .first_success(RequireSynced::Yes, |beacon_node| async move { beacon_node.get_beacon_blocks_root(BlockId::Head).await }) .await .map_err(|e| e.to_string())? - .ok_or_else(|| format!("No block root found for slot {}", slot))? - .data - .root; + .ok_or_else(|| format!("No block root found for slot {}", slot))?; + + let block_root = response.data.root; + if let Some(execution_optimistic) = response.execution_optimistic { + if execution_optimistic { + warn!( + log, + "Refusing to sign sync committee messages for optimistic head block"; + "slot" => slot, + ); + return Ok(()); + } + } else if let Some(bellatrix_fork_epoch) = self.duties_service.spec.bellatrix_fork_epoch { + // If the slot is post Bellatrix, do not sign messages when we cannot verify the + // optimistic status of the head block. + if slot.epoch(E::slots_per_epoch()) > bellatrix_fork_epoch { + warn!( + log, + "Refusing to sign sync committee messages for a head block with an unknown \ + optimistic status"; + "slot" => slot, + ); + return Ok(()); + } + } // Spawn one task to publish all of the sync committee signatures. let validator_duties = slot_duties.duties;