From 11dc5002957b4c25fa9b568a29fc5000cf0bd714 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 18 Feb 2023 16:10:21 +1100 Subject: [PATCH 1/6] Add extra encoding/decoding tests --- consensus/types/src/beacon_block.rs | 98 +++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-) diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index f960b21178f..60dd781a67f 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"); + } } } From fcad6bd7a7d0e67831ff3b385ae45da7a04962cb Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 18 Feb 2023 16:12:27 +1100 Subject: [PATCH 2/6] Remove TODO The method LGTM --- consensus/types/src/beacon_state.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index e70b8842758..c98df48d14e 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 { From 08d5bc6d6bcda8dbc98c0e8faa3e5876d4eab367 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 18 Feb 2023 16:28:36 +1100 Subject: [PATCH 3/6] Remove `FreeAttestation` This is an ancient relic, I'm surprised it still existed! --- consensus/types/src/free_attestation.rs | 13 ------------- consensus/types/src/lib.rs | 2 -- 2 files changed, 15 deletions(-) delete mode 100644 consensus/types/src/free_attestation.rs diff --git a/consensus/types/src/free_attestation.rs b/consensus/types/src/free_attestation.rs deleted file mode 100644 index dd3782d3ce1..00000000000 --- 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 8d9156ff5d3..2926a434b10 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; From 110f853715b03061ec92fe27f546644d2da5af9e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Feb 2023 15:31:47 +1100 Subject: [PATCH 4/6] 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. --- consensus/state_processing/src/per_block_processing.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index e12fb59565e..4f686200b01 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(()) } From e7d93e6cc7cf1b92dd5a9e1966ce47d4078121eb Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Feb 2023 12:11:14 +1100 Subject: [PATCH 5/6] 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. --- .../src/bls_to_execution_changes.rs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/beacon_node/operation_pool/src/bls_to_execution_changes.rs b/beacon_node/operation_pool/src/bls_to_execution_changes.rs index c73666e1458..9978bd5a986 100644 --- a/beacon_node/operation_pool/src/bls_to_execution_changes.rs +++ b/beacon_node/operation_pool/src/bls_to_execution_changes.rs @@ -106,6 +106,16 @@ impl BlsToExecutionChanges { spec: &ChainSpec, ) { let mut validator_indices_pruned = vec![]; + let recently_changed_indices: HashSet = head_block + .message() + .body() + .bls_to_execution_changes() + .map_or(HashSet::default(), |recent_changes| { + recent_changes + .iter() + .map(|c| c.message.validator_index) + .collect() + }); self.queue.retain(|address_change| { let validator_index = address_change.as_inner().message.validator_index; @@ -114,15 +124,7 @@ impl BlsToExecutionChanges { .get(validator_index as usize) .map_or(true, |validator| { let prune = validator.has_eth1_withdrawal_credential(spec) - && head_block - .message() - .body() - .bls_to_execution_changes() - .map_or(true, |recent_changes| { - !recent_changes - .iter() - .any(|c| c.message.validator_index == validator_index) - }); + && !recently_changed_indices.contains(&validator_index); if prune { validator_indices_pruned.push(validator_index); } From f5581524d53b9e038ed36aaf17d58c95827bfcf3 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Feb 2023 15:26:15 +1100 Subject: [PATCH 6/6] Revert "Reduce big-O complexity of address change pruning" This reverts commit e7d93e6cc7cf1b92dd5a9e1966ce47d4078121eb. --- .../src/bls_to_execution_changes.rs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/beacon_node/operation_pool/src/bls_to_execution_changes.rs b/beacon_node/operation_pool/src/bls_to_execution_changes.rs index 9978bd5a986..c73666e1458 100644 --- a/beacon_node/operation_pool/src/bls_to_execution_changes.rs +++ b/beacon_node/operation_pool/src/bls_to_execution_changes.rs @@ -106,16 +106,6 @@ impl BlsToExecutionChanges { spec: &ChainSpec, ) { let mut validator_indices_pruned = vec![]; - let recently_changed_indices: HashSet = head_block - .message() - .body() - .bls_to_execution_changes() - .map_or(HashSet::default(), |recent_changes| { - recent_changes - .iter() - .map(|c| c.message.validator_index) - .collect() - }); self.queue.retain(|address_change| { let validator_index = address_change.as_inner().message.validator_index; @@ -124,7 +114,15 @@ impl BlsToExecutionChanges { .get(validator_index as usize) .map_or(true, |validator| { let prune = validator.has_eth1_withdrawal_credential(spec) - && !recently_changed_indices.contains(&validator_index); + && head_block + .message() + .body() + .bls_to_execution_changes() + .map_or(true, |recent_changes| { + !recent_changes + .iter() + .any(|c| c.message.validator_index == validator_index) + }); if prune { validator_indices_pruned.push(validator_index); }