From adf24472dd1328d32432bceae73ee7466bbb6cb9 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Tue, 16 Jul 2024 15:35:51 +0900 Subject: [PATCH] Improve frozen abi dx a bit and document it (#2131) * Run frozen-abi tests earlier in ci * Run cargo check & clippy on frozen-abi code in ci * Rename IgnoreAsHelper => TransparentAsHelper * Add explaantion of impl AbiExample for Bank * Use PhantomData<_> to avoid AbiExample for Bank * Fix ci... --- accounts-db/src/accounts.rs | 2 -- ci/buildkite-pipeline.sh | 1 + ci/test-checks.sh | 3 +- frozen-abi/src/abi_digester.rs | 6 ++-- frozen-abi/src/abi_example.rs | 17 +++++----- programs/sbf/Cargo.toml | 2 ++ runtime/src/bank.rs | 31 ------------------- runtime/src/bank/builtins/prototypes.rs | 18 ----------- .../src/bank/partitioned_epoch_rewards/mod.rs | 3 -- runtime/src/bank/serde_snapshot.rs | 31 +++++++++++++++---- runtime/src/serde_snapshot.rs | 4 +-- runtime/src/serde_snapshot/storage.rs | 2 +- runtime/src/serde_snapshot/utils.rs | 8 ++--- scripts/cargo-clippy-nightly.sh | 2 +- sdk/src/packet.rs | 2 +- 15 files changed, 52 insertions(+), 80 deletions(-) diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 1f87be1ae86e44..ecf70b7701bfc1 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -45,7 +45,6 @@ use { pub type PubkeyAccountSlot = (Pubkey, AccountSharedData, Slot); -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug, Default)] pub struct AccountLocks { write_locks: HashSet, @@ -99,7 +98,6 @@ impl AccountLocks { } /// This structure handles synchronization for db -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug)] pub struct Accounts { /// Single global AccountsDb diff --git a/ci/buildkite-pipeline.sh b/ci/buildkite-pipeline.sh index 711551805d2de9..4d1559b606c4df 100755 --- a/ci/buildkite-pipeline.sh +++ b/ci/buildkite-pipeline.sh @@ -184,6 +184,7 @@ all_test_steps() { command_step checks2 "ci/docker-run-default-image.sh ci/test-dev-context-only-utils.sh check-bins" 15 check command_step checks3 "ci/docker-run-default-image.sh ci/test-dev-context-only-utils.sh check-all-targets" 15 check command_step miri "ci/docker-run-default-image.sh ci/test-miri.sh" 5 check + command_step frozen-abi "ci/docker-run-default-image.sh ./test-abi.sh" 15 check wait_step # Full test suite diff --git a/ci/test-checks.sh b/ci/test-checks.sh index a2022204df7ad4..873c3a9469e497 100755 --- a/ci/test-checks.sh +++ b/ci/test-checks.sh @@ -54,7 +54,8 @@ export RUSTFLAGS="-D warnings -A incomplete_features" # Only force up-to-date lock files on edge if [[ $CI_BASE_BRANCH = "$EDGE_CHANNEL" ]]; then - if _ scripts/cargo-for-all-lock-files.sh "+${rust_nightly}" check --locked --workspace --all-targets --features dummy-for-ci-check; then + if _ scripts/cargo-for-all-lock-files.sh "+${rust_nightly}" check \ + --locked --workspace --all-targets --features dummy-for-ci-check,frozen-abi; then true else check_status=$? diff --git a/frozen-abi/src/abi_digester.rs b/frozen-abi/src/abi_digester.rs index 39e6ce46f34150..9d2ee5f296d470 100644 --- a/frozen-abi/src/abi_digester.rs +++ b/frozen-abi/src/abi_digester.rs @@ -127,7 +127,7 @@ impl AbiDigester { value.serialize(self.create_new()) } else { // Don't call value.visit_for_abi(...) to prefer autoref specialization - // resolution for IgnoreAsHelper + // resolution for TransparentAsHelper <&T>::visit_for_abi(&value, &mut self.create_new()) } } @@ -657,7 +657,7 @@ mod tests { type TestBitVec = bv::BitVec; mod bitflags_abi { - use crate::abi_example::{AbiExample, EvenAsOpaque, IgnoreAsHelper}; + use crate::abi_example::{AbiExample, EvenAsOpaque, TransparentAsHelper}; bitflags::bitflags! { #[frozen_abi(digest = "HhKNkaeAd7AohTb8S8sPKjAWwzxWY2DPz5FvkWmx5bSH")] @@ -673,7 +673,7 @@ mod tests { } } - impl IgnoreAsHelper for TestFlags {} + impl TransparentAsHelper for TestFlags {} // This (EvenAsOpaque) marker trait is needed for bitflags-generated types because we can't // impl AbiExample for its private type: // thread '...TestFlags_frozen_abi...' panicked at ...: diff --git a/frozen-abi/src/abi_example.rs b/frozen-abi/src/abi_example.rs index 7931b05b6a81b4..d0de2845776619 100644 --- a/frozen-abi/src/abi_example.rs +++ b/frozen-abi/src/abi_example.rs @@ -235,7 +235,7 @@ impl AbiExample for BitVec { } } -impl IgnoreAsHelper for BitVec {} +impl TransparentAsHelper for BitVec {} // This (EvenAsOpaque) marker trait is needed for BitVec because we can't impl AbiExample for its // private type: // thread '...TestBitVec_frozen_abi...' panicked at ...: @@ -515,12 +515,12 @@ impl AbiExample for IpAddr { // User-defined enums usually just need to impl this with namesake derive macro (AbiEnumVisitor). // // Note that sometimes this indirection doesn't work for various reasons. For that end, there are -// hacks with marker traits (IgnoreAsHelper/EvenAsOpaque). +// hacks with marker traits (TransparentAsHelper/EvenAsOpaque). pub trait AbiEnumVisitor: Serialize { fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult; } -pub trait IgnoreAsHelper {} +pub trait TransparentAsHelper {} pub trait EvenAsOpaque { const TYPE_NAME_MATCHER: &'static str; } @@ -538,7 +538,7 @@ impl AbiEnumVisitor for T { default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult { info!("AbiEnumVisitor for T: {}", type_name::()); // not calling self.serialize(...) is intentional here as the most generic impl - // consider IgnoreAsHelper and EvenAsOpaque if you're stuck on this.... + // consider TransparentAsHelper and EvenAsOpaque if you're stuck on this.... T::example() .serialize(digester.create_new()) .map_err(DigestError::wrap_by_type::) @@ -558,9 +558,12 @@ impl AbiEnumVisitor for &T { // force to call self.serialize instead of T::visit_for_abi() for serialization // helper structs like ad-hoc iterator `struct`s -impl AbiEnumVisitor for &T { +impl AbiEnumVisitor for &T { default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult { - info!("AbiEnumVisitor for (IgnoreAsHelper): {}", type_name::()); + info!( + "AbiEnumVisitor for (TransparentAsHelper): {}", + type_name::() + ); self.serialize(digester.create_new()) .map_err(DigestError::wrap_by_type::) } @@ -568,7 +571,7 @@ impl AbiEnumVisitor for &T { // force to call self.serialize instead of T::visit_for_abi() to work around the // inability of implementing AbiExample for private structs from other crates -impl AbiEnumVisitor for &T { +impl AbiEnumVisitor for &T { default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult { let type_name = type_name::(); let matcher = T::TYPE_NAME_MATCHER; diff --git a/programs/sbf/Cargo.toml b/programs/sbf/Cargo.toml index 85d21639e15ca4..0c8da26f7574ae 100644 --- a/programs/sbf/Cargo.toml +++ b/programs/sbf/Cargo.toml @@ -79,6 +79,8 @@ strip = true sbf_c = [] sbf_rust = [] dummy-for-ci-check = ["sbf_c", "sbf_rust"] +# This was needed for ci +frozen-abi = [] [dev-dependencies] agave-validator = { workspace = true } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index ab7393f649df78..20a15364121066 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -271,7 +271,6 @@ impl AddAssign for SquashTiming { } } -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug, Default, PartialEq)] pub(crate) struct CollectorFeeDetails { transaction_fee: u64, @@ -313,22 +312,6 @@ pub struct BankRc { pub(crate) bank_id_generator: Arc, } -#[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -use solana_frozen_abi::abi_example::AbiExample; - -#[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl AbiExample for BankRc { - fn example() -> Self { - BankRc { - // Set parent to None to cut the recursion into another Bank - parent: RwLock::new(None), - // AbiExample for Accounts is specially implemented to contain a storage example - accounts: AbiExample::example(), - bank_id_generator: Arc::new(AtomicU64::new(0)), - } - } -} - impl BankRc { pub(crate) fn new(accounts: Accounts) -> Self { Self { @@ -380,7 +363,6 @@ impl TransactionBalancesSet { } pub type TransactionBalances = Vec>; -#[cfg_attr(feature = "frozen-abi", derive(AbiExample, AbiEnumVisitor))] #[derive(Serialize, Deserialize, Debug, PartialEq, Eq)] pub enum TransactionLogCollectorFilter { All, @@ -395,14 +377,12 @@ impl Default for TransactionLogCollectorFilter { } } -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug, Default)] pub struct TransactionLogCollectorConfig { pub mentioned_addresses: HashSet, pub filter: TransactionLogCollectorFilter, } -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Clone, Debug, PartialEq, Eq)] pub struct TransactionLogInfo { pub signature: Signature, @@ -411,7 +391,6 @@ pub struct TransactionLogInfo { pub log_messages: TransactionLogMessages, } -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Default, Debug)] pub struct TransactionLogCollector { // All the logs collected for from this Bank. Exact contents depend on the @@ -696,17 +675,7 @@ pub trait DropCallback: fmt::Debug { #[derive(Debug, Default)] pub struct OptionalDropCallback(Option>); -#[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl AbiExample for OptionalDropCallback { - fn example() -> Self { - Self(None) - } -} - /// Manager for the state of all accounts and programs after processing its entries. -/// AbiExample is needed even without Serialize/Deserialize; actual (de-)serialization -/// are implemented elsewhere for versioning -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug)] pub struct Bank { /// References to accounts, parent and signature status diff --git a/runtime/src/bank/builtins/prototypes.rs b/runtime/src/bank/builtins/prototypes.rs index 813d948a3bd1e5..403544d4469f97 100644 --- a/runtime/src/bank/builtins/prototypes.rs +++ b/runtime/src/bank/builtins/prototypes.rs @@ -23,24 +23,6 @@ impl std::fmt::Debug for BuiltinPrototype { } } -#[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl solana_frozen_abi::abi_example::AbiExample for BuiltinPrototype { - fn example() -> Self { - // BuiltinPrototype isn't serializable by definition. - solana_program_runtime::declare_process_instruction!(MockBuiltin, 0, |_invoke_context| { - // Do nothing - Ok(()) - }); - Self { - core_bpf_migration_config: None, - enable_feature_id: None, - program_id: Pubkey::default(), - name: "", - entrypoint: MockBuiltin::vm, - } - } -} - /// Transitions of stateless built-in programs at epoch boundaries when /// features are activated. /// These are built-in programs that don't actually exist, but their address diff --git a/runtime/src/bank/partitioned_epoch_rewards/mod.rs b/runtime/src/bank/partitioned_epoch_rewards/mod.rs index ea405dce8e1316..49622ba183a396 100644 --- a/runtime/src/bank/partitioned_epoch_rewards/mod.rs +++ b/runtime/src/bank/partitioned_epoch_rewards/mod.rs @@ -26,7 +26,6 @@ use { /// Distributing rewards to stake accounts begins AFTER this many blocks. const REWARD_CALCULATION_NUM_BLOCKS: u64 = 1; -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] pub(crate) struct PartitionedStakeReward { /// Stake account address @@ -55,7 +54,6 @@ impl PartitionedStakeReward { type PartitionedStakeRewards = Vec; -#[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub(crate) struct StartBlockHeightAndRewards { /// the block height of the slot at which rewards distribution began @@ -65,7 +63,6 @@ pub(crate) struct StartBlockHeightAndRewards { } /// Represent whether bank is in the reward phase or not. -#[cfg_attr(feature = "frozen-abi", derive(AbiExample, AbiEnumVisitor))] #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Default)] pub(crate) enum EpochRewardStatus { /// this bank is in the reward phase. diff --git a/runtime/src/bank/serde_snapshot.rs b/runtime/src/bank/serde_snapshot.rs index c4f716071064f1..f3718b97b8b896 100644 --- a/runtime/src/bank/serde_snapshot.rs +++ b/runtime/src/bank/serde_snapshot.rs @@ -473,18 +473,36 @@ mod tests { assert_eq!(dbank.epoch_reward_status, EpochRewardStatus::Inactive); } - #[cfg(RUSTC_WITH_SPECIALIZATION)] + #[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] mod test_bank_serialize { use { super::*, solana_accounts_db::{ account_storage::meta::StoredMetaWriteVersion, accounts_db::BankHashStats, }, + solana_frozen_abi::abi_example::AbiExample, solana_sdk::clock::Slot, + std::marker::PhantomData, }; - // This some what long test harness is required to freeze the ABI of - // Bank's serialization due to versioned nature + // This some what long test harness is required to freeze the ABI of Bank's serialization, + // which is implemented manually by calling serialize_bank_snapshot_with() mainly based on + // get_fields_to_serialize(). However, note that Bank's serialization is coupled with + // snapshot storages as well. + // + // It was avoided to impl AbiExample for Bank by wrapping it around PhantomData inside the + // spcecial wrapper called BankAbiTestWrapper. And internally, it creates an actual bank + // from Bank::default_for_tests(). + // + // In this way, frozen abi can increase the coverage of the serialization code path as much + // as possible. Alternatively, we could derive AbiExample for the minimum set of actually + // serialized fields of bank as an ad-hoc tuple. But that was avoided to avoid maintenance + // burden instead. + // + // Involving the Bank here is preferred conceptually because snapshot abi is + // important and snapshot is just a (rooted) serialized bank at the high level. Only + // abi-freezing bank.get_fields_to_serialize() is kind of relying on the implementation + // detail. #[cfg_attr( feature = "frozen-abi", derive(AbiExample), @@ -493,14 +511,15 @@ mod tests { #[derive(Serialize)] pub struct BankAbiTestWrapper { #[serde(serialize_with = "wrapper")] - bank: Bank, + bank: PhantomData, } - pub fn wrapper(bank: &Bank, serializer: S) -> Result + pub fn wrapper(_bank: &PhantomData, serializer: S) -> Result where S: serde::Serializer, { - let snapshot_storages = bank.get_snapshot_storages(None); + let bank = Bank::default_for_tests(); + let snapshot_storages = AccountsDb::example().get_snapshot_storages(0..1).0; // ensure there is at least one snapshot storage example for ABI digesting assert!(!snapshot_storages.is_empty()); diff --git a/runtime/src/serde_snapshot.rs b/runtime/src/serde_snapshot.rs index 28964394c21785..6a3a179966368a 100644 --- a/runtime/src/serde_snapshot.rs +++ b/runtime/src/serde_snapshot.rs @@ -284,7 +284,7 @@ impl From for SerializableVersionedBank { } #[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl<'a> solana_frozen_abi::abi_example::IgnoreAsHelper for SerializableVersionedBank {} +impl solana_frozen_abi::abi_example::TransparentAsHelper for SerializableVersionedBank {} /// Helper type to wrap BufReader streams when deserializing and reconstructing from either just a /// full snapshot, or both a full and incremental snapshot @@ -790,7 +790,7 @@ impl<'a> Serialize for SerializableAccountsDb<'a> { } #[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl<'a> solana_frozen_abi::abi_example::IgnoreAsHelper for SerializableAccountsDb<'a> {} +impl<'a> solana_frozen_abi::abi_example::TransparentAsHelper for SerializableAccountsDb<'a> {} #[allow(clippy::too_many_arguments)] fn reconstruct_bank_from_fields( diff --git a/runtime/src/serde_snapshot/storage.rs b/runtime/src/serde_snapshot/storage.rs index 5b8bed48fa90b1..06557332536828 100644 --- a/runtime/src/serde_snapshot/storage.rs +++ b/runtime/src/serde_snapshot/storage.rs @@ -37,4 +37,4 @@ impl From<&AccountStorageEntry> for SerializableAccountStorageEntry { } #[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl solana_frozen_abi::abi_example::IgnoreAsHelper for SerializableAccountStorageEntry {} +impl solana_frozen_abi::abi_example::TransparentAsHelper for SerializableAccountStorageEntry {} diff --git a/runtime/src/serde_snapshot/utils.rs b/runtime/src/serde_snapshot/utils.rs index d12edd21222876..a9b953ba4851bf 100644 --- a/runtime/src/serde_snapshot/utils.rs +++ b/runtime/src/serde_snapshot/utils.rs @@ -3,7 +3,7 @@ use serde::{ Serialize, Serializer, }; #[cfg(all(test, RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -use solana_frozen_abi::abi_example::IgnoreAsHelper; +use solana_frozen_abi::abi_example::TransparentAsHelper; // consumes an iterator and returns an object that will serialize as a serde seq #[allow(dead_code)] @@ -18,7 +18,7 @@ where } #[cfg(all(test, RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] - impl IgnoreAsHelper for SerializableSequencedIterator {} + impl TransparentAsHelper for SerializableSequencedIterator {} impl Serialize for SerializableSequencedIterator where @@ -57,7 +57,7 @@ where } #[cfg(all(test, RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] - impl IgnoreAsHelper for SerializableSequencedIterator {} + impl TransparentAsHelper for SerializableSequencedIterator {} impl Serialize for SerializableSequencedIterator where @@ -96,7 +96,7 @@ where } #[cfg(all(test, RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] - impl IgnoreAsHelper for SerializableMappedIterator {} + impl TransparentAsHelper for SerializableMappedIterator {} impl Serialize for SerializableMappedIterator where diff --git a/scripts/cargo-clippy-nightly.sh b/scripts/cargo-clippy-nightly.sh index 12e0fc5b4ad0bb..756699408f4340 100755 --- a/scripts/cargo-clippy-nightly.sh +++ b/scripts/cargo-clippy-nightly.sh @@ -24,7 +24,7 @@ source "$here/../ci/rust-version.sh" nightly # ref: https://github.com/rust-lang/rust/issues/66287 "$here/cargo-for-all-lock-files.sh" -- \ "+${rust_nightly}" clippy \ - --workspace --all-targets --features dummy-for-ci-check -- \ + --workspace --all-targets --features dummy-for-ci-check,frozen-abi -- \ --deny=warnings \ --deny=clippy::default_trait_access \ --deny=clippy::arithmetic_side_effects \ diff --git a/sdk/src/packet.rs b/sdk/src/packet.rs index 0f45dd4e6311c0..8d3ef8b3e539cf 100644 --- a/sdk/src/packet.rs +++ b/sdk/src/packet.rs @@ -58,7 +58,7 @@ impl ::solana_frozen_abi::abi_example::AbiExample for PacketFlags { } #[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] -impl ::solana_frozen_abi::abi_example::IgnoreAsHelper for PacketFlags {} +impl ::solana_frozen_abi::abi_example::TransparentAsHelper for PacketFlags {} #[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))] impl ::solana_frozen_abi::abi_example::EvenAsOpaque for PacketFlags {