From b72f273e47c04d14d0fdb32ca7f451312ca852c6 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Feb 2023 15:33:27 +1100 Subject: [PATCH] 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. --- .../src/per_block_processing.rs | 6 +- consensus/types/src/beacon_block.rs | 98 ++++++++++++++++++- consensus/types/src/beacon_state.rs | 1 - consensus/types/src/free_attestation.rs | 13 --- consensus/types/src/lib.rs | 2 - 5 files changed, 99 insertions(+), 21 deletions(-) delete mode 100644 consensus/types/src/free_attestation.rs diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index e12fb5956..4f686200b 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -180,7 +180,11 @@ pub fn per_block_processing>( )?; } - 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(()) } diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index f960b2117..60dd781a6 100644 --- a/consensus/types/src/beacon_block.rs +++ b/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"); + } } } diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index e70b88427..c98df48d1 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -710,7 +710,6 @@ impl BeaconState { .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, Error> { match self { diff --git a/consensus/types/src/free_attestation.rs b/consensus/types/src/free_attestation.rs deleted file mode 100644 index dd3782d3c..000000000 --- a/consensus/types/src/free_attestation.rs +++ /dev/null @@ -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, -} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 8d9156ff5..2926a434b 100644 --- a/consensus/types/src/lib.rs +++ b/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;