Fix Arbitrary implementations (#3867)

* Fix Arbitrary implementations

* Remove remaining vestiges of arbitrary-fuzz

* Remove FIXME

* Clippy

Author: michaelsproul

Cargo.lock

@@ -100,6 +100,7 @@ eth2_hashing = { path = "crypto/eth2_hashing" }
 tree_hash = { path = "consensus/tree_hash" }
 tree_hash_derive = { path = "consensus/tree_hash_derive" }
 eth2_serde_utils = { path = "consensus/serde_utils" }
+arbitrary = { git = "https://github.com/michaelsproul/arbitrary", rev="a572fd8743012a4f1ada5ee5968b1b3619c427ba" }
 
 [profile.maxperf]
 inherits = "release"

Cargo.toml

@@ -660,7 +660,7 @@ impl<N: 'static + Unsigned> arbitrary::Arbitrary<'_> for Bitfield<Fixed<N>> {
         let size = N::to_usize();
         let mut vec = smallvec![0u8; size];
         u.fill_buffer(&mut vec)?;
-        Ok(Self::from_bytes(vec).map_err(|_| arbitrary::Error::IncorrectFormat)?)
+        Self::from_bytes(vec).map_err(|_| arbitrary::Error::IncorrectFormat)
     }
 }
 
@@ -672,7 +672,7 @@ impl<N: 'static + Unsigned> arbitrary::Arbitrary<'_> for Bitfield<Variable<N>> {
         let size = std::cmp::min(rand, max_size);
         let mut vec = smallvec![0u8; size];
         u.fill_buffer(&mut vec)?;
-        Ok(Self::from_bytes(vec).map_err(|_| arbitrary::Error::IncorrectFormat)?)
+        Self::from_bytes(vec).map_err(|_| arbitrary::Error::IncorrectFormat)
     }
 }
 

consensus/ssz_types/src/bitfield.rs

@@ -291,7 +291,7 @@ impl<'a, T: arbitrary::Arbitrary<'a>, N: 'static + Unsigned> arbitrary::Arbitrar
         for _ in 0..size {
             vec.push(<T>::arbitrary(u)?);
         }
-        Ok(Self::new(vec).map_err(|_| arbitrary::Error::IncorrectFormat)?)
+        Self::new(vec).map_err(|_| arbitrary::Error::IncorrectFormat)
     }
 }
 

consensus/ssz_types/src/fixed_vector.rs

@@ -273,7 +273,7 @@ impl<'a, T: arbitrary::Arbitrary<'a>, N: 'static + Unsigned> arbitrary::Arbitrar
         for _ in 0..size {
             vec.push(<T>::arbitrary(u)?);
         }
-        Ok(Self::new(vec).map_err(|_| arbitrary::Error::IncorrectFormat)?)
+        Self::new(vec).map_err(|_| arbitrary::Error::IncorrectFormat)
     }
 }
 

consensus/ssz_types/src/variable_list.rs

@@ -11,11 +11,11 @@ harness = false
 [dependencies]
 serde-big-array = {version = "0.3.2", features = ["const-generics"]}
 merkle_proof = { path = "../../consensus/merkle_proof" }
-bls = { path = "../../crypto/bls" }
+bls = { path = "../../crypto/bls", features = ["arbitrary"] }
 compare_fields = { path = "../../common/compare_fields" }
 compare_fields_derive = { path = "../../common/compare_fields_derive" }
 eth2_interop_keypairs = { path = "../../common/eth2_interop_keypairs" }
-ethereum-types = "0.12.1"
+ethereum-types = { version = "0.12.1", features = ["arbitrary"] }
 eth2_hashing = "0.3.0"
 hex = "0.4.2"
 int_to_bytes = { path = "../int_to_bytes" }
@@ -26,20 +26,22 @@ safe_arith = { path = "../safe_arith" }
 serde = {version = "1.0.116" , features = ["rc"] }
 serde_derive = "1.0.116"
 slog = "2.5.2"
-eth2_ssz = "0.4.1"
+eth2_ssz = { version = "0.4.1", features = ["arbitrary"] }
 eth2_ssz_derive = "0.3.1"
-eth2_ssz_types = "0.2.2"
-swap_or_not_shuffle = { path = "../swap_or_not_shuffle" }
+eth2_ssz_types = { version = "0.2.2", features = ["arbitrary"] }
+swap_or_not_shuffle = { path = "../swap_or_not_shuffle", features = ["arbitrary"] }
 test_random_derive = { path = "../../common/test_random_derive" }
-tree_hash = "0.4.1"
+tree_hash = { version = "0.4.1", features = ["arbitrary"] }
 tree_hash_derive = "0.4.0"
 rand_xorshift = "0.3.0"
 cached_tree_hash = { path = "../cached_tree_hash" }
 serde_yaml = "0.8.13"
 tempfile = "3.1.0"
 derivative = "2.1.1"
 rusqlite = { version = "0.25.3", features = ["bundled"], optional = true }
-arbitrary = { version = "1.0", features = ["derive"], optional = true }
+# The arbitrary dependency is enabled by default since Capella to avoid complexity introduced by
+# `AbstractExecPayload`
+arbitrary = { version = "1.0", features = ["derive"] }
 eth2_serde_utils = "0.1.1"
 regex = "1.5.5"
 lazy_static = "1.4.0"
@@ -63,12 +65,6 @@ default = ["sqlite", "legacy-arith"]
 # Allow saturating arithmetic on slots and epochs. Enabled by default, but deprecated.
 legacy-arith = []
 sqlite = ["rusqlite"]
-arbitrary-fuzz = [
-  "arbitrary",
-  "ethereum-types/arbitrary",
-  "bls/arbitrary",
-  "eth2_ssz/arbitrary",
-  "eth2_ssz_types/arbitrary",
-  "swap_or_not_shuffle/arbitrary",
-  "tree_hash/arbitrary",
-]
+# The `arbitrary-fuzz` feature is a no-op provided for backwards compatibility.
+# For simplicity `Arbitrary` is now derived regardless of the feature's presence.
+arbitrary-fuzz = []

consensus/types/Cargo.toml

@@ -11,9 +11,20 @@ use tree_hash_derive::TreeHash;
 /// A Validators aggregate attestation and selection proof.
 ///
 /// Spec v0.12.1
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
-#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TestRandom, TreeHash)]
+#[derive(
+    arbitrary::Arbitrary,
+    Debug,
+    Clone,
+    PartialEq,
+    Serialize,
+    Deserialize,
+    Encode,
+    Decode,
+    TestRandom,
+    TreeHash,
+)]
 #[serde(bound = "T: EthSpec")]
+#[arbitrary(bound = "T: EthSpec")]
 pub struct AggregateAndProof<T: EthSpec> {
     /// The index of the validator that created the attestation.
     #[serde(with = "eth2_serde_utils::quoted_u64")]

consensus/types/src/aggregate_and_proof.rs

@@ -23,12 +23,21 @@ pub enum Error {
 /// Details an attestation that can be slashable.
 ///
 /// Spec v0.12.1
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
 #[derive(
-    Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, Derivative,
+    arbitrary::Arbitrary,
+    Debug,
+    Clone,
+    Serialize,
+    Deserialize,
+    Encode,
+    Decode,
+    TreeHash,
+    TestRandom,
+    Derivative,
 )]
 #[derivative(PartialEq, Hash(bound = "T: EthSpec"))]
 #[serde(bound = "T: EthSpec")]
+#[arbitrary(bound = "T: EthSpec")]
 pub struct Attestation<T: EthSpec> {
     pub aggregation_bits: BitList<T::MaxValidatorsPerCommittee>,
     pub data: AttestationData,

consensus/types/src/attestation.rs

@@ -10,8 +10,8 @@ use tree_hash_derive::TreeHash;
 /// The data upon which an attestation is based.
 ///
 /// Spec v0.12.1
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
 #[derive(
+    arbitrary::Arbitrary,
     Debug,
     Clone,
     PartialEq,

consensus/types/src/attestation_data.rs

@@ -1,8 +1,7 @@
 use crate::*;
 use serde_derive::{Deserialize, Serialize};
 
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
-#[derive(Debug, PartialEq, Clone, Copy, Default, Serialize, Deserialize)]
+#[derive(arbitrary::Arbitrary, Debug, PartialEq, Clone, Copy, Default, Serialize, Deserialize)]
 pub struct AttestationDuty {
     /// The slot during which the attester must attest.
     pub slot: Slot,

consensus/types/src/attestation_duty.rs

@@ -9,12 +9,21 @@ use tree_hash_derive::TreeHash;
 /// Two conflicting attestations.
 ///
 /// Spec v0.12.1
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
 #[derive(
-    Derivative, Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom,
+    Derivative,
+    Debug,
+    Clone,
+    Serialize,
+    Deserialize,
+    Encode,
+    Decode,
+    TreeHash,
+    TestRandom,
+    arbitrary::Arbitrary,
 )]
 #[derivative(PartialEq, Eq, Hash(bound = "T: EthSpec"))]
 #[serde(bound = "T: EthSpec")]
+#[arbitrary(bound = "T: EthSpec")]
 pub struct AttesterSlashing<T: EthSpec> {
     pub attestation_1: IndexedAttestation<T>,
     pub attestation_2: IndexedAttestation<T>,

consensus/types/src/attester_slashing.rs

@@ -29,10 +29,14 @@ use tree_hash_derive::TreeHash;
             TreeHash,
             TestRandom,
             Derivative,
+            arbitrary::Arbitrary
         ),
-        derivative(PartialEq, Hash(bound = "T: EthSpec, Payload: ExecPayload<T>")),
-        serde(bound = "T: EthSpec, Payload: ExecPayload<T>", deny_unknown_fields),
-        cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary)),
+        derivative(PartialEq, Hash(bound = "T: EthSpec, Payload: AbstractExecPayload<T>")),
+        serde(
+            bound = "T: EthSpec, Payload: AbstractExecPayload<T>",
+            deny_unknown_fields
+        ),
+        arbitrary(bound = "T: EthSpec, Payload: AbstractExecPayload<T>"),
     ),
     ref_attributes(
         derive(Debug, PartialEq, TreeHash),
@@ -41,11 +45,13 @@ use tree_hash_derive::TreeHash;
     map_ref_into(BeaconBlockBodyRef, BeaconBlock),
     map_ref_mut_into(BeaconBlockBodyRefMut)
 )]
-#[derive(Debug, Clone, Serialize, Deserialize, Encode, TreeHash, Derivative)]
+#[derive(
+    Debug, Clone, Serialize, Deserialize, Encode, TreeHash, Derivative, arbitrary::Arbitrary,
+)]
 #[derivative(PartialEq, Hash(bound = "T: EthSpec"))]
 #[serde(untagged)]
-#[serde(bound = "T: EthSpec, Payload: ExecPayload<T>")]
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
+#[serde(bound = "T: EthSpec, Payload: AbstractExecPayload<T>")]
+#[arbitrary(bound = "T: EthSpec, Payload: AbstractExecPayload<T>")]
 #[tree_hash(enum_behaviour = "transparent")]
 #[ssz(enum_behaviour = "transparent")]
 pub struct BeaconBlock<T: EthSpec, Payload: AbstractExecPayload<T> = FullPayload<T>> {

consensus/types/src/beacon_block.rs

@@ -26,19 +26,23 @@ use tree_hash_derive::TreeHash;
             TreeHash,
             TestRandom,
             Derivative,
+            arbitrary::Arbitrary
         ),
-        derivative(PartialEq, Hash(bound = "T: EthSpec, Payload: ExecPayload<T>")),
-        serde(bound = "T: EthSpec, Payload: ExecPayload<T>", deny_unknown_fields),
-        cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))
+        derivative(PartialEq, Hash(bound = "T: EthSpec, Payload: AbstractExecPayload<T>")),
+        serde(
+            bound = "T: EthSpec, Payload: AbstractExecPayload<T>",
+            deny_unknown_fields
+        ),
+        arbitrary(bound = "T: EthSpec, Payload: AbstractExecPayload<T>"),
     ),
     cast_error(ty = "Error", expr = "Error::IncorrectStateVariant"),
     partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant")
 )]
-#[derive(Debug, Clone, Serialize, Deserialize, Derivative)]
+#[derive(Debug, Clone, Serialize, Deserialize, Derivative, arbitrary::Arbitrary)]
 #[derivative(PartialEq, Hash(bound = "T: EthSpec"))]
 #[serde(untagged)]
-#[serde(bound = "T: EthSpec, Payload: ExecPayload<T>")]
-#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))]
+#[serde(bound = "T: EthSpec, Payload: AbstractExecPayload<T>")]
+#[arbitrary(bound = "T: EthSpec, Payload: AbstractExecPayload<T>")]
 pub struct BeaconBlockBody<T: EthSpec, Payload: AbstractExecPayload<T> = FullPayload<T>> {
     pub randao_reveal: Signature,
     pub eth1_data: Eth1Data,
@@ -71,6 +75,7 @@ pub struct BeaconBlockBody<T: EthSpec, Payload: AbstractExecPayload<T> = FullPay
     #[ssz(skip_serializing, skip_deserializing)]
     #[tree_hash(skip_hashing)]
     #[serde(skip)]
+    #[arbitrary(default)]
     pub _phantom: PhantomData<Payload>,
 }