Skip to content

Commit

Permalink
Refuse to sign sync committee messages when head is optimistic (#3191)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
macladson committed Jul 26, 2022
1 parent 904dd62 commit bb88bad
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 8 deletions.
35 changes: 33 additions & 2 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1380,10 +1380,41 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
pub fn get_aggregated_sync_committee_contribution(
&self,
sync_contribution_data: &SyncContributionData,
) -> Option<SyncCommitteeContribution<T::EthSpec>> {
self.naive_sync_aggregation_pool
) -> Result<Option<SyncCommitteeContribution<T::EthSpec>>, 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<T::EthSpec>,
) -> Result<SyncCommitteeContribution<T::EthSpec>, 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`.
Expand Down
3 changes: 3 additions & 0 deletions beacon_node/beacon_chain/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ pub enum BeaconChainError {
CannotAttestToFinalizedBlock {
beacon_block_root: Hash256,
},
SyncContributionDataReferencesFinalizedBlock {
beacon_block_root: Hash256,
},
RuntimeShutdown,
TokioJoin(tokio::task::JoinError),
ProcessInvalidExecutionPayload(JoinError),
Expand Down
6 changes: 6 additions & 0 deletions beacon_node/http_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2319,6 +2319,12 @@ pub fn serve<T: BeaconChainTypes>(
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(
Expand Down
34 changes: 28 additions & 6 deletions validator_client/src/sync_committee_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -174,17 +174,39 @@ impl<T: SlotClock + 'static, E: EthSpec> SyncCommitteeService<T, E> {
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;
Expand Down

0 comments on commit bb88bad

Please sign in to comment.