Merge pull request #5816 from realbigsean/electra-attestation-slashin…

…g-handling Electra slashing handling
sigp · May 24, 2024 · 3e10e68 · 3e10e68
2 parents 3f169ef + bafb5f0
commit 3e10e68
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 50 deletions.
diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs
@@ -115,6 +115,22 @@ impl<E: EthSpec> IndexedAttestation<E> {
             IndexedAttestation::Electra(att) => att.attesting_indices.first(),
         }
     }
+
+    pub fn to_electra(self) -> Result<IndexedAttestationElectra<E>, ssz_types::Error> {
+        Ok(match self {
+            Self::Base(att) => {
+                let extended_attesting_indices: VariableList<u64, E::MaxValidatorsPerSlot> =
+                    VariableList::new(att.attesting_indices.to_vec())?;
+
+                IndexedAttestationElectra {
+                    attesting_indices: extended_attesting_indices,
+                    data: att.data,
+                    signature: att.signature,
+                }
+            }
+            Self::Electra(att) => att,
+        })
+    }
 }
 
 impl<'a, E: EthSpec> IndexedAttestationRef<'a, E> {

diff --git a/slasher/src/array.rs b/slasher/src/array.rs
@@ -5,6 +5,7 @@ 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;
@@ -485,6 +486,7 @@ 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
@@ -504,6 +506,7 @@ pub fn update<E: EthSpec>(
         &chunk_attestations,
         current_epoch,
         config,
+        log,
     )?;
     slashings.extend(update_array::<_, MaxTargetChunk>(
         db,
@@ -512,6 +515,7 @@ pub fn update<E: EthSpec>(
         &chunk_attestations,
         current_epoch,
         config,
+        log,
     )?);
 
     // Update all current epochs.
@@ -570,6 +574,7 @@ 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.
@@ -603,8 +608,19 @@ pub fn update_array<E: EthSpec, T: TargetArrayChunk>(
                     current_epoch,
                     config,
                 )?;
-                if let Some(slashing) = slashing_status.into_slashing(&attestation.indexed) {
-                    slashings.insert(slashing);
+                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) => {}
                 }
             }
         }

diff --git a/slasher/src/lib.rs b/slasher/src/lib.rs
@@ -53,55 +53,56 @@ impl<E: EthSpec> AttesterSlashingStatus<E> {
     pub fn into_slashing(
         self,
         new_attestation: &IndexedAttestation<E>,
-    ) -> Option<AttesterSlashing<E>> {
+    ) -> Result<Option<AttesterSlashing<E>>, String> {
         use AttesterSlashingStatus::*;
 
         // The surrounding attestation must be in `attestation_1` to be valid.
-        match self {
+        Ok(match self {
             NotSlashable => None,
             AlreadyDoubleVoted => None,
-            DoubleVote(existing) | SurroundedByExisting(existing) => {
-                match (*existing, new_attestation) {
-                    // TODO(electra) - determine when we would convert a Base attestation to Electra / how to handle mismatched attestations here
-                    (IndexedAttestation::Base(existing_att), IndexedAttestation::Base(new_att)) => {
-                        Some(AttesterSlashing::Base(AttesterSlashingBase {
-                            attestation_1: existing_att,
-                            attestation_2: new_att.clone(),
-                        }))
-                    }
-                    (
-                        IndexedAttestation::Electra(existing_att),
-                        IndexedAttestation::Electra(new_att),
-                    ) => Some(AttesterSlashing::Electra(AttesterSlashingElectra {
+            DoubleVote(existing) | SurroundedByExisting(existing) => match *existing {
+                IndexedAttestation::Base(existing_att) => {
+                    Some(AttesterSlashing::Base(AttesterSlashingBase {
                         attestation_1: existing_att,
-                        attestation_2: new_att.clone(),
-                    })),
-                    _ => panic!("attestations must be of the same type"),
+                        attestation_2: new_attestation
+                            .as_base()
+                            .map_err(|e| format!("{e:?}"))?
+                            .clone(),
+                    }))
                 }
-            }
-            // TODO(electra): fix this once we superstruct IndexedAttestation (return the correct type)
-            SurroundsExisting(existing) => match (*existing, new_attestation) {
-                (IndexedAttestation::Base(existing_att), IndexedAttestation::Base(new_att)) => {
-                    Some(AttesterSlashing::Base(AttesterSlashingBase {
-                        attestation_1: new_att.clone(),
-                        attestation_2: existing_att,
+                IndexedAttestation::Electra(existing_att) => {
+                    Some(AttesterSlashing::Electra(AttesterSlashingElectra {
+                        attestation_1: existing_att,
+                        // A double vote should never convert, a surround vote where the surrounding
+                        // vote is electra may convert.
+                        attestation_2: new_attestation
+                            .clone()
+                            .to_electra()
+                            .map_err(|e| format!("{e:?}"))?,
                     }))
                 }
-                (
-                    IndexedAttestation::Electra(existing_att),
-                    IndexedAttestation::Electra(new_att),
-                ) => Some(AttesterSlashing::Electra(AttesterSlashingElectra {
-                    attestation_1: new_att.clone(),
-                    attestation_2: existing_att,
-                })),
-                _ => panic!("attestations must be of the same type"),
             },
-            /*
-            Some(AttesterSlashing::Base(AttesterSlashingBase {
-                attestation_1: new_attestation.clone(),
-                attestation_2: *existing,
-            })),
-            */
-        }
+            SurroundsExisting(existing) => {
+                match new_attestation {
+                    IndexedAttestation::Base(new_attestation) => {
+                        Some(AttesterSlashing::Base(AttesterSlashingBase {
+                            attestation_1: existing
+                                .as_base()
+                                .map_err(|e| format!("{e:?}"))?
+                                .clone(),
+                            attestation_2: new_attestation.clone(),
+                        }))
+                    }
+                    IndexedAttestation::Electra(new_attestation) => {
+                        Some(AttesterSlashing::Electra(AttesterSlashingElectra {
+                            attestation_1: existing.to_electra().map_err(|e| format!("{e:?}"))?,
+                            // A double vote should never convert, a surround vote where the surrounding
+                            // vote is electra may convert.
+                            attestation_2: new_attestation.clone(),
+                        }))
+                    }
+                }
+            }
+        })
     }
 }
diff --git a/slasher/src/slasher.rs b/slasher/src/slasher.rs
@@ -247,6 +247,7 @@ impl<E: EthSpec> Slasher<E> {
             batch,
             current_epoch,
             &self.config,
+            &self.log,
         ) {
             Ok(slashings) => {
                 if !slashings.is_empty() {
@@ -294,14 +295,25 @@ impl<E: EthSpec> Slasher<E> {
                 indexed_attestation_id,
             )?;
 
-            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);
+            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) => {}
             }
         }