diff --git a/Cargo.lock b/Cargo.lock index f23777bc4c..adefb20420 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7824,6 +7824,9 @@ name = "smallvec" version = "1.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3c5e1a9a646d36c3599cd173a41282daf47c44583ad367b8e6837255952e5c67" +dependencies = [ + "arbitrary", +] [[package]] name = "snap" @@ -7942,10 +7945,12 @@ dependencies = [ "lazy_static", "lighthouse_metrics", "merkle_proof", + "rand", "rayon", "safe_arith", "smallvec", "ssz_types", + "test_random_derive", "tokio", "tree_hash", "types", diff --git a/Cargo.toml b/Cargo.toml index d67f6edf1d..c09a3af7ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -158,7 +158,7 @@ slog = { version = "2", features = ["max_level_trace", "release_max_level_trace" slog-async = "2" slog-term = "2" sloggers = { version = "2", features = ["json"] } -smallvec = "1.11.2" +smallvec = { version = "1.11.2", features = ["arbitrary"] } snap = "1" ssz_types = "0.6" strum = { version = "0.24", features = ["derive"] } diff --git a/consensus/state_processing/Cargo.toml b/consensus/state_processing/Cargo.toml index be5367eb08..e05c0bcfeb 100644 --- a/consensus/state_processing/Cargo.toml +++ b/consensus/state_processing/Cargo.toml @@ -28,6 +28,8 @@ arbitrary = { workspace = true } lighthouse_metrics = { workspace = true } lazy_static = { workspace = true } derivative = { workspace = true } +test_random_derive = { path = "../../common/test_random_derive" } +rand = { workspace = true } [features] default = ["legacy-arith"] diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index c4b7c6a026..3b20c67b4d 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -7,17 +7,17 @@ use crate::per_block_processing::{ verify_proposer_slashing, }; use crate::VerifySignatures; +use arbitrary::Arbitrary; use derivative::Derivative; use smallvec::{smallvec, SmallVec}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use std::marker::PhantomData; +use test_random_derive::TestRandom; use types::{ - AttesterSlashing, AttesterSlashingBase, AttesterSlashingOnDisk, AttesterSlashingRefOnDisk, -}; -use types::{ - BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, ProposerSlashing, - SignedBlsToExecutionChange, SignedVoluntaryExit, + test_utils::TestRandom, AttesterSlashing, AttesterSlashingBase, AttesterSlashingOnDisk, + AttesterSlashingRefOnDisk, BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, + ProposerSlashing, SignedBlsToExecutionChange, SignedVoluntaryExit, }; const MAX_FORKS_VERIFIED_AGAINST: usize = 2; @@ -39,12 +39,13 @@ pub trait TransformPersist { /// /// The inner `op` field is private, meaning instances of this type can only be constructed /// by calling `validate`. -#[derive(Derivative, Debug, Clone)] +#[derive(Derivative, Debug, Clone, Arbitrary)] #[derivative( PartialEq, Eq, Hash(bound = "T: TransformPersist + std::hash::Hash, E: EthSpec") )] +#[arbitrary(bound = "T: TransformPersist + Arbitrary<'arbitrary>, E: EthSpec")] pub struct SigVerifiedOp { op: T, verified_against: VerifiedAgainst, @@ -53,92 +54,68 @@ pub struct SigVerifiedOp { impl Encode for SigVerifiedOp { fn is_ssz_fixed_len() -> bool { - ::is_ssz_fixed_len() - && ::is_ssz_fixed_len() + as Encode>::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") - } + as Encode>::ssz_fixed_len() } 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(); + let persistable_ref = self.op.as_persistable_ref(); + SigVerifiedOpEncode { + op: persistable_ref, + verified_against: &self.verified_against, + } + .ssz_append(buf) + } + + fn ssz_bytes_len(&self) -> usize { + let persistable_ref = self.op.as_persistable_ref(); + SigVerifiedOpEncode { + op: persistable_ref, + verified_against: &self.verified_against, + } + .ssz_bytes_len() } } impl Decode for SigVerifiedOp { fn is_ssz_fixed_len() -> bool { - ::is_ssz_fixed_len() - && ::is_ssz_fixed_len() + as Decode>::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 - } + as Decode>::ssz_fixed_len() } 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); - + let on_disk = SigVerifiedOpDecode::::from_ssz_bytes(bytes)?; Ok(SigVerifiedOp { - op, - verified_against, + op: T::from_persistable(on_disk.op), + verified_against: on_disk.verified_against, _phantom: PhantomData, }) } } +/// On-disk variant of `SigVerifiedOp` that implements `Encode`. +/// +/// We use separate types for Encode and Decode so we can efficiently handle references: the Encode +/// type contains references, while the Decode type does not. +#[derive(Debug, Encode)] +struct SigVerifiedOpEncode<'a, P: Encode> { + op: P, + verified_against: &'a VerifiedAgainst, +} + +/// On-disk variant of `SigVerifiedOp` that implements `Encode`. +#[derive(Debug, Decode)] +struct SigVerifiedOpDecode { + op: P, + verified_against: VerifiedAgainst, +} + /// 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 @@ -156,7 +133,7 @@ impl Decode for SigVerifiedOp { /// /// We need to store multiple `ForkVersion`s because attester slashings contain two indexed /// attestations which may be signed using different versions. -#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode)] +#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode, TestRandom, Arbitrary)] pub struct VerifiedAgainst { fork_versions: SmallVec<[ForkVersion; MAX_FORKS_VERIFIED_AGAINST]>, } @@ -435,3 +412,56 @@ impl TransformPersist for SignedBlsToExecutionChange { persistable } } + +#[cfg(all(test, not(debug_assertions)))] +mod test { + use super::*; + use types::{ + test_utils::{SeedableRng, TestRandom, XorShiftRng}, + MainnetEthSpec, + }; + + type E = MainnetEthSpec; + + fn roundtrip_test() { + let runs = 10; + let mut rng = XorShiftRng::seed_from_u64(0xff0af5a356af1123); + + for _ in 0..runs { + let op = T::random_for_test(&mut rng); + let verified_against = VerifiedAgainst::random_for_test(&mut rng); + + let verified_op = SigVerifiedOp { + op, + verified_against, + _phantom: PhantomData::, + }; + + let serialized = verified_op.as_ssz_bytes(); + let deserialized = SigVerifiedOp::from_ssz_bytes(&serialized).unwrap(); + let reserialized = deserialized.as_ssz_bytes(); + assert_eq!(verified_op, deserialized); + assert_eq!(serialized, reserialized); + } + } + + #[test] + fn sig_verified_op_exit_roundtrip() { + roundtrip_test::(); + } + + #[test] + fn proposer_slashing_roundtrip() { + roundtrip_test::(); + } + + #[test] + fn attester_slashing_roundtrip() { + roundtrip_test::>(); + } + + #[test] + fn bls_to_execution_roundtrip() { + roundtrip_test::(); + } +} diff --git a/consensus/types/src/attester_slashing.rs b/consensus/types/src/attester_slashing.rs index a8d4e6989c..c8e2fb4f82 100644 --- a/consensus/types/src/attester_slashing.rs +++ b/consensus/types/src/attester_slashing.rs @@ -3,6 +3,7 @@ use crate::indexed_attestation::{ }; use crate::{test_utils::TestRandom, EthSpec}; use derivative::Derivative; +use rand::{Rng, RngCore}; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use superstruct::superstruct; @@ -160,6 +161,16 @@ impl AttesterSlashing { } } +impl TestRandom for AttesterSlashing { + fn random_for_test(rng: &mut impl RngCore) -> Self { + if rng.gen_bool(0.5) { + AttesterSlashing::Base(AttesterSlashingBase::random_for_test(rng)) + } else { + AttesterSlashing::Electra(AttesterSlashingElectra::random_for_test(rng)) + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/test_utils/test_random.rs b/consensus/types/src/test_utils/test_random.rs index 72a7a036cc..00355779d2 100644 --- a/consensus/types/src/test_utils/test_random.rs +++ b/consensus/types/src/test_utils/test_random.rs @@ -2,6 +2,7 @@ use crate::*; use rand::RngCore; use rand::SeedableRng; use rand_xorshift::XorShiftRng; +use smallvec::{smallvec, SmallVec}; use std::marker::PhantomData; use std::sync::Arc; @@ -118,6 +119,21 @@ where } } +impl TestRandom for SmallVec<[U; N]> +where + U: TestRandom, +{ + fn random_for_test(rng: &mut impl RngCore) -> Self { + let mut output = smallvec![]; + + for _ in 0..(usize::random_for_test(rng) % 4) { + output.push(::random_for_test(rng)); + } + + output + } +} + macro_rules! impl_test_random_for_u8_array { ($len: expr) => { impl TestRandom for [u8; $len] {