Capella consensus review (#4012)

* Add extra encoding/decoding tests

* Remove TODO

The method LGTM

* Remove `FreeAttestation`

This is an ancient relic, I'm surprised it still existed!

* Add paranoid check for eip4844 code

This is not technically necessary, but I think it's nice to be explicit about
EIP4844 consensus code for the time being.

* Reduce big-O complexity of address change pruning

I'm not sure this is *actually* useful, but it might come in handy if we see a
ton of address changes at the fork boundary. I know the devops team have been
testing with ~100k changes, so maybe this will help in that case.

* Revert "Reduce big-O complexity of address change pruning"

This reverts commit e7d93e6cc7cf1b92dd5a9e1966ce47d4078121eb.

consensus/state_processing/src/per_block_processing.rs

@@ -180,7 +180,11 @@ pub fn per_block_processing<T: EthSpec, Payload: AbstractExecPayload<T>>(
         )?;
     }
 
-    process_blob_kzg_commitments(block.body())?;
+    // Eip4844 specifications are not yet released so additional care is taken
+    // to ensure the code does not run in production.
+    if matches!(block, BeaconBlockRef::Eip4844(_)) {
+        process_blob_kzg_commitments(block.body())?;
+    }
 
     Ok(())
 }

consensus/types/src/beacon_block.rs

@@ -752,19 +752,65 @@ mod tests {
         });
     }
 
+    #[test]
+    fn roundtrip_capella_block() {
+        let rng = &mut XorShiftRng::from_seed([42; 16]);
+        let spec = &ForkName::Capella.make_genesis_spec(MainnetEthSpec::default_spec());
+
+        let inner_block = BeaconBlockCapella {
+            slot: Slot::random_for_test(rng),
+            proposer_index: u64::random_for_test(rng),
+            parent_root: Hash256::random_for_test(rng),
+            state_root: Hash256::random_for_test(rng),
+            body: BeaconBlockBodyCapella::random_for_test(rng),
+        };
+        let block = BeaconBlock::Capella(inner_block.clone());
+
+        test_ssz_tree_hash_pair_with(&block, &inner_block, |bytes| {
+            BeaconBlock::from_ssz_bytes(bytes, spec)
+        });
+    }
+
+    #[test]
+    fn roundtrip_4844_block() {
+        let rng = &mut XorShiftRng::from_seed([42; 16]);
+        let spec = &ForkName::Eip4844.make_genesis_spec(MainnetEthSpec::default_spec());
+
+        let inner_block = BeaconBlockEip4844 {
+            slot: Slot::random_for_test(rng),
+            proposer_index: u64::random_for_test(rng),
+            parent_root: Hash256::random_for_test(rng),
+            state_root: Hash256::random_for_test(rng),
+            body: BeaconBlockBodyEip4844::random_for_test(rng),
+        };
+        let block = BeaconBlock::Eip4844(inner_block.clone());
+
+        test_ssz_tree_hash_pair_with(&block, &inner_block, |bytes| {
+            BeaconBlock::from_ssz_bytes(bytes, spec)
+        });
+    }
+
     #[test]
     fn decode_base_and_altair() {
         type E = MainnetEthSpec;
-        let spec = E::default_spec();
+        let mut spec = E::default_spec();
 
         let rng = &mut XorShiftRng::from_seed([42; 16]);
 
-        let fork_epoch = spec.altair_fork_epoch.unwrap();
+        let altair_fork_epoch = spec.altair_fork_epoch.unwrap();
 
-        let base_epoch = fork_epoch.saturating_sub(1_u64);
+        let base_epoch = altair_fork_epoch.saturating_sub(1_u64);
         let base_slot = base_epoch.end_slot(E::slots_per_epoch());
-        let altair_epoch = fork_epoch;
+        let altair_epoch = altair_fork_epoch;
         let altair_slot = altair_epoch.start_slot(E::slots_per_epoch());
+        let capella_epoch = altair_fork_epoch + 1;
+        let capella_slot = capella_epoch.start_slot(E::slots_per_epoch());
+        let eip4844_epoch = capella_epoch + 1;
+        let eip4844_slot = eip4844_epoch.start_slot(E::slots_per_epoch());
+
+        spec.altair_fork_epoch = Some(altair_epoch);
+        spec.capella_fork_epoch = Some(capella_epoch);
+        spec.eip4844_fork_epoch = Some(eip4844_epoch);
 
         // BeaconBlockBase
         {
@@ -809,5 +855,49 @@ mod tests {
             BeaconBlock::from_ssz_bytes(&bad_altair_block.as_ssz_bytes(), &spec)
                 .expect_err("bad altair block cannot be decoded");
         }
+
+        // BeaconBlockCapella
+        {
+            let good_block = BeaconBlock::Capella(BeaconBlockCapella {
+                slot: capella_slot,
+                ..<_>::random_for_test(rng)
+            });
+            // It's invalid to have an Capella block with a epoch lower than the fork epoch.
+            let bad_block = {
+                let mut bad = good_block.clone();
+                *bad.slot_mut() = altair_slot;
+                bad
+            };
+
+            assert_eq!(
+                BeaconBlock::from_ssz_bytes(&good_block.as_ssz_bytes(), &spec)
+                    .expect("good capella block can be decoded"),
+                good_block
+            );
+            BeaconBlock::from_ssz_bytes(&bad_block.as_ssz_bytes(), &spec)
+                .expect_err("bad capella block cannot be decoded");
+        }
+
+        // BeaconBlockEip4844
+        {
+            let good_block = BeaconBlock::Eip4844(BeaconBlockEip4844 {
+                slot: eip4844_slot,
+                ..<_>::random_for_test(rng)
+            });
+            // It's invalid to have an Capella block with a epoch lower than the fork epoch.
+            let bad_block = {
+                let mut bad = good_block.clone();
+                *bad.slot_mut() = capella_slot;
+                bad
+            };
+
+            assert_eq!(
+                BeaconBlock::from_ssz_bytes(&good_block.as_ssz_bytes(), &spec)
+                    .expect("good eip4844 block can be decoded"),
+                good_block
+            );
+            BeaconBlock::from_ssz_bytes(&bad_block.as_ssz_bytes(), &spec)
+                .expect_err("bad eip4844 block cannot be decoded");
+        }
     }
 }

consensus/types/src/beacon_state.rs

@@ -710,7 +710,6 @@ impl<T: EthSpec> BeaconState<T> {
             .ok_or(Error::ShuffleIndexOutOfBounds(index))
     }
 
-    // TODO: check this implementation
     /// Convenience accessor for the `execution_payload_header` as an `ExecutionPayloadHeaderRef`.
     pub fn latest_execution_payload_header(&self) -> Result<ExecutionPayloadHeaderRef<T>, Error> {
         match self {

consensus/types/src/free_attestation.rs

@@ -1,13 +0,0 @@
-/// Note: this object does not actually exist in the spec.
-///
-/// We use it for managing attestations that have not been aggregated.
-use super::{AttestationData, Signature};
-use serde_derive::Serialize;
-
-#[derive(arbitrary::Arbitrary, Debug, Clone, PartialEq, Serialize)]
-pub struct FreeAttestation {
-    pub data: AttestationData,
-    pub signature: Signature,
-    #[serde(with = "eth2_serde_utils::quoted_u64")]
-    pub validator_index: u64,
-}

consensus/types/src/lib.rs

@@ -47,7 +47,6 @@ pub mod fork;
 pub mod fork_data;
 pub mod fork_name;
 pub mod fork_versioned_response;
-pub mod free_attestation;
 pub mod graffiti;
 pub mod historical_batch;
 pub mod historical_summary;
@@ -154,7 +153,6 @@ pub use crate::fork_name::{ForkName, InconsistentFork};
 pub use crate::fork_versioned_response::{
     ExecutionOptimisticForkVersionedResponse, ForkVersionDeserialize, ForkVersionedResponse,
 };
-pub use crate::free_attestation::FreeAttestation;
 pub use crate::graffiti::{Graffiti, GRAFFITI_BYTES_LEN};
 pub use crate::historical_batch::HistoricalBatch;
 pub use crate::indexed_attestation::IndexedAttestation;