From 3a7b0360ed99454b3a8e8372e9169b8bdb27ef98 Mon Sep 17 00:00:00 2001 From: Mark Mackey Date: Sat, 27 Apr 2024 13:04:48 -0500 Subject: [PATCH] Stop Encode/Decode Bounds from Propagating Out --- beacon_node/beacon_chain/src/beacon_chain.rs | 10 +- .../beacon_chain/src/observed_operations.rs | 14 +- .../beacon_chain/src/validator_monitor.rs | 6 +- beacon_node/operation_pool/src/lib.rs | 11 +- beacon_node/operation_pool/src/persistence.rs | 2 +- consensus/state_processing/src/lib.rs | 2 +- .../per_block_processing/signature_sets.rs | 4 +- .../verify_attester_slashing.rs | 12 +- .../state_processing/src/verify_operation.rs | 167 +++++++++++++++++- consensus/types/src/attester_slashing.rs | 30 +++- consensus/types/src/lib.rs | 2 +- 11 files changed, 212 insertions(+), 48 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index ef08c8aac01..cdccb896f5b 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -419,7 +419,7 @@ pub struct BeaconChain { pub observed_proposer_slashings: Mutex>, /// Maintains a record of which validators we've seen attester slashings for. pub observed_attester_slashings: - Mutex, T::EthSpec>>, + Mutex, T::EthSpec>>, /// Maintains a record of which validators we've seen BLS to execution changes for. pub observed_bls_to_execution_changes: Mutex>, @@ -2443,10 +2443,10 @@ impl BeaconChain { pub fn verify_attester_slashing_for_gossip( &self, attester_slashing: AttesterSlashing, - ) -> Result, T::EthSpec>, Error> { + ) -> Result, T::EthSpec>, Error> { let wall_clock_state = self.wall_clock_state()?; Ok(self.observed_attester_slashings.lock().verify_and_observe( - attester_slashing.into(), + attester_slashing, &wall_clock_state, &self.spec, )?) @@ -2458,7 +2458,7 @@ impl BeaconChain { /// 2. Add it to the op pool. pub fn import_attester_slashing( &self, - attester_slashing: SigVerifiedOp, T::EthSpec>, + attester_slashing: SigVerifiedOp, T::EthSpec>, ) { // Add to fork choice. self.canonical_head @@ -4941,7 +4941,7 @@ impl BeaconChain { }); attester_slashings.retain(|slashing| { - AttesterSlashingOnDisk::from(slashing.clone()) + AttesterSlashing::from(slashing.clone()) .validate(&state, &self.spec) .map_err(|e| { warn!( diff --git a/beacon_node/beacon_chain/src/observed_operations.rs b/beacon_node/beacon_chain/src/observed_operations.rs index d864e620ed3..931ab20a53a 100644 --- a/beacon_node/beacon_chain/src/observed_operations.rs +++ b/beacon_node/beacon_chain/src/observed_operations.rs @@ -1,11 +1,10 @@ use derivative::Derivative; use smallvec::{smallvec, SmallVec}; -use ssz::{Decode, Encode}; -use state_processing::{SigVerifiedOp, VerifyOperation, VerifyOperationAt}; +use state_processing::{SigVerifiedOp, TransformPersist, VerifyOperation, VerifyOperationAt}; use std::collections::HashSet; use std::marker::PhantomData; use types::{ - AttesterSlashingOnDisk, BeaconState, ChainSpec, Epoch, EthSpec, ForkName, ProposerSlashing, + AttesterSlashing, BeaconState, ChainSpec, Epoch, EthSpec, ForkName, ProposerSlashing, SignedBlsToExecutionChange, SignedVoluntaryExit, Slot, }; @@ -34,7 +33,7 @@ pub struct ObservedOperations, E: EthSpec> { /// Was the observed operation new and valid for further processing, or a useless duplicate? #[derive(Debug, PartialEq, Eq, Clone)] -pub enum ObservationOutcome { +pub enum ObservationOutcome { New(SigVerifiedOp), AlreadyKnown, } @@ -59,16 +58,15 @@ impl ObservableOperation for ProposerSlashing { } } -impl ObservableOperation for AttesterSlashingOnDisk { +impl ObservableOperation for AttesterSlashing { fn observed_validators(&self) -> SmallVec<[u64; SMALL_VEC_SIZE]> { - let slashing_ref = self.to_ref(); - let attestation_1_indices = slashing_ref + let attestation_1_indices = self .attestation_1() .attesting_indices .iter() .copied() .collect::>(); - let attestation_2_indices = slashing_ref + let attestation_2_indices = self .attestation_2() .attesting_indices .iter() diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index dc5cc2244ac..2a77baa667d 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -1799,16 +1799,16 @@ impl ValidatorMonitor { } fn register_attester_slashing(&self, src: &str, slashing: AttesterSlashingRef<'_, E>) { - let data = &slashing.attestation_1.data; + let data = &slashing.attestation_1().data; let attestation_1_indices: HashSet = slashing - .attestation_1 + .attestation_1() .attesting_indices .iter() .copied() .collect(); slashing - .attestation_2 + .attestation_2() .attesting_indices .iter() .filter(|index| attestation_1_indices.contains(index)) diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index f6fd2a96632..00f46df6a9b 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -39,10 +39,9 @@ use std::marker::PhantomData; use std::ptr; use types::{ sync_aggregate::Error as SyncAggregateError, typenum::Unsigned, AbstractExecPayload, - Attestation, AttestationData, AttesterSlashing, AttesterSlashingOnDisk, BeaconState, - BeaconStateError, ChainSpec, Epoch, EthSpec, ProposerSlashing, SignedBeaconBlock, - SignedBlsToExecutionChange, SignedVoluntaryExit, Slot, SyncAggregate, - SyncCommitteeContribution, Validator, + Attestation, AttestationData, AttesterSlashing, BeaconState, BeaconStateError, ChainSpec, + Epoch, EthSpec, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange, + SignedVoluntaryExit, Slot, SyncAggregate, SyncCommitteeContribution, Validator, }; type SyncContributions = RwLock>>>; @@ -54,7 +53,7 @@ pub struct OperationPool { /// Map from sync aggregate ID to the best `SyncCommitteeContribution`s seen for that ID. sync_contributions: SyncContributions, /// Set of attester slashings, and the fork version they were verified against. - attester_slashings: RwLock, E>>>, + attester_slashings: RwLock, E>>>, /// Map from proposer index to slashing. proposer_slashings: RwLock>>, /// Map from exiting validator to their exit data. @@ -367,7 +366,7 @@ impl OperationPool { /// Insert an attester slashing into the pool. pub fn insert_attester_slashing( &self, - verified_slashing: SigVerifiedOp, E>, + verified_slashing: SigVerifiedOp, E>, ) { self.attester_slashings.write().insert(verified_slashing); } diff --git a/beacon_node/operation_pool/src/persistence.rs b/beacon_node/operation_pool/src/persistence.rs index ceab5cbfe9c..99545edff5f 100644 --- a/beacon_node/operation_pool/src/persistence.rs +++ b/beacon_node/operation_pool/src/persistence.rs @@ -42,7 +42,7 @@ pub struct PersistedOperationPool { /// TODO(electra): we've made a DB change here!!! /// Attester slashings. #[superstruct(only(V12, V14, V15))] - pub attester_slashings: Vec, E>>, + pub attester_slashings: Vec, E>>, /// [DEPRECATED] Proposer slashings. #[superstruct(only(V5))] pub proposer_slashings_v5: Vec, diff --git a/consensus/state_processing/src/lib.rs b/consensus/state_processing/src/lib.rs index 74f9d84bb11..adabf6862d3 100644 --- a/consensus/state_processing/src/lib.rs +++ b/consensus/state_processing/src/lib.rs @@ -45,4 +45,4 @@ pub use per_epoch_processing::{ }; pub use per_slot_processing::{per_slot_processing, Error as SlotProcessingError}; pub use types::{EpochCache, EpochCacheError, EpochCacheKey}; -pub use verify_operation::{SigVerifiedOp, VerifyOperation, VerifyOperationAt}; +pub use verify_operation::{SigVerifiedOp, TransformPersist, VerifyOperation, VerifyOperationAt}; diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index 4f23e90bbcb..4032b6fdda9 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -347,14 +347,14 @@ where state, get_pubkey.clone(), &attester_slashing.attestation_1().signature, - &attester_slashing.attestation_1(), + attester_slashing.attestation_1(), spec, )?, indexed_attestation_signature_set( state, get_pubkey, &attester_slashing.attestation_2().signature, - &attester_slashing.attestation_2(), + attester_slashing.attestation_2(), spec, )?, )) diff --git a/consensus/state_processing/src/per_block_processing/verify_attester_slashing.rs b/consensus/state_processing/src/per_block_processing/verify_attester_slashing.rs index bad0ae52e89..7430a030e2d 100644 --- a/consensus/state_processing/src/per_block_processing/verify_attester_slashing.rs +++ b/consensus/state_processing/src/per_block_processing/verify_attester_slashing.rs @@ -16,9 +16,9 @@ fn error(reason: Invalid) -> BlockOperationError { /// Returns `Ok(indices)` with `indices` being a non-empty vec of validator indices in ascending /// order if the `AttesterSlashing` is valid. Otherwise returns `Err(e)` with the reason for /// invalidity. -pub fn verify_attester_slashing<'a, E: EthSpec>( +pub fn verify_attester_slashing( state: &BeaconState, - attester_slashing: AttesterSlashingRef<'a, E>, + attester_slashing: AttesterSlashingRef<'_, E>, verify_signatures: VerifySignatures, spec: &ChainSpec, ) -> Result> { @@ -43,9 +43,9 @@ pub fn verify_attester_slashing<'a, E: EthSpec>( /// For a given attester slashing, return the indices able to be slashed in ascending order. /// /// Returns Ok(indices) if `indices.len() > 0` -pub fn get_slashable_indices<'a, E: EthSpec>( +pub fn get_slashable_indices( state: &BeaconState, - attester_slashing: AttesterSlashingRef<'a, E>, + attester_slashing: AttesterSlashingRef<'_, E>, ) -> Result> { get_slashable_indices_modular(state, attester_slashing, |_, validator| { validator.is_slashable_at(state.current_epoch()) @@ -54,9 +54,9 @@ pub fn get_slashable_indices<'a, E: EthSpec>( /// Same as `gather_attester_slashing_indices` but allows the caller to specify the criteria /// for determining whether a given validator should be considered slashable. -pub fn get_slashable_indices_modular<'a, F, E: EthSpec>( +pub fn get_slashable_indices_modular( state: &BeaconState, - attester_slashing: AttesterSlashingRef<'a, E>, + attester_slashing: AttesterSlashingRef<'_, E>, is_slashable: F, ) -> Result> where diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index a1acdf88cc1..c7f3d6a96df 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -12,7 +12,7 @@ use smallvec::{smallvec, SmallVec}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use std::marker::PhantomData; -use types::AttesterSlashingOnDisk; +use types::{AttesterSlashing, AttesterSlashingOnDisk, AttesterSlashingRefOnDisk}; use types::{ BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, ProposerSlashing, SignedBlsToExecutionChange, SignedVoluntaryExit, @@ -20,23 +20,124 @@ use types::{ const MAX_FORKS_VERIFIED_AGAINST: usize = 2; +pub trait TransformPersist { + type Persistable: Encode + Decode; + type PersistableRef<'a>: Encode + where + Self: 'a; + + /// Returns a reference to the object in a form that implements `Encode` + fn as_persistable_ref(&self) -> Self::PersistableRef<'_>; + + /// Converts the object back into its original form. + fn from_persistable(persistable: Self::Persistable) -> Self; +} + /// Wrapper around an operation type that acts as proof that its signature has been checked. /// /// The inner `op` field is private, meaning instances of this type can only be constructed /// by calling `validate`. -#[derive(Derivative, Debug, Clone, Encode, Decode)] +#[derive(Derivative, Debug, Clone)] #[derivative( PartialEq, Eq, - Hash(bound = "T: Encode + Decode + std::hash::Hash, E: EthSpec") + Hash(bound = "T: TransformPersist + std::hash::Hash, E: EthSpec") )] -pub struct SigVerifiedOp { +pub struct SigVerifiedOp { op: T, verified_against: VerifiedAgainst, - #[ssz(skip_serializing, skip_deserializing)] + //#[ssz(skip_serializing, skip_deserializing)] _phantom: PhantomData, } +impl Encode for SigVerifiedOp { + fn is_ssz_fixed_len() -> bool { + ::is_ssz_fixed_len() + && ::is_ssz_fixed_len() + } + + #[allow(clippy::expect_used)] + fn ssz_fixed_len() -> usize { + if ::is_ssz_fixed_len() { + ::ssz_fixed_len() + .checked_add(::ssz_fixed_len()) + .expect("encode ssz_fixed_len length overflow") + } else { + ssz::BYTES_PER_LENGTH_OFFSET + } + } + + #[allow(clippy::expect_used)] + fn ssz_bytes_len(&self) -> usize { + if ::is_ssz_fixed_len() { + ::ssz_fixed_len() + } else { + let persistable = self.op.as_persistable_ref(); + persistable + .ssz_bytes_len() + .checked_add(self.verified_against.ssz_bytes_len()) + .expect("ssz_bytes_len length overflow") + } + } + + fn ssz_append(&self, buf: &mut Vec) { + let mut encoder = ssz::SszEncoder::container(buf, ::ssz_fixed_len()); + let persistable = self.op.as_persistable_ref(); + encoder.append(&persistable); + encoder.append(&self.verified_against); + encoder.finalize(); + } +} + +impl Decode for SigVerifiedOp { + fn is_ssz_fixed_len() -> bool { + ::is_ssz_fixed_len() + && ::is_ssz_fixed_len() + } + + #[allow(clippy::expect_used)] + fn ssz_fixed_len() -> usize { + if ::is_ssz_fixed_len() { + ::ssz_fixed_len() + .checked_add(::ssz_fixed_len()) + .expect("decode ssz_fixed_len length overflow") + } else { + ssz::BYTES_PER_LENGTH_OFFSET + } + } + + fn from_ssz_bytes(bytes: &[u8]) -> Result { + let mut builder = ssz::SszDecoderBuilder::new(bytes); + + // Register types based on whether they are fixed or variable length + if ::is_ssz_fixed_len() { + builder.register_type::()?; + } else { + builder.register_anonymous_variable_length_item()?; + } + + if ::is_ssz_fixed_len() { + builder.register_type::()?; + } else { + builder.register_anonymous_variable_length_item()?; + } + + let mut decoder = builder.build()?; + // Decode each component + let persistable: T::Persistable = decoder.decode_next()?; + let verified_against: VerifiedAgainst = decoder.decode_next()?; + + // Use TransformPersist to convert persistable back into the original type + let op = T::from_persistable(persistable); + + Ok(SigVerifiedOp { + op, + verified_against, + _phantom: PhantomData, + }) + } +} + /// Information about the fork versions that this message was verified against. /// /// In general it is not safe to assume that a `SigVerifiedOp` constructed at some point in the past @@ -110,7 +211,7 @@ where } /// Trait for operations that can be verified and transformed into a `SigVerifiedOp`. -pub trait VerifyOperation: Encode + Decode + Sized { +pub trait VerifyOperation: TransformPersist + Sized { type Error; fn validate( @@ -145,7 +246,7 @@ impl VerifyOperation for SignedVoluntaryExit { } } -impl VerifyOperation for AttesterSlashingOnDisk { +impl VerifyOperation for AttesterSlashing { type Error = AttesterSlashingValidationError; fn validate( @@ -238,3 +339,55 @@ impl VerifyOperationAt for SignedVoluntaryExit { Ok(SigVerifiedOp::new(self, state)) } } + +impl TransformPersist for SignedVoluntaryExit { + type Persistable = Self; + type PersistableRef<'a> = &'a Self; + + fn as_persistable_ref(&self) -> Self::PersistableRef<'_> { + self + } + + fn from_persistable(persistable: Self::Persistable) -> Self { + persistable + } +} + +impl TransformPersist for AttesterSlashing { + type Persistable = AttesterSlashingOnDisk; + type PersistableRef<'a> = AttesterSlashingRefOnDisk<'a, E>; + + fn as_persistable_ref(&self) -> Self::PersistableRef<'_> { + self.to_ref().into() + } + + fn from_persistable(persistable: Self::Persistable) -> Self { + persistable.into() + } +} + +impl TransformPersist for ProposerSlashing { + type Persistable = Self; + type PersistableRef<'a> = &'a Self; + + fn as_persistable_ref(&self) -> Self::PersistableRef<'_> { + self + } + + fn from_persistable(persistable: Self::Persistable) -> Self { + persistable + } +} + +impl TransformPersist for SignedBlsToExecutionChange { + type Persistable = Self; + type PersistableRef<'a> = &'a Self; + + fn as_persistable_ref(&self) -> Self::PersistableRef<'_> { + self + } + + fn from_persistable(persistable: Self::Persistable) -> Self { + persistable + } +} diff --git a/consensus/types/src/attester_slashing.rs b/consensus/types/src/attester_slashing.rs index 66b546473e5..aa727984586 100644 --- a/consensus/types/src/attester_slashing.rs +++ b/consensus/types/src/attester_slashing.rs @@ -51,6 +51,13 @@ pub enum AttesterSlashingOnDisk { Electra(AttesterSlashingElectra), } +#[derive(Debug, Clone, Encode)] +#[ssz(enum_behaviour = "union")] +pub enum AttesterSlashingRefOnDisk<'a, E: EthSpec> { + Base(&'a AttesterSlashingBase), + Electra(&'a AttesterSlashingElectra), +} + impl From> for AttesterSlashingOnDisk { fn from(attester_slashing: AttesterSlashing) -> Self { match attester_slashing { @@ -69,19 +76,26 @@ impl From> for AttesterSlashing { } } -impl AttesterSlashingOnDisk { - pub fn to_ref(&self) -> AttesterSlashingRef<'_, E> { - match self { - AttesterSlashingOnDisk::Base(attester_slashing) => { - AttesterSlashingRef::Base(attester_slashing) - } - AttesterSlashingOnDisk::Electra(attester_slashing) => { - AttesterSlashingRef::Electra(attester_slashing) +impl<'a, E: EthSpec> From> for AttesterSlashingRef<'a, E> { + fn from(attester_slashing: AttesterSlashingRefOnDisk<'a, E>) -> Self { + match attester_slashing { + AttesterSlashingRefOnDisk::Base(attester_slashing) => Self::Base(attester_slashing), + AttesterSlashingRefOnDisk::Electra(attester_slashing) => { + Self::Electra(attester_slashing) } } } } +impl<'a, E: EthSpec> From> for AttesterSlashingRefOnDisk<'a, E> { + fn from(attester_slashing: AttesterSlashingRef<'a, E>) -> Self { + match attester_slashing { + AttesterSlashingRef::Base(attester_slashing) => Self::Base(attester_slashing), + AttesterSlashingRef::Electra(attester_slashing) => Self::Electra(attester_slashing), + } + } +} + impl<'a, E: EthSpec> AttesterSlashingRef<'a, E> { pub fn clone_as_attester_slashing(self) -> AttesterSlashing { match self { diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index c443261bead..5300fbcf9a1 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -120,7 +120,7 @@ pub use crate::attestation_data::AttestationData; pub use crate::attestation_duty::AttestationDuty; pub use crate::attester_slashing::{ AttesterSlashing, AttesterSlashingBase, AttesterSlashingElectra, AttesterSlashingOnDisk, - AttesterSlashingRef, + AttesterSlashingRef, AttesterSlashingRefOnDisk, }; pub use crate::beacon_block::{ BeaconBlock, BeaconBlockAltair, BeaconBlockBase, BeaconBlockCapella, BeaconBlockDeneb,