Merge pull request #5975 from michaelsproul/electra-slasher-no-migration

Avoid changing slasher schema for Electra

Author: realbigsean

Cargo.lock

@@ -2,7 +2,9 @@
 
 use beacon_chain::block_verification_types::{AsBlock, ExecutedBlock, RpcBlock};
 use beacon_chain::{
-    test_utils::{AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType},
+    test_utils::{
+        test_spec, AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType,
+    },
     AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, ExecutionPendingBlock,
 };
 use beacon_chain::{
@@ -1210,8 +1212,14 @@ async fn block_gossip_verification() {
 #[tokio::test]
 async fn verify_block_for_gossip_slashing_detection() {
     let slasher_dir = tempdir().unwrap();
+    let spec = Arc::new(test_spec::<E>());
     let slasher = Arc::new(
-        Slasher::open(SlasherConfig::new(slasher_dir.path().into()), test_logger()).unwrap(),
+        Slasher::open(
+            SlasherConfig::new(slasher_dir.path().into()),
+            spec,
+            test_logger(),
+        )
+        .unwrap(),
     );
 
     let inner_slasher = slasher.clone();

beacon_node/beacon_chain/tests/block_verification.rs

@@ -80,7 +80,7 @@ impl<E: EthSpec> ProductionBeaconNode<E> {
 
         let builder = ClientBuilder::new(context.eth_spec_instance.clone())
             .runtime_context(context)
-            .chain_spec(spec)
+            .chain_spec(spec.clone())
             .beacon_processor(client_config.beacon_processor.clone())
             .http_api_config(client_config.http_api.clone())
             .disk_store(
@@ -113,8 +113,12 @@ impl<E: EthSpec> ProductionBeaconNode<E> {
                 _ => {}
             }
             let slasher = Arc::new(
-                Slasher::open(slasher_config, log.new(slog::o!("service" => "slasher")))
-                    .map_err(|e| format!("Slasher open error: {:?}", e))?,
+                Slasher::open(
+                    slasher_config,
+                    Arc::new(spec),
+                    log.new(slog::o!("service" => "slasher")),
+                )
+                .map_err(|e| format!("Slasher open error: {:?}", e))?,
             );
             builder.slasher(slasher)
         } else {

beacon_node/src/lib.rs

@@ -240,38 +240,6 @@ mod quoted_variable_list_u64 {
     }
 }
 
-#[derive(Debug, Clone, Encode, Decode, PartialEq)]
-#[ssz(enum_behaviour = "union")]
-pub enum IndexedAttestationOnDisk<E: EthSpec> {
-    Base(IndexedAttestationBase<E>),
-    Electra(IndexedAttestationElectra<E>),
-}
-
-#[derive(Debug, Clone, Encode, PartialEq)]
-#[ssz(enum_behaviour = "union")]
-pub enum IndexedAttestationRefOnDisk<'a, E: EthSpec> {
-    Base(&'a IndexedAttestationBase<E>),
-    Electra(&'a IndexedAttestationElectra<E>),
-}
-
-impl<'a, E: EthSpec> From<&'a IndexedAttestation<E>> for IndexedAttestationRefOnDisk<'a, E> {
-    fn from(attestation: &'a IndexedAttestation<E>) -> Self {
-        match attestation {
-            IndexedAttestation::Base(attestation) => Self::Base(attestation),
-            IndexedAttestation::Electra(attestation) => Self::Electra(attestation),
-        }
-    }
-}
-
-impl<E: EthSpec> From<IndexedAttestationOnDisk<E>> for IndexedAttestation<E> {
-    fn from(attestation: IndexedAttestationOnDisk<E>) -> Self {
-        match attestation {
-            IndexedAttestationOnDisk::Base(attestation) => Self::Base(attestation),
-            IndexedAttestationOnDisk::Electra(attestation) => Self::Electra(attestation),
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

consensus/types/src/indexed_attestation.rs

@@ -176,8 +176,7 @@ pub use crate::fork_versioned_response::{ForkVersionDeserialize, ForkVersionedRe
 pub use crate::graffiti::{Graffiti, GRAFFITI_BYTES_LEN};
 pub use crate::historical_batch::HistoricalBatch;
 pub use crate::indexed_attestation::{
-    IndexedAttestation, IndexedAttestationBase, IndexedAttestationElectra,
-    IndexedAttestationOnDisk, IndexedAttestationRef, IndexedAttestationRefOnDisk,
+    IndexedAttestation, IndexedAttestationBase, IndexedAttestationElectra, IndexedAttestationRef,
 };
 pub use crate::light_client_bootstrap::{
     LightClientBootstrap, LightClientBootstrapAltair, LightClientBootstrapCapella,

consensus/types/src/lib.rs

@@ -29,6 +29,7 @@ tree_hash = { workspace = true }
 tree_hash_derive = { workspace = true }
 types = { workspace = true }
 strum = { workspace = true }
+ssz_types = { workspace = true }
 
 # MDBX is pinned at the last version with Windows and macOS support.
 mdbx = { package = "libmdbx", git = "https://github.com/sigp/libmdbx-rs", tag = "v0.1.4", optional = true }

slasher/Cargo.toml

@@ -13,17 +13,19 @@ use parking_lot::Mutex;
 use serde::de::DeserializeOwned;
 use slog::{info, Logger};
 use ssz::{Decode, Encode};
+use ssz_derive::{Decode, Encode};
 use std::borrow::{Borrow, Cow};
 use std::marker::PhantomData;
 use std::sync::Arc;
 use tree_hash::TreeHash;
 use types::{
-    Epoch, EthSpec, Hash256, IndexedAttestation, IndexedAttestationOnDisk,
-    IndexedAttestationRefOnDisk, ProposerSlashing, SignedBeaconBlockHeader, Slot,
+    AggregateSignature, AttestationData, ChainSpec, Epoch, EthSpec, Hash256, IndexedAttestation,
+    IndexedAttestationBase, IndexedAttestationElectra, ProposerSlashing, SignedBeaconBlockHeader,
+    Slot, VariableList,
 };
 
 /// Current database schema version, to check compatibility of on-disk DB with software.
-pub const CURRENT_SCHEMA_VERSION: u64 = 4;
+pub const CURRENT_SCHEMA_VERSION: u64 = 3;
 
 /// Metadata about the slashing database itself.
 const METADATA_DB: &str = "metadata";
@@ -70,6 +72,7 @@ pub struct SlasherDB<E: EthSpec> {
     /// LRU cache mapping indexed attestation IDs to their attestation data roots.
     attestation_root_cache: Mutex<LruCache<IndexedAttestationId, Hash256>>,
     pub(crate) config: Arc<Config>,
+    pub(crate) spec: Arc<ChainSpec>,
     _phantom: PhantomData<E>,
 }
 
@@ -236,6 +239,43 @@ impl AsRef<[u8]> for IndexedAttestationId {
     }
 }
 
+/// Indexed attestation that abstracts over Phase0 and Electra variants by using a plain `Vec` for
+/// the attesting indices.
+///
+/// This allows us to avoid rewriting the entire indexed attestation database at Electra, which
+/// saves a lot of execution time. The bytes that it encodes to are the same as the bytes that a
+/// regular IndexedAttestation encodes to, because SSZ doesn't care about the length-bound.
+#[derive(Debug, PartialEq, Decode, Encode)]
+pub struct IndexedAttestationOnDisk {
+    attesting_indices: Vec<u64>,
+    data: AttestationData,
+    signature: AggregateSignature,
+}
+
+impl IndexedAttestationOnDisk {
+    fn into_indexed_attestation<E: EthSpec>(
+        self,
+        spec: &ChainSpec,
+    ) -> Result<IndexedAttestation<E>, Error> {
+        let fork_at_target_epoch = spec.fork_name_at_epoch(self.data.target.epoch);
+        if fork_at_target_epoch.electra_enabled() {
+            let attesting_indices = VariableList::new(self.attesting_indices)?;
+            Ok(IndexedAttestation::Electra(IndexedAttestationElectra {
+                attesting_indices,
+                data: self.data,
+                signature: self.signature,
+            }))
+        } else {
+            let attesting_indices = VariableList::new(self.attesting_indices)?;
+            Ok(IndexedAttestation::Base(IndexedAttestationBase {
+                attesting_indices,
+                data: self.data,
+                signature: self.signature,
+            }))
+        }
+    }
+}
+
 /// Bincode deserialization specialised to `Cow<[u8]>`.
 fn bincode_deserialize<T: DeserializeOwned>(bytes: Cow<[u8]>) -> Result<T, Error> {
     Ok(bincode::deserialize(bytes.borrow())?)
@@ -246,7 +286,7 @@ fn ssz_decode<T: Decode>(bytes: Cow<[u8]>) -> Result<T, Error> {
 }
 
 impl<E: EthSpec> SlasherDB<E> {
-    pub fn open(config: Arc<Config>, log: Logger) -> Result<Self, Error> {
+    pub fn open(config: Arc<Config>, spec: Arc<ChainSpec>, log: Logger) -> Result<Self, Error> {
         info!(log, "Opening slasher database"; "backend" => %config.backend);
 
         std::fs::create_dir_all(&config.database_path)?;
@@ -269,6 +309,7 @@ impl<E: EthSpec> SlasherDB<E> {
             databases,
             attestation_root_cache,
             config,
+            spec,
             _phantom: PhantomData,
         };
 
@@ -458,9 +499,8 @@ impl<E: EthSpec> SlasherDB<E> {
         };
 
         let attestation_key = IndexedAttestationId::new(indexed_att_id);
-        let indexed_attestation_on_disk: IndexedAttestationRefOnDisk<E> =
-            indexed_attestation.into();
-        let data = indexed_attestation_on_disk.as_ssz_bytes();
+        // IndexedAttestationOnDisk and IndexedAttestation have compatible encodings.
+        let data = indexed_attestation.as_ssz_bytes();
 
         cursor.put(attestation_key.as_ref(), &data)?;
         drop(cursor);
@@ -484,8 +524,8 @@ impl<E: EthSpec> SlasherDB<E> {
             .ok_or(Error::MissingIndexedAttestation {
                 id: indexed_attestation_id.as_u64(),
             })?;
-        let indexed_attestation: IndexedAttestationOnDisk<E> = ssz_decode(bytes)?;
-        Ok(indexed_attestation.into())
+        let indexed_attestation_on_disk: IndexedAttestationOnDisk = ssz_decode(bytes)?;
+        indexed_attestation_on_disk.into_indexed_attestation(&self.spec)
     }
 
     fn get_attestation_data_root(
@@ -775,3 +815,93 @@ impl<E: EthSpec> SlasherDB<E> {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use types::{Checkpoint, ForkName, MainnetEthSpec, Unsigned};
+
+    type E = MainnetEthSpec;
+
+    fn indexed_attestation_on_disk_roundtrip_test(
+        spec: &ChainSpec,
+        make_attestation: fn(
+            Vec<u64>,
+            AttestationData,
+            AggregateSignature,
+        ) -> IndexedAttestation<E>,
+        committee_len: u64,
+    ) {
+        let attestation_data = AttestationData {
+            slot: Slot::new(1000),
+            index: 0,
+            beacon_block_root: Hash256::repeat_byte(0xaa),
+            source: Checkpoint {
+                epoch: Epoch::new(0),
+                root: Hash256::repeat_byte(0xbb),
+            },
+            target: Checkpoint {
+                epoch: Epoch::new(31),
+                root: Hash256::repeat_byte(0xcc),
+            },
+        };
+
+        let attesting_indices = (0..committee_len).collect::<Vec<_>>();
+        let signature = AggregateSignature::infinity();
+
+        let fork_attestation = make_attestation(
+            attesting_indices.clone(),
+            attestation_data.clone(),
+            signature.clone(),
+        );
+
+        let on_disk = IndexedAttestationOnDisk {
+            attesting_indices,
+            data: attestation_data,
+            signature,
+        };
+        let encoded = on_disk.as_ssz_bytes();
+        assert_eq!(encoded, fork_attestation.as_ssz_bytes());
+
+        let decoded_on_disk = IndexedAttestationOnDisk::from_ssz_bytes(&encoded).unwrap();
+        assert_eq!(decoded_on_disk, on_disk);
+
+        let decoded = on_disk.into_indexed_attestation(spec).unwrap();
+        assert_eq!(decoded, fork_attestation);
+    }
+
+    /// Check that `IndexedAttestationOnDisk` and `IndexedAttestation` have compatible encodings.
+    #[test]
+    fn indexed_attestation_on_disk_roundtrip_base() {
+        let spec = ForkName::Base.make_genesis_spec(E::default_spec());
+        let make_attestation = |attesting_indices, data, signature| {
+            IndexedAttestation::<E>::Base(IndexedAttestationBase {
+                attesting_indices: VariableList::new(attesting_indices).unwrap(),
+                data,
+                signature,
+            })
+        };
+        indexed_attestation_on_disk_roundtrip_test(
+            &spec,
+            make_attestation,
+            <E as EthSpec>::MaxValidatorsPerCommittee::to_u64(),
+        )
+    }
+
+    #[test]
+    fn indexed_attestation_on_disk_roundtrip_electra() {
+        let spec = ForkName::Electra.make_genesis_spec(E::default_spec());
+        let make_attestation = |attesting_indices, data, signature| {
+            IndexedAttestation::<E>::Electra(IndexedAttestationElectra {
+                attesting_indices: VariableList::new(attesting_indices).unwrap(),
+                data,
+                signature,
+            })
+        };
+        indexed_attestation_on_disk_roundtrip_test(
+            &spec,
+            make_attestation,
+            <E as EthSpec>::MaxValidatorsPerSlot::to_u64(),
+        )
+    }
+}

slasher/src/database.rs

@@ -13,6 +13,7 @@ pub enum Error {
     DatabaseIOError(io::Error),
     DatabasePermissionsError(filesystem::Error),
     SszDecodeError(ssz::DecodeError),
+    SszTypesError(ssz_types::Error),
     BincodeError(bincode::Error),
     ArithError(safe_arith::ArithError),
     ChunkIndexOutOfBounds(usize),
@@ -100,6 +101,12 @@ impl From<ssz::DecodeError> for Error {
     }
 }
 
+impl From<ssz_types::Error> for Error {
+    fn from(e: ssz_types::Error) -> Self {
+        Error::SszTypesError(e)
+    }
+}
+
 impl From<bincode::Error> for Error {
     fn from(e: bincode::Error) -> Self {
         Error::BincodeError(e)

slasher/src/error.rs

@@ -17,10 +17,6 @@ impl<E: EthSpec> SlasherDB<E> {
                     software_schema_version: CURRENT_SCHEMA_VERSION,
                 }),
                 (x, y) if x == y => Ok(self),
-                (3, 4) => {
-                    // TODO(electra): db migration due to `IndexedAttestationOnDisk`
-                    Ok(self)
-                }
                 (_, _) => Err(Error::IncompatibleSchemaVersion {
                     database_schema_version: schema_version,
                     software_schema_version: CURRENT_SCHEMA_VERSION,

slasher/src/migrate.rs

@@ -13,7 +13,8 @@ use slog::{debug, error, info, Logger};
 use std::collections::HashSet;
 use std::sync::Arc;
 use types::{
-    AttesterSlashing, Epoch, EthSpec, IndexedAttestation, ProposerSlashing, SignedBeaconBlockHeader,
+    AttesterSlashing, ChainSpec, Epoch, EthSpec, IndexedAttestation, ProposerSlashing,
+    SignedBeaconBlockHeader,
 };
 
 #[derive(Debug)]
@@ -28,10 +29,10 @@ pub struct Slasher<E: EthSpec> {
 }
 
 impl<E: EthSpec> Slasher<E> {
-    pub fn open(config: Config, log: Logger) -> Result<Self, Error> {
+    pub fn open(config: Config, spec: Arc<ChainSpec>, log: Logger) -> Result<Self, Error> {
         config.validate()?;
         let config = Arc::new(config);
-        let db = SlasherDB::open(config.clone(), log.clone())?;
+        let db = SlasherDB::open(config.clone(), spec, log.clone())?;
         let attester_slashings = Mutex::new(HashSet::new());
         let proposer_slashings = Mutex::new(HashSet::new());
         let attestation_queue = AttestationQueue::default();