Make to_electra not fallible

sigp · Jun 17, 2024 · 67ea6c6 · 67ea6c6
1 parent 2d11fc4
commit 67ea6c6
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 63 deletions.
diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs
@@ -116,11 +116,13 @@ impl<E: EthSpec> IndexedAttestation<E> {
         }
     }
 
-    pub fn to_electra(self) -> Result<IndexedAttestationElectra<E>, ssz_types::Error> {
-        Ok(match self {
+    pub fn to_electra(self) -> IndexedAttestationElectra<E> {
+        match self {
             Self::Base(att) => {
                 let extended_attesting_indices: VariableList<u64, E::MaxValidatorsPerSlot> =
-                    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<E: EthSpec> IndexedAttestation<E> {
                 }
             }
             Self::Electra(att) => att,
-        })
+        }
     }
 }
 

diff --git 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<E: EthSpec>(
     batch: Vec<Arc<IndexedAttesterRecord<E>>>,
     current_epoch: Epoch,
     config: &Config,
-    log: &Logger,
 ) -> Result<HashSet<AttesterSlashing<E>>, 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<E: EthSpec>(
         &chunk_attestations,
         current_epoch,
         config,
-        log,
     )?;
     slashings.extend(update_array::<_, MaxTargetChunk>(
         db,
@@ -515,7 +512,6 @@ pub fn update<E: EthSpec>(
         &chunk_attestations,
         current_epoch,
         config,
-        log,
     )?);
 
     // Update all current epochs.
@@ -574,7 +570,6 @@ pub fn update_array<E: EthSpec, T: TargetArrayChunk>(
     chunk_attestations: &BTreeMap<usize, Vec<Arc<IndexedAttesterRecord<E>>>>,
     current_epoch: Epoch,
     config: &Config,
-    log: &Logger,
 ) -> Result<HashSet<AttesterSlashing<E>>, Error> {
     let mut slashings = HashSet::new();
     // Map from chunk index to updated chunk at that index.
@@ -608,19 +603,8 @@ pub fn update_array<E: EthSpec, T: TargetArrayChunk>(
                     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
@@ -53,11 +53,11 @@ impl<E: EthSpec> AttesterSlashingStatus<E> {
     pub fn into_slashing(
         self,
         new_attestation: &IndexedAttestation<E>,
-    ) -> Result<Option<AttesterSlashing<E>>, String> {
+    ) -> Option<AttesterSlashing<E>> {
         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<E: EthSpec> AttesterSlashingStatus<E> {
                     }
                     // 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<E: EthSpec> AttesterSlashingStatus<E> {
                 }
                 // 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
@@ -247,7 +247,6 @@ impl<E: EthSpec> Slasher<E> {
             batch,
             current_epoch,
             &self.config,
-            &self.log,
         ) {
             Ok(slashings) => {
                 if !slashings.is_empty() {
@@ -295,25 +294,14 @@ impl<E: EthSpec> Slasher<E> {
                 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
@@ -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(),
         }),
     }
 }