From 466f21acfabaa7c23f915aa870bd3619b81f72bc Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Wed, 25 Sep 2024 13:27:37 +0200 Subject: [PATCH] feat: verify unused bits on types derived with `Compact` (#11131) --- crates/storage/codecs/Cargo.toml | 1 + .../codecs/derive/src/compact/flags.rs | 9 +++ .../storage/codecs/derive/src/compact/mod.rs | 4 + crates/storage/codecs/derive/src/lib.rs | 30 +++++++ .../codecs/src/alloy/authorization_list.rs | 2 +- .../codecs/src/alloy/genesis_account.rs | 8 +- crates/storage/codecs/src/alloy/header.rs | 2 +- crates/storage/codecs/src/alloy/mod.rs | 38 +++++++++ .../codecs/src/alloy/transaction/mod.rs | 12 +-- crates/storage/codecs/src/alloy/withdrawal.rs | 2 +- crates/storage/codecs/src/lib.rs | 3 + crates/storage/codecs/src/test_utils.rs | 81 +++++++++++++++++++ crates/storage/db-api/Cargo.toml | 2 +- crates/storage/db-api/src/models/mod.rs | 54 +++++++++++++ 14 files changed, 234 insertions(+), 14 deletions(-) create mode 100644 crates/storage/codecs/src/test_utils.rs diff --git a/crates/storage/codecs/Cargo.toml b/crates/storage/codecs/Cargo.toml index 1b9882b47..7a999d35f 100644 --- a/crates/storage/codecs/Cargo.toml +++ b/crates/storage/codecs/Cargo.toml @@ -61,3 +61,4 @@ alloy = [ "dep:serde" ] optimism = ["alloy", "dep:op-alloy-consensus"] +test-utils = [] diff --git a/crates/storage/codecs/derive/src/compact/flags.rs b/crates/storage/codecs/derive/src/compact/flags.rs index 34ab2edba..3242a611e 100644 --- a/crates/storage/codecs/derive/src/compact/flags.rs +++ b/crates/storage/codecs/derive/src/compact/flags.rs @@ -53,6 +53,7 @@ pub(crate) fn generate_flag_struct( let docs = format!("Fieldset that facilitates compacting the parent type. Used bytes: {total_bytes} | Unused bits: {unused_bits}"); let bitflag_encoded_bytes = format!("Used bytes by [`{flags_ident}`]"); + let bitflag_unused_bits = format!("Unused bits for new fields by [`{flags_ident}`]"); let impl_bitflag_encoded_bytes = if has_lifetime { quote! { impl<'a> #ident<'a> { @@ -60,6 +61,10 @@ pub(crate) fn generate_flag_struct( pub const fn bitflag_encoded_bytes() -> usize { #total_bytes as usize } + #[doc = #bitflag_unused_bits] + pub const fn bitflag_unused_bits() -> usize { + #unused_bits as usize + } } } } else { @@ -69,6 +74,10 @@ pub(crate) fn generate_flag_struct( pub const fn bitflag_encoded_bytes() -> usize { #total_bytes as usize } + #[doc = #bitflag_unused_bits] + pub const fn bitflag_unused_bits() -> usize { + #unused_bits as usize + } } } }; diff --git a/crates/storage/codecs/derive/src/compact/mod.rs b/crates/storage/codecs/derive/src/compact/mod.rs index 6190b668e..e5a79b3fe 100644 --- a/crates/storage/codecs/derive/src/compact/mod.rs +++ b/crates/storage/codecs/derive/src/compact/mod.rs @@ -245,6 +245,10 @@ mod tests { pub const fn bitflag_encoded_bytes() -> usize { 2u8 as usize } + #[doc = "Unused bits for new fields by [`TestStructFlags`]"] + pub const fn bitflag_unused_bits() -> usize { + 1u8 as usize + } } pub use TestStruct_flags::TestStructFlags; diff --git a/crates/storage/codecs/derive/src/lib.rs b/crates/storage/codecs/derive/src/lib.rs index 0c317641c..4ffdbfd6e 100644 --- a/crates/storage/codecs/derive/src/lib.rs +++ b/crates/storage/codecs/derive/src/lib.rs @@ -20,12 +20,42 @@ use syn::{ mod arbitrary; mod compact; +/// Derives the `Compact` trait for custom structs, optimizing serialization with a possible +/// bitflag struct. +/// +/// ## Implementation: +/// The derived `Compact` implementation leverages a bitflag struct when needed to manage the +/// presence of certain field types, primarily for compacting fields efficiently. This bitflag +/// struct records information about fields that require a small, fixed number of bits for their +/// encoding, such as `bool`, `Option`, or other small types. +/// +/// ### Bit Sizes for Fields: +/// The amount of bits used to store a field size is determined by the field's type. For specific +/// types, a fixed number of bits is allocated (from `fn get_bit_size`): +/// - `bool`, `Option`, `TransactionKind`, `Signature`: **1 bit** +/// - `TxType`: **2 bits** +/// - `u64`, `BlockNumber`, `TxNumber`, `ChainId`, `NumTransactions`: **4 bits** +/// - `u128`: **5 bits** +/// - `U256`: **6 bits** +/// +/// ### Warning: Extending structs, unused bits and backwards compatibility: +/// When the bitflag only has one bit left (for example, when adding many `Option` fields), +/// you should introduce a new struct (e.g., `TExtension`) with additional fields, and use +/// `Option` in the original struct. This approach allows further field extensions while +/// maintaining backward compatibility. +/// +/// ### Limitations: +/// - Fields not listed above, or types such `Vec`, or large composite types, should manage their +/// own encoding and do not rely on the bitflag struct. +/// - `Bytes` fields and any types containing a `Bytes` field should be placed last to ensure +/// efficient decoding. #[proc_macro_derive(Compact, attributes(maybe_zero))] pub fn derive(input: TokenStream) -> TokenStream { let is_zstd = false; compact::derive(input, is_zstd) } +/// Adds `zstd` compression to derived [`Compact`]. #[proc_macro_derive(CompactZstd, attributes(maybe_zero))] pub fn derive_zstd(input: TokenStream) -> TokenStream { let is_zstd = true; diff --git a/crates/storage/codecs/src/alloy/authorization_list.rs b/crates/storage/codecs/src/alloy/authorization_list.rs index b59c699e6..2c1495abf 100644 --- a/crates/storage/codecs/src/alloy/authorization_list.rs +++ b/crates/storage/codecs/src/alloy/authorization_list.rs @@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, Compact)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[add_arbitrary_tests(compact)] -struct Authorization { +pub(crate) struct Authorization { chain_id: U256, address: Address, nonce: u64, diff --git a/crates/storage/codecs/src/alloy/genesis_account.rs b/crates/storage/codecs/src/alloy/genesis_account.rs index 84c8ee77a..a94f4e2ef 100644 --- a/crates/storage/codecs/src/alloy/genesis_account.rs +++ b/crates/storage/codecs/src/alloy/genesis_account.rs @@ -10,7 +10,7 @@ use serde::{Deserialize, Serialize}; /// /// Notice: Make sure this struct is 1:1 with `alloy_genesis::GenesisAccount` #[derive(Debug, Clone, PartialEq, Eq, Compact)] -struct GenesisAccountRef<'a> { +pub(crate) struct GenesisAccountRef<'a> { /// The nonce of the account at genesis. nonce: Option, /// The balance of the account at genesis. @@ -26,7 +26,7 @@ struct GenesisAccountRef<'a> { #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, Compact)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[add_arbitrary_tests(compact)] -struct GenesisAccount { +pub(crate) struct GenesisAccount { /// The nonce of the account at genesis. nonce: Option, /// The balance of the account at genesis. @@ -42,14 +42,14 @@ struct GenesisAccount { #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, Compact)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[add_arbitrary_tests(compact)] -struct StorageEntries { +pub(crate) struct StorageEntries { entries: Vec, } #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, Compact)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[add_arbitrary_tests(compact)] -struct StorageEntry { +pub(crate) struct StorageEntry { key: B256, value: B256, } diff --git a/crates/storage/codecs/src/alloy/header.rs b/crates/storage/codecs/src/alloy/header.rs index c989f45e7..a72021fdc 100644 --- a/crates/storage/codecs/src/alloy/header.rs +++ b/crates/storage/codecs/src/alloy/header.rs @@ -12,7 +12,7 @@ use serde::{Deserialize, Serialize}; /// /// Notice: Make sure this struct is 1:1 with [`alloy_consensus::Header`] #[derive(Debug, Clone, PartialEq, Eq, Hash, Default, Serialize, Deserialize, Compact)] -struct Header { +pub(crate) struct Header { parent_hash: B256, ommers_hash: B256, beneficiary: Address, diff --git a/crates/storage/codecs/src/alloy/mod.rs b/crates/storage/codecs/src/alloy/mod.rs index 92d52d8b7..8da0f7a94 100644 --- a/crates/storage/codecs/src/alloy/mod.rs +++ b/crates/storage/codecs/src/alloy/mod.rs @@ -9,3 +9,41 @@ mod transaction; mod trie; mod txkind; mod withdrawal; + +#[cfg(test)] +mod tests { + use crate::{ + alloy::{ + authorization_list::Authorization, + genesis_account::{GenesisAccount, GenesisAccountRef, StorageEntries, StorageEntry}, + header::Header, + transaction::{ + eip1559::TxEip1559, eip2930::TxEip2930, eip4844::TxEip4844, eip7702::TxEip7702, + legacy::TxLegacy, + }, + withdrawal::Withdrawal, + }, + test_utils::UnusedBits, + validate_bitflag_backwards_compat, + }; + + #[test] + fn validate_bitflag_backwards_compat() { + // In case of failure, refer to the documentation of the + // [`validate_bitflag_backwards_compat`] macro for detailed instructions on handling + // it. + validate_bitflag_backwards_compat!(Header, UnusedBits::Zero); + validate_bitflag_backwards_compat!(TxEip2930, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StorageEntries, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StorageEntry, UnusedBits::Zero); + + validate_bitflag_backwards_compat!(Authorization, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(GenesisAccountRef<'_>, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(GenesisAccount, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(TxEip1559, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(TxEip4844, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(TxEip7702, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(TxLegacy, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(Withdrawal, UnusedBits::NotZero); + } +} diff --git a/crates/storage/codecs/src/alloy/transaction/mod.rs b/crates/storage/codecs/src/alloy/transaction/mod.rs index 717f918fb..86edfee5b 100644 --- a/crates/storage/codecs/src/alloy/transaction/mod.rs +++ b/crates/storage/codecs/src/alloy/transaction/mod.rs @@ -1,10 +1,10 @@ -mod eip1559; -mod eip2930; -mod eip4844; -mod eip7702; -mod legacy; +pub(crate) mod eip1559; +pub(crate) mod eip2930; +pub(crate) mod eip4844; +pub(crate) mod eip7702; +pub(crate) mod legacy; #[cfg(feature = "optimism")] -mod optimism; +pub(crate) mod optimism; #[cfg(test)] mod tests { diff --git a/crates/storage/codecs/src/alloy/withdrawal.rs b/crates/storage/codecs/src/alloy/withdrawal.rs index 55d2f6065..0ec169321 100644 --- a/crates/storage/codecs/src/alloy/withdrawal.rs +++ b/crates/storage/codecs/src/alloy/withdrawal.rs @@ -10,7 +10,7 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize, Compact)] #[cfg_attr(test, derive(arbitrary::Arbitrary))] #[add_arbitrary_tests(compact)] -struct Withdrawal { +pub(crate) struct Withdrawal { /// Monotonically increasing identifier issued by consensus layer. index: u64, /// Index of validator associated with withdrawal. diff --git a/crates/storage/codecs/src/lib.rs b/crates/storage/codecs/src/lib.rs index a3d8070e6..3b05ce8e9 100644 --- a/crates/storage/codecs/src/lib.rs +++ b/crates/storage/codecs/src/lib.rs @@ -28,6 +28,9 @@ use alloc::vec::Vec; #[cfg(any(test, feature = "alloy"))] mod alloy; +#[cfg(any(test, feature = "test-utils"))] +pub mod test_utils; + /// Trait that implements the `Compact` codec. /// /// When deriving the trait for custom structs, be aware of certain limitations/recommendations: diff --git a/crates/storage/codecs/src/test_utils.rs b/crates/storage/codecs/src/test_utils.rs new file mode 100644 index 000000000..bb377c691 --- /dev/null +++ b/crates/storage/codecs/src/test_utils.rs @@ -0,0 +1,81 @@ +//! Test utilities for `Compact` derive macro + +/// Macro to ensure that derived `Compact` types can be extended with new fields while maintaining +/// backwards compatibility. +/// +/// Verifies that the unused bits in the bitflag struct remain as expected: `Zero` or `NotZero`. For +/// more on bitflag struct: [`reth_codecs_derive::Compact`]. +/// +/// Possible failures: +/// ### 1. `NotZero` -> `Zero` +/// This wouldn't allow new fields to be added in the future. Instead, the new field of `T` +/// should be `Option` to allow for new fields. The new user field should be included +/// in `TExtension` type. **Only then, update the test to expect `Zero` for `T` and +/// add a new test for `TExtension`.** +/// +/// **Goal:** +/// +/// ```rust,ignore +/// { +/// struct T { +/// // ... other fields +/// ext: Option +/// } +/// +/// // Use an extension type for new fields: +/// struct TExtension { +/// new_field_b: Option, +/// } +/// +/// // Change tests +/// validate_bitflag_backwards_compat!(T, UnusedBits::Zero); +/// validate_bitflag_backwards_compat!(TExtension, UnusedBits::NotZero); +/// } +/// ``` +/// +/// ### 2. `Zero` -> `NotZero` +/// If it becomes `NotZero`, it would break backwards compatibility, so there is not an action item, +/// and should be handled with care in a case by case scenario. +#[macro_export] +macro_rules! validate_bitflag_backwards_compat { + ($type:ty, $expected_unused_bits:expr) => { + let actual_unused_bits = <$type>::bitflag_unused_bits(); + + match $expected_unused_bits { + UnusedBits::NotZero => { + assert_ne!( + actual_unused_bits, + 0, + "Assertion failed: `bitflag_unused_bits` for type `{}` unexpectedly went from non-zero to zero!", + stringify!($type) + ); + } + UnusedBits::Zero => { + assert_eq!( + actual_unused_bits, + 0, + "Assertion failed: `bitflag_unused_bits` for type `{}` unexpectedly went from zero to non-zero!", + stringify!($type) + ); + } + } + }; +} + +/// Whether there are zero or more unused bits on `Compact` bitflag struct. +/// +/// To be used with [`validate_bitflag_backwards_compat`]. +#[derive(Debug)] +pub enum UnusedBits { + /// Zero bits available for a new field. + Zero, + /// Bits available for a new field. + NotZero, +} + +impl UnusedBits { + /// Returns true if the variant is [`Self::NotZero`]. + pub const fn not_zero(&self) -> bool { + matches!(self, Self::NotZero) + } +} diff --git a/crates/storage/db-api/Cargo.toml b/crates/storage/db-api/Cargo.toml index a6a600abc..d674f9d7b 100644 --- a/crates/storage/db-api/Cargo.toml +++ b/crates/storage/db-api/Cargo.toml @@ -45,7 +45,7 @@ proptest = { workspace = true, optional = true } [dev-dependencies] # reth libs with arbitrary reth-primitives = { workspace = true, features = ["arbitrary"] } -reth-codecs.workspace = true +reth-codecs = { workspace = true, features = ["test-utils"] } rand.workspace = true diff --git a/crates/storage/db-api/src/models/mod.rs b/crates/storage/db-api/src/models/mod.rs index af8d7da0d..942b9b27a 100644 --- a/crates/storage/db-api/src/models/mod.rs +++ b/crates/storage/db-api/src/models/mod.rs @@ -306,6 +306,7 @@ add_wrapper_struct!((ClientVersion, CompactClientVersion)); #[cfg(test)] mod tests { use super::*; + use reth_codecs::{test_utils::UnusedBits, validate_bitflag_backwards_compat}; use reth_primitives::{Account, Receipt, ReceiptWithBloom, SealedHeader, Withdrawals}; use reth_prune_types::{PruneCheckpoint, PruneMode, PruneSegment}; use reth_stages_types::{ @@ -345,6 +346,31 @@ mod tests { assert_eq!(StoredBlockWithdrawals::bitflag_encoded_bytes(), 0); assert_eq!(StorageHashingCheckpoint::bitflag_encoded_bytes(), 1); assert_eq!(Withdrawals::bitflag_encoded_bytes(), 0); + + validate_bitflag_backwards_compat!(Account, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(AccountHashingCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(CheckpointBlockRange, UnusedBits::Zero); + validate_bitflag_backwards_compat!(CompactClientVersion, UnusedBits::Zero); + validate_bitflag_backwards_compat!(CompactU256, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(CompactU64, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(EntitiesCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(ExecutionCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(HeadersCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(IndexHistoryCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(PruneCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(PruneMode, UnusedBits::Zero); + validate_bitflag_backwards_compat!(PruneSegment, UnusedBits::Zero); + validate_bitflag_backwards_compat!(Receipt, UnusedBits::Zero); + validate_bitflag_backwards_compat!(ReceiptWithBloom, UnusedBits::Zero); + validate_bitflag_backwards_compat!(SealedHeader, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StageCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(StageUnitCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StoredBlockBodyIndices, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StoredBlockOmmers, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StoredBlockWithdrawals, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StorageHashingCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(Withdrawals, UnusedBits::Zero); + validate_bitflag_backwards_compat!(Requests, UnusedBits::Zero); } #[cfg(feature = "optimism")] @@ -373,5 +399,33 @@ mod tests { assert_eq!(StoredBlockWithdrawals::bitflag_encoded_bytes(), 0); assert_eq!(StorageHashingCheckpoint::bitflag_encoded_bytes(), 1); assert_eq!(Withdrawals::bitflag_encoded_bytes(), 0); + + // In case of failure, refer to the documentation of the + // [`validate_bitflag_backwards_compat`] macro for detailed instructions on handling + // it. + validate_bitflag_backwards_compat!(Account, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(AccountHashingCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(CheckpointBlockRange, UnusedBits::Zero); + validate_bitflag_backwards_compat!(CompactClientVersion, UnusedBits::Zero); + validate_bitflag_backwards_compat!(CompactU256, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(CompactU64, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(EntitiesCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(ExecutionCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(HeadersCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(IndexHistoryCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(PruneCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(PruneMode, UnusedBits::Zero); + validate_bitflag_backwards_compat!(PruneSegment, UnusedBits::Zero); + validate_bitflag_backwards_compat!(Receipt, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(ReceiptWithBloom, UnusedBits::Zero); + validate_bitflag_backwards_compat!(SealedHeader, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StageCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(StageUnitCheckpoint, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StoredBlockBodyIndices, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StoredBlockOmmers, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StoredBlockWithdrawals, UnusedBits::Zero); + validate_bitflag_backwards_compat!(StorageHashingCheckpoint, UnusedBits::NotZero); + validate_bitflag_backwards_compat!(Withdrawals, UnusedBits::Zero); + validate_bitflag_backwards_compat!(Requests, UnusedBits::Zero); } }