diff --git a/beacon_node/beacon_chain/src/light_client_finality_update_verification.rs b/beacon_node/beacon_chain/src/light_client_finality_update_verification.rs index 879fa02f7d9..0d5a5425d5c 100644 --- a/beacon_node/beacon_chain/src/light_client_finality_update_verification.rs +++ b/beacon_node/beacon_chain/src/light_client_finality_update_verification.rs @@ -3,7 +3,7 @@ use derivative::Derivative; use slot_clock::SlotClock; use std::time::Duration; use strum::AsRefStr; -use types::LightClientFinalityUpdate; +use types::{Hash256, LightClientFinalityUpdate, Slot}; /// Returned when a light client finality update was not successfully verified. It might not have been verified for /// two reasons: @@ -21,12 +21,37 @@ pub enum Error { /// /// Assuming the local clock is correct, the peer has sent an invalid message. TooEarly, - /// Light client finality update message does not match the locally constructed one. - InvalidLightClientFinalityUpdate, + /// Light client finalized update message does not match the locally constructed one, it has a + /// different signature slot. + MismatchedSignatureSlot { local: Slot, observed: Slot }, + /// Light client finalized update message does not match the locally constructed one, it has a + /// different finalized block header for the same signature slot. + MismatchedFinalizedHeader { + local_finalized_header_root: Hash256, + observed_finalized_header_root: Hash256, + signature_slot: Slot, + }, + /// Light client finalized update message does not match the locally constructed one, it has a + /// different attested block header for the same signature slot and finalized header. + MismatchedAttestedHeader { + local_attested_header_root: Hash256, + observed_attested_header_root: Hash256, + finalized_header_root: Hash256, + signature_slot: Slot, + }, + /// Light client finalized update message does not match the locally constructed one, it has a + /// different proof or sync aggregate for the same slot, attested header and finalized header. + MismatchedProofOrSyncAggregate { + attested_header_root: Hash256, + finalized_header_root: Hash256, + signature_slot: Slot, + }, /// Signature slot start time is none. SigSlotStartIsNone, /// Failed to construct a LightClientFinalityUpdate from state. FailedConstructingUpdate, + /// Silently ignore this light client finality update + Ignore, } /// Wraps a `LightClientFinalityUpdate` that has been verified for propagation on the gossip network. @@ -48,7 +73,7 @@ impl VerifiedLightClientFinalityUpdate { // verify that enough time has passed for the block to have been propagated let start_time = chain .slot_clock - .start_of(*rcv_finality_update.signature_slot()) + .start_of(rcv_finality_update.signature_slot()) .ok_or(Error::SigSlotStartIsNone)?; let one_third_slot_duration = Duration::new(chain.spec.seconds_per_slot / 3, 0); if seen_timestamp + chain.spec.maximum_gossip_clock_disparity() @@ -57,16 +82,76 @@ impl VerifiedLightClientFinalityUpdate { return Err(Error::TooEarly); } + if let Some(latest_broadcasted_finality_update) = chain + .light_client_server_cache + .get_latest_broadcasted_finality_update() + { + // Ignore the incoming finality update if we've already broadcasted it + if latest_broadcasted_finality_update == rcv_finality_update { + return Err(Error::Ignore); + } + + // Ignore the incoming finality update if the latest broadcasted attested header slot + // is greater than the incoming attested header slot. + if latest_broadcasted_finality_update.get_attested_header_slot() + > rcv_finality_update.get_attested_header_slot() + { + return Err(Error::Ignore); + } + } + let latest_finality_update = chain .light_client_server_cache .get_latest_finality_update() .ok_or(Error::FailedConstructingUpdate)?; - // verify that the gossiped finality update is the same as the locally constructed one. + // Ignore the incoming finality update if the latest constructed attested header slot + // is greater than the incoming attested header slot. + if latest_finality_update.get_attested_header_slot() + > rcv_finality_update.get_attested_header_slot() + { + return Err(Error::Ignore); + } + + // Verify that the gossiped finality update is the same as the locally constructed one. if latest_finality_update != rcv_finality_update { - return Err(Error::InvalidLightClientFinalityUpdate); + let signature_slot = latest_finality_update.signature_slot(); + if signature_slot != rcv_finality_update.signature_slot() { + return Err(Error::MismatchedSignatureSlot { + local: signature_slot, + observed: rcv_finality_update.signature_slot(), + }); + } + let local_finalized_header_root = latest_finality_update.get_finalized_header_root(); + let observed_finalized_header_root = rcv_finality_update.get_finalized_header_root(); + if local_finalized_header_root != observed_finalized_header_root { + return Err(Error::MismatchedFinalizedHeader { + local_finalized_header_root, + observed_finalized_header_root, + signature_slot, + }); + } + let local_attested_header_root = latest_finality_update.get_attested_header_root(); + let observed_attested_header_root = rcv_finality_update.get_attested_header_root(); + if local_attested_header_root != observed_attested_header_root { + return Err(Error::MismatchedAttestedHeader { + local_attested_header_root, + observed_attested_header_root, + finalized_header_root: local_finalized_header_root, + signature_slot, + }); + } + return Err(Error::MismatchedProofOrSyncAggregate { + attested_header_root: local_attested_header_root, + finalized_header_root: local_finalized_header_root, + signature_slot, + }); } + chain + .light_client_server_cache + .set_latest_broadcasted_finality_update(rcv_finality_update.clone()); + Ok(Self { light_client_finality_update: rcv_finality_update, seen_timestamp, diff --git a/beacon_node/beacon_chain/src/light_client_optimistic_update_verification.rs b/beacon_node/beacon_chain/src/light_client_optimistic_update_verification.rs index 5665adc3ed9..4da69134433 100644 --- a/beacon_node/beacon_chain/src/light_client_optimistic_update_verification.rs +++ b/beacon_node/beacon_chain/src/light_client_optimistic_update_verification.rs @@ -4,7 +4,7 @@ use eth2::types::Hash256; use slot_clock::SlotClock; use std::time::Duration; use strum::AsRefStr; -use types::LightClientOptimisticUpdate; +use types::{LightClientOptimisticUpdate, Slot}; /// Returned when a light client optimistic update was not successfully verified. It might not have been verified for /// two reasons: @@ -22,14 +22,30 @@ pub enum Error { /// /// Assuming the local clock is correct, the peer has sent an invalid message. TooEarly, - /// Light client optimistic update message does not match the locally constructed one. - InvalidLightClientOptimisticUpdate, + /// Light client optimistic update message does not match the locally constructed one, it has a + /// different signature slot. + MismatchedSignatureSlot { local: Slot, observed: Slot }, + /// Light client optimistic update message does not match the locally constructed one, it has a + /// different block header at the same slot. + MismatchedAttestedHeader { + local_attested_header_root: Hash256, + observed_attested_header_root: Hash256, + signature_slot: Slot, + }, + /// Light client optimistic update message does not match the locally constructed one, it has a + /// different sync aggregate for the same slot and attested header. + MismatchedSyncAggregate { + attested_header_root: Hash256, + signature_slot: Slot, + }, /// Signature slot start time is none. SigSlotStartIsNone, /// Failed to construct a LightClientOptimisticUpdate from state. FailedConstructingUpdate, /// Unknown block with parent root. UnknownBlockParentRoot(Hash256), + /// Silently ignore this light client optimistic update + Ignore, } /// Wraps a `LightClientOptimisticUpdate` that has been verified for propagation on the gossip network. @@ -52,7 +68,7 @@ impl VerifiedLightClientOptimisticUpdate { // verify that enough time has passed for the block to have been propagated let start_time = chain .slot_clock - .start_of(*rcv_optimistic_update.signature_slot()) + .start_of(rcv_optimistic_update.signature_slot()) .ok_or(Error::SigSlotStartIsNone)?; let one_third_slot_duration = Duration::new(chain.spec.seconds_per_slot / 3, 0); if seen_timestamp + chain.spec.maximum_gossip_clock_disparity() @@ -61,6 +77,22 @@ impl VerifiedLightClientOptimisticUpdate { return Err(Error::TooEarly); } + if let Some(latest_broadcasted_optimistic_update) = chain + .light_client_server_cache + .get_latest_broadcasted_optimistic_update() + { + // Ignore the incoming optimistic update if we've already broadcasted it + if latest_broadcasted_optimistic_update == rcv_optimistic_update { + return Err(Error::Ignore); + } + + // Ignore the incoming optimistic update if the latest broadcasted slot + // is greater than the incoming slot. + if latest_broadcasted_optimistic_update.get_slot() > rcv_optimistic_update.get_slot() { + return Err(Error::Ignore); + } + } + let head = chain.canonical_head.cached_head(); let head_block = &head.snapshot.beacon_block; // check if we can process the optimistic update immediately @@ -76,11 +108,40 @@ impl VerifiedLightClientOptimisticUpdate { .get_latest_optimistic_update() .ok_or(Error::FailedConstructingUpdate)?; - // verify that the gossiped optimistic update is the same as the locally constructed one. + // Ignore the incoming optimistic update if the latest constructed slot + // is greater than the incoming slot. + if latest_optimistic_update.get_slot() > rcv_optimistic_update.get_slot() { + return Err(Error::Ignore); + } + + // Verify that the gossiped optimistic update is the same as the locally constructed one. if latest_optimistic_update != rcv_optimistic_update { - return Err(Error::InvalidLightClientOptimisticUpdate); + let signature_slot = latest_optimistic_update.signature_slot(); + if signature_slot != rcv_optimistic_update.signature_slot() { + return Err(Error::MismatchedSignatureSlot { + local: signature_slot, + observed: rcv_optimistic_update.signature_slot(), + }); + } + let local_attested_header_root = latest_optimistic_update.get_canonical_root(); + let observed_attested_header_root = rcv_optimistic_update.get_canonical_root(); + if local_attested_header_root != observed_attested_header_root { + return Err(Error::MismatchedAttestedHeader { + local_attested_header_root, + observed_attested_header_root, + signature_slot, + }); + } + return Err(Error::MismatchedSyncAggregate { + attested_header_root: local_attested_header_root, + signature_slot, + }); } + chain + .light_client_server_cache + .set_latest_broadcasted_optimistic_update(rcv_optimistic_update.clone()); + let parent_root = rcv_optimistic_update.get_parent_root(); Ok(Self { light_client_optimistic_update: rcv_optimistic_update, diff --git a/beacon_node/beacon_chain/src/light_client_server_cache.rs b/beacon_node/beacon_chain/src/light_client_server_cache.rs index 3099c451c00..22122ee5547 100644 --- a/beacon_node/beacon_chain/src/light_client_server_cache.rs +++ b/beacon_node/beacon_chain/src/light_client_server_cache.rs @@ -40,6 +40,10 @@ pub struct LightClientServerCache { latest_written_current_sync_committee: RwLock>>>, /// Caches state proofs by block root prev_block_cache: Mutex>>, + /// Tracks the latest broadcasted finality update + latest_broadcasted_finality_update: RwLock>>, + /// Tracks the latest broadcasted optimistic update + latest_broadcasted_optimistic_update: RwLock>>, } impl LightClientServerCache { @@ -49,6 +53,8 @@ impl LightClientServerCache { latest_optimistic_update: None.into(), latest_light_client_update: None.into(), latest_written_current_sync_committee: None.into(), + latest_broadcasted_finality_update: None.into(), + latest_broadcasted_optimistic_update: None.into(), prev_block_cache: lru::LruCache::new(PREV_BLOCK_CACHE_SIZE).into(), } } @@ -334,10 +340,89 @@ impl LightClientServerCache { Ok(new_value) } + /// Checks if we've already broadcasted the latest finality update. + /// If we haven't, update the `latest_broadcasted_finality_update` cache + /// and return the latest finality update for broadcasting, else return `None`. + pub fn should_broadcast_latest_finality_update( + &self, + ) -> Option> { + if let Some(latest_finality_update) = self.get_latest_finality_update() { + let latest_broadcasted_finality_update = self.get_latest_broadcasted_finality_update(); + match latest_broadcasted_finality_update { + Some(latest_broadcasted_finality_update) => { + if latest_broadcasted_finality_update != latest_finality_update { + self.set_latest_broadcasted_finality_update(latest_finality_update.clone()); + return Some(latest_finality_update); + } + } + None => { + self.set_latest_broadcasted_finality_update(latest_finality_update.clone()); + return Some(latest_finality_update); + } + } + } + + None + } + pub fn get_latest_finality_update(&self) -> Option> { self.latest_finality_update.read().clone() } + pub fn get_latest_broadcasted_optimistic_update( + &self, + ) -> Option> { + self.latest_broadcasted_optimistic_update.read().clone() + } + + pub fn get_latest_broadcasted_finality_update( + &self, + ) -> Option> { + self.latest_broadcasted_finality_update.read().clone() + } + + pub fn set_latest_broadcasted_optimistic_update( + &self, + optimistic_update: LightClientOptimisticUpdate, + ) { + *self.latest_broadcasted_optimistic_update.write() = Some(optimistic_update.clone()); + } + + pub fn set_latest_broadcasted_finality_update( + &self, + finality_update: LightClientFinalityUpdate, + ) { + *self.latest_broadcasted_finality_update.write() = Some(finality_update.clone()); + } + + /// Checks if we've already broadcasted the latest optimistic update. + /// If we haven't, update the `latest_broadcasted_optimistic_update` cache + /// and return the latest optimistic update for broadcasting, else return `None`. + pub fn should_broadcast_latest_optimistic_update( + &self, + ) -> Option> { + if let Some(latest_optimistic_update) = self.get_latest_optimistic_update() { + let latest_broadcasted_optimistic_update = + self.get_latest_broadcasted_optimistic_update(); + match latest_broadcasted_optimistic_update { + Some(latest_broadcasted_optimistic_update) => { + if latest_broadcasted_optimistic_update != latest_optimistic_update { + self.set_latest_broadcasted_optimistic_update( + latest_optimistic_update.clone(), + ); + return Some(latest_optimistic_update); + } + } + None => { + self.set_latest_broadcasted_optimistic_update(latest_optimistic_update.clone()); + return Some(latest_optimistic_update); + } + } + } + + None + } + pub fn get_latest_optimistic_update(&self) -> Option> { self.latest_optimistic_update.read().clone() } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index e9b2e8e6bf3..2db93c00336 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2600,7 +2600,7 @@ pub fn serve( let fork_name = chain .spec - .fork_name_at_slot::(*update.signature_slot()); + .fork_name_at_slot::(update.signature_slot()); match accept_header { Some(api_types::Accept::Ssz) => Response::builder() .status(200) diff --git a/beacon_node/http_api/src/sync_committees.rs b/beacon_node/http_api/src/sync_committees.rs index aa126bbc822..57c74f8d019 100644 --- a/beacon_node/http_api/src/sync_committees.rs +++ b/beacon_node/http_api/src/sync_committees.rs @@ -320,6 +320,38 @@ pub fn process_signed_contribution_and_proofs( let seen_timestamp = timestamp_now(); + if let Some(latest_optimistic_update) = chain + .light_client_server_cache + .should_broadcast_latest_optimistic_update() + { + let _ = publish_pubsub_message( + &network_tx, + PubsubMessage::LightClientOptimisticUpdate(Box::new(latest_optimistic_update)), + ) + .inspect_err(|e| { + error!( + error = ?e, + "Unable to broadcast latest light client optimistic update" + ); + }); + }; + + if let Some(latest_finality_update) = chain + .light_client_server_cache + .should_broadcast_latest_finality_update() + { + let _ = publish_pubsub_message( + &network_tx, + PubsubMessage::LightClientFinalityUpdate(Box::new(latest_finality_update)), + ) + .inspect_err(|e| { + error!( + error = ?e, + "Unable to broadcast latest light client finality update" + ); + }); + }; + // Verify contributions & broadcast to the network. for (index, contribution) in signed_contribution_and_proofs.into_iter().enumerate() { let aggregator_index = contribution.message.aggregator_index; diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 6bdcd02197e..0b17965f3cb 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1967,7 +1967,10 @@ impl NetworkBeaconProcessor { Err(e) => { metrics::register_finality_update_error(&e); match e { - LightClientFinalityUpdateError::InvalidLightClientFinalityUpdate => { + LightClientFinalityUpdateError::MismatchedSignatureSlot { .. } + | LightClientFinalityUpdateError::MismatchedAttestedHeader { .. } + | LightClientFinalityUpdateError::MismatchedFinalizedHeader { .. } + | LightClientFinalityUpdateError::MismatchedProofOrSyncAggregate { .. } => { debug!( %peer_id, error = ?e, @@ -1999,6 +2002,7 @@ impl NetworkBeaconProcessor { error = ?e, "Light client error constructing finality update" ), + LightClientFinalityUpdateError::Ignore => {} } self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); } @@ -2080,7 +2084,9 @@ impl NetworkBeaconProcessor { } return; } - LightClientOptimisticUpdateError::InvalidLightClientOptimisticUpdate => { + LightClientOptimisticUpdateError::MismatchedSignatureSlot { .. } + | LightClientOptimisticUpdateError::MismatchedAttestedHeader { .. } + | LightClientOptimisticUpdateError::MismatchedSyncAggregate { .. } => { metrics::register_optimistic_update_error(&e); debug!( @@ -2119,6 +2125,7 @@ impl NetworkBeaconProcessor { "Light client error constructing optimistic update" ) } + LightClientOptimisticUpdateError::Ignore => {} } self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); } diff --git a/consensus/types/src/light_client_finality_update.rs b/consensus/types/src/light_client_finality_update.rs index 9189dcd0a0b..2125b4668b4 100644 --- a/consensus/types/src/light_client_finality_update.rs +++ b/consensus/types/src/light_client_finality_update.rs @@ -79,6 +79,7 @@ pub struct LightClientFinalityUpdate { /// current sync aggregate pub sync_aggregate: SyncAggregate, /// Slot of the sync aggregated signature + #[superstruct(getter(copy))] pub signature_slot: Slot, } @@ -179,6 +180,20 @@ impl LightClientFinalityUpdate { }) } + pub fn get_attested_header_root<'a>(&'a self) -> Hash256 { + map_light_client_finality_update_ref!(&'a _, self.to_ref(), |inner, cons| { + cons(inner); + inner.attested_header.beacon.canonical_root() + }) + } + + pub fn get_finalized_header_root<'a>(&'a self) -> Hash256 { + map_light_client_finality_update_ref!(&'a _, self.to_ref(), |inner, cons| { + cons(inner); + inner.finalized_header.beacon.canonical_root() + }) + } + pub fn from_ssz_bytes(bytes: &[u8], fork_name: ForkName) -> Result { let finality_update = match fork_name { ForkName::Altair | ForkName::Bellatrix => { @@ -227,7 +242,7 @@ impl LightClientFinalityUpdate { if attested_slot > prev_slot { true } else { - attested_slot == prev_slot && signature_slot > *self.signature_slot() + attested_slot == prev_slot && signature_slot > self.signature_slot() } } } diff --git a/consensus/types/src/light_client_optimistic_update.rs b/consensus/types/src/light_client_optimistic_update.rs index 5701ebd8751..13e308cd278 100644 --- a/consensus/types/src/light_client_optimistic_update.rs +++ b/consensus/types/src/light_client_optimistic_update.rs @@ -60,6 +60,7 @@ pub struct LightClientOptimisticUpdate { /// current sync aggregate pub sync_aggregate: SyncAggregate, /// Slot of the sync aggregated signature + #[superstruct(getter(copy))] pub signature_slot: Slot, } @@ -200,7 +201,7 @@ impl LightClientOptimisticUpdate { if attested_slot > prev_slot { true } else { - attested_slot == prev_slot && signature_slot > *self.signature_slot() + attested_slot == prev_slot && signature_slot > self.signature_slot() } } } diff --git a/testing/simulator/src/checks.rs b/testing/simulator/src/checks.rs index 1b2d4024d11..e7cc9b7a4ee 100644 --- a/testing/simulator/src/checks.rs +++ b/testing/simulator/src/checks.rs @@ -303,7 +303,7 @@ pub(crate) async fn verify_light_client_updates( } // Verify light client optimistic update. `signature_slot_distance` should be 1 in the ideal scenario. - let signature_slot = *client + let signature_slot = client .get_beacon_light_client_optimistic_update::() .await .map_err(|e| format!("Error while getting light client updates: {:?}", e))? @@ -332,7 +332,7 @@ pub(crate) async fn verify_light_client_updates( } continue; } - let signature_slot = *client + let signature_slot = client .get_beacon_light_client_finality_update::() .await .map_err(|e| format!("Error while getting light client updates: {:?}", e))?