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

@@ -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<Pubkey>,
@@ -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

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

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=$?

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<u64>;
 
     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 ...:

frozen-abi/src/abi_example.rs

@@ -235,7 +235,7 @@ impl<T: BlockType> AbiExample for BitVec<T> {
     }
 }
 
-impl<T: BlockType> IgnoreAsHelper for BitVec<T> {}
+impl<T: BlockType> TransparentAsHelper for BitVec<T> {}
 // 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<T: Serialize + ?Sized + AbiExample> AbiEnumVisitor for T {
     default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult {
         info!("AbiEnumVisitor for T: {}", type_name::<T>());
         // 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::<T>)
@@ -558,17 +558,20 @@ impl<T: Serialize + ?Sized + AbiEnumVisitor> 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<T: Serialize + IgnoreAsHelper> AbiEnumVisitor for &T {
+impl<T: Serialize + TransparentAsHelper> AbiEnumVisitor for &T {
     default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult {
-        info!("AbiEnumVisitor for (IgnoreAsHelper): {}", type_name::<T>());
+        info!(
+            "AbiEnumVisitor for (TransparentAsHelper): {}",
+            type_name::<T>()
+        );
         self.serialize(digester.create_new())
             .map_err(DigestError::wrap_by_type::<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<T: Serialize + IgnoreAsHelper + EvenAsOpaque> AbiEnumVisitor for &T {
+impl<T: Serialize + TransparentAsHelper + EvenAsOpaque> AbiEnumVisitor for &T {
     default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult {
         let type_name = type_name::<T>();
         let matcher = T::TYPE_NAME_MATCHER;

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 }

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<AtomicU64>,
 }
 
-#[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<Vec<u64>>;
 
-#[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<Pubkey>,
     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<Box<dyn DropCallback + Send + Sync>>);
 
-#[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

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

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<PartitionedStakeReward>;
 
-#[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.

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<Bank>,
         }
 
-        pub fn wrapper<S>(bank: &Bank, serializer: S) -> Result<S::Ok, S::Error>
+        pub fn wrapper<S>(_bank: &PhantomData<Bank>, serializer: S) -> Result<S::Ok, S::Error>
         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());
 

runtime/src/serde_snapshot.rs

@@ -284,7 +284,7 @@ impl From<BankFieldsToSerialize> 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<E>(

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 {}

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<I> IgnoreAsHelper for SerializableSequencedIterator<I> {}
+    impl<I> TransparentAsHelper for SerializableSequencedIterator<I> {}
 
     impl<I> Serialize for SerializableSequencedIterator<I>
     where
@@ -57,7 +57,7 @@ where
     }
 
     #[cfg(all(test, RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))]
-    impl<I> IgnoreAsHelper for SerializableSequencedIterator<I> {}
+    impl<I> TransparentAsHelper for SerializableSequencedIterator<I> {}
 
     impl<I> Serialize for SerializableSequencedIterator<I>
     where
@@ -96,7 +96,7 @@ where
     }
 
     #[cfg(all(test, RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))]
-    impl<I> IgnoreAsHelper for SerializableMappedIterator<I> {}
+    impl<I> TransparentAsHelper for SerializableMappedIterator<I> {}
 
     impl<K, V, I> Serialize for SerializableMappedIterator<I>
     where

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 \

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 {