From 67ea6c679462e77ac0afbe950d33745dac5666e2 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 18:18:44 +0200 Subject: [PATCH] Make to_electra not fallible --- consensus/types/src/indexed_attestation.rs | 10 ++++---- slasher/src/array.rs | 20 ++-------------- slasher/src/lib.rs | 26 ++++++-------------- slasher/src/slasher.rs | 28 +++++++--------------- slasher/src/test_utils.rs | 4 ++-- 5 files changed, 25 insertions(+), 63 deletions(-) diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index d282e2f259c..ecddcd21796 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -116,11 +116,13 @@ impl IndexedAttestation { } } - pub fn to_electra(self) -> Result, ssz_types::Error> { - Ok(match self { + pub fn to_electra(self) -> IndexedAttestationElectra { + match self { Self::Base(att) => { let extended_attesting_indices: VariableList = - VariableList::new(att.attesting_indices.to_vec())?; + VariableList::new(att.attesting_indices.to_vec()) + .expect("MaxValidatorsPerSlot must be >= MaxValidatorsPerCommittee"); + // TODO: Add test after unstable rebase https://github.com/sigp/lighthouse/blob/474c1b44863927c588dd05ab2ac0f934298398e1/consensus/types/src/eth_spec.rs#L541 IndexedAttestationElectra { attesting_indices: extended_attesting_indices, @@ -129,7 +131,7 @@ impl IndexedAttestation { } } Self::Electra(att) => att, - }) + } } } diff --git a/slasher/src/array.rs b/slasher/src/array.rs index 714ccf4e77b..77ddceb85fe 100644 --- a/slasher/src/array.rs +++ b/slasher/src/array.rs @@ -5,7 +5,6 @@ use crate::{ }; use flate2::bufread::{ZlibDecoder, ZlibEncoder}; use serde::{Deserialize, Serialize}; -use slog::Logger; use std::borrow::Borrow; use std::collections::{btree_map::Entry, BTreeMap, HashSet}; use std::io::Read; @@ -486,7 +485,6 @@ pub fn update( batch: Vec>>, current_epoch: Epoch, config: &Config, - log: &Logger, ) -> Result>, Error> { // Split the batch up into horizontal segments. // Map chunk indexes in the range `0..self.config.chunk_size` to attestations @@ -506,7 +504,6 @@ pub fn update( &chunk_attestations, current_epoch, config, - log, )?; slashings.extend(update_array::<_, MaxTargetChunk>( db, @@ -515,7 +512,6 @@ pub fn update( &chunk_attestations, current_epoch, config, - log, )?); // Update all current epochs. @@ -574,7 +570,6 @@ pub fn update_array( chunk_attestations: &BTreeMap>>>, current_epoch: Epoch, config: &Config, - log: &Logger, ) -> Result>, Error> { let mut slashings = HashSet::new(); // Map from chunk index to updated chunk at that index. @@ -608,19 +603,8 @@ pub fn update_array( current_epoch, config, )?; - match slashing_status.into_slashing(&attestation.indexed) { - Ok(Some(slashing)) => { - slashings.insert(slashing); - } - Err(e) => { - slog::error!( - log, - "Invalid slashing conversion"; - "validator_index" => validator_index, - "error" => e - ); - } - Ok(None) => {} + if let Some(slashing) = slashing_status.into_slashing(&attestation.indexed) { + slashings.insert(slashing); } } } diff --git a/slasher/src/lib.rs b/slasher/src/lib.rs index 0c097ac77fb..4d58fa77029 100644 --- a/slasher/src/lib.rs +++ b/slasher/src/lib.rs @@ -53,11 +53,11 @@ impl AttesterSlashingStatus { pub fn into_slashing( self, new_attestation: &IndexedAttestation, - ) -> Result>, String> { + ) -> Option> { use AttesterSlashingStatus::*; // The surrounding attestation must be in `attestation_1` to be valid. - Ok(match self { + match self { NotSlashable => None, AlreadyDoubleVoted => None, DoubleVote(existing) | SurroundedByExisting(existing) => { @@ -70,14 +70,8 @@ impl AttesterSlashingStatus { } // A slashing involving an electra attestation type must return an `AttesterSlashingElectra` type (_, _) => Some(AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: existing - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, - attestation_2: new_attestation - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, + attestation_1: existing.clone().to_electra(), + attestation_2: new_attestation.clone().to_electra(), })), } } @@ -90,16 +84,10 @@ impl AttesterSlashingStatus { } // A slashing involving an electra attestation type must return an `AttesterSlashingElectra` type (_, _) => Some(AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: new_attestation - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, - attestation_2: existing - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, + attestation_1: new_attestation.clone().to_electra(), + attestation_2: existing.clone().to_electra(), })), }, - }) + } } } diff --git a/slasher/src/slasher.rs b/slasher/src/slasher.rs index ef6dcce9b1b..fc8e8453c89 100644 --- a/slasher/src/slasher.rs +++ b/slasher/src/slasher.rs @@ -247,7 +247,6 @@ impl Slasher { batch, current_epoch, &self.config, - &self.log, ) { Ok(slashings) => { if !slashings.is_empty() { @@ -295,25 +294,14 @@ impl Slasher { indexed_attestation_id, )?; - match slashing_status.into_slashing(attestation) { - Ok(Some(slashing)) => { - debug!( - self.log, - "Found double-vote slashing"; - "validator_index" => validator_index, - "epoch" => slashing.attestation_1().data().target.epoch, - ); - slashings.insert(slashing); - } - Err(e) => { - error!( - self.log, - "Invalid slashing conversion"; - "validator_index" => validator_index, - "error" => e - ); - } - Ok(None) => {} + if let Some(slashing) = slashing_status.into_slashing(attestation) { + debug!( + self.log, + "Found double-vote slashing"; + "validator_index" => validator_index, + "epoch" => slashing.attestation_1().data().target.epoch, + ); + slashings.insert(slashing); } } diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index ef623911bfa..7019a8aed76 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -71,8 +71,8 @@ pub fn att_slashing( } // A slashing involving an electra attestation type must return an electra AttesterSlashing type (_, _) => AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: attestation_1.clone().to_electra().unwrap(), - attestation_2: attestation_2.clone().to_electra().unwrap(), + attestation_1: attestation_1.clone().to_electra(), + attestation_2: attestation_2.clone().to_electra(), }), } }