Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Enable frozen_abi on banking trace file #33501

Merged
merged 10 commits into from
Oct 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions core/src/banking_trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,17 @@ pub struct BankingTracer {
active_tracer: Option<ActiveTracer>,
}

#[derive(Serialize, Deserialize, Debug)]
#[frozen_abi(digest = "Eq6YrAFtTbtPrCEvh6Et1mZZDCARUg1gcK2qiZdqyjUz")]
#[derive(Serialize, Deserialize, Debug, AbiExample)]
pub struct TimedTracedEvent(pub std::time::SystemTime, pub TracedEvent);

#[derive(Serialize, Deserialize, Debug)]
#[derive(Serialize, Deserialize, Debug, AbiExample, AbiEnumVisitor)]
pub enum TracedEvent {
PacketBatch(ChannelLabel, BankingPacketBatch),
BlockAndBankHash(Slot, Hash, Hash),
}

#[derive(Serialize, Deserialize, Debug, Clone, Copy)]
#[derive(Serialize, Deserialize, Debug, Clone, Copy, AbiExample, AbiEnumVisitor)]
pub enum ChannelLabel {
NonVote,
TpuVote,
Expand Down
2 changes: 1 addition & 1 deletion core/src/sigverify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use {
solana_sdk::{packet::Packet, saturating_add_assign},
};

#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, AbiExample)]
pub struct SigverifyTracerPacketStats {
pub total_removed_before_sigverify_stage: usize,
pub total_tracer_packets_received_in_sigverify_stage: usize,
Expand Down
1 change: 1 addition & 0 deletions frozen-abi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ subtle = { workspace = true }

[target.'cfg(not(target_os = "solana"))'.dev-dependencies]
solana-logger = { workspace = true }
bitflags = { workspace = true }

[build-dependencies]
rustc_version = { workspace = true }
54 changes: 45 additions & 9 deletions frozen-abi/src/abi_digester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub struct AbiDigester {
data_types: std::rc::Rc<std::cell::RefCell<Vec<String>>>,
depth: usize,
for_enum: bool,
opaque_scope: Option<String>,
opaque_type_matcher: Option<String>,
}

pub type DigestResult = Result<AbiDigester, DigestError>;
Expand Down Expand Up @@ -70,7 +70,7 @@ impl AbiDigester {
data_types: std::rc::Rc::new(std::cell::RefCell::new(vec![])),
for_enum: false,
depth: 0,
opaque_scope: None,
opaque_type_matcher: None,
}
}

Expand All @@ -81,16 +81,16 @@ impl AbiDigester {
data_types: self.data_types.clone(),
depth: self.depth,
for_enum: false,
opaque_scope: self.opaque_scope.clone(),
opaque_type_matcher: self.opaque_type_matcher.clone(),
}
}

pub fn create_new_opaque(&self, top_scope: &str) -> Self {
pub fn create_new_opaque(&self, type_matcher: &str) -> Self {
Self {
data_types: self.data_types.clone(),
depth: self.depth,
for_enum: false,
opaque_scope: Some(top_scope.to_owned()),
opaque_type_matcher: Some(type_matcher.to_owned()),
}
}

Expand All @@ -103,7 +103,7 @@ impl AbiDigester {
data_types: self.data_types.clone(),
depth,
for_enum: false,
opaque_scope: self.opaque_scope.clone(),
opaque_type_matcher: self.opaque_type_matcher.clone(),
})
}

Expand All @@ -116,15 +116,15 @@ impl AbiDigester {
data_types: self.data_types.clone(),
depth,
for_enum: true,
opaque_scope: self.opaque_scope.clone(),
opaque_type_matcher: self.opaque_type_matcher.clone(),
})
}

pub fn digest_data<T: ?Sized + Serialize>(&mut self, value: &T) -> DigestResult {
let type_name = normalize_type_name(type_name::<T>());
if type_name.ends_with("__SerializeWith")
|| (self.opaque_scope.is_some()
&& type_name.starts_with(self.opaque_scope.as_ref().unwrap()))
|| (self.opaque_type_matcher.is_some()
&& type_name.contains(self.opaque_type_matcher.as_ref().unwrap()))
{
// we can't use the AbiEnumVisitor trait for these cases.
value.serialize(self.create_new())
Expand Down Expand Up @@ -661,6 +661,34 @@ mod tests {
#[frozen_abi(digest = "9PMdHRb49BpkywrmPoJyZWMsEmf5E1xgmsFGkGmea5RW")]
type TestBitVec = bv::BitVec<u64>;

mod bitflags_abi {
use crate::abi_example::{AbiExample, EvenAsOpaque, IgnoreAsHelper};

bitflags::bitflags! {
#[frozen_abi(digest = "HhKNkaeAd7AohTb8S8sPKjAWwzxWY2DPz5FvkWmx5bSH")]
#[derive(Serialize, Deserialize)]
struct TestFlags: u8 {
const TestBit = 0b0000_0001;
}
}

impl AbiExample for TestFlags {
fn example() -> Self {
Self::empty()
}
}

impl IgnoreAsHelper 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 ...:
// derive or implement AbiExample/AbiEnumVisitor for
// solana_frozen_abi::abi_digester::tests::_::InternalBitFlags
impl EvenAsOpaque for TestFlags {
const TYPE_NAME_MATCHER: &'static str = "::_::InternalBitFlags";
}
}

mod skip_should_be_same {
#[frozen_abi(digest = "4LbuvQLX78XPbm4hqqZcHFHpseDJcw4qZL9EUZXSi2Ss")]
#[derive(Serialize, AbiExample)]
Expand Down Expand Up @@ -691,4 +719,12 @@ mod tests {
Variant2(u8, u16, #[serde(skip)] u32),
}
}

#[frozen_abi(digest = "B1PcwZdUfGnxaRid9e6ZwkST3NZ2KUEYobA1DkxWrYLP")]
#[derive(Serialize, AbiExample)]
struct TestArcWeak(std::sync::Weak<u64>);

#[frozen_abi(digest = "4R8uCLR1BVU1aFgkSaNyKcFD1FeM6rGdsjbJBFpnqx4v")]
#[derive(Serialize, AbiExample)]
struct TestRcWeak(std::rc::Weak<u64>);
}
109 changes: 84 additions & 25 deletions frozen-abi/src/abi_example.rs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you can see, enabling frozen_abi for banking trace files wasn't so joyful... xD

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,24 @@ use {
std::any::type_name,
};

// The most important trait for the abi digesting. This trait is used to create any complexities of
// object graph to generate the abi digest. The frozen abi test harness calls T::example() to
// instantiate the tested root type and traverses its fields recursively, abusing the
// serde::serialize().
//
// This trait applicability is similar to the Default trait. That means all referenced types must
// implement this trait. AbiExample is implemented for almost all common types in this file.
//
// When implementing AbiExample manually, you need to return a _minimally-populated_ value
// from it to actually generate a meaningful digest. This impl semantics is unlike Default, which
// usually returns something empty. See actual impls for inspiration.
//
// The requirement of AbiExample impls even applies to those types of `#[serde(skip)]`-ed fields.
// That's because the abi digesting needs a properly initialized object to enter into the
// serde::serialize() to begin with, even knowning they aren't used for serialization and thus abi
// digest. Luckily, `#[serde(skip)]`-ed fields' AbiExample impls can just delegate to T::default(),
// exploiting the nature of this artificial impl requirement as an exception from the usual
// AbiExample semantics.
pub trait AbiExample: Sized {
fn example() -> Self;
}
Expand Down Expand Up @@ -137,25 +155,12 @@ tuple_example_impls! {
}
}

// Source: https://github.com/rust-lang/rust/blob/ba18875557aabffe386a2534a1aa6118efb6ab88/src/libcore/array/mod.rs#L417
macro_rules! array_example_impls {
{$n:expr, $t:ident $($ts:ident)*} => {
impl<T> AbiExample for [T; $n] where T: AbiExample {
fn example() -> Self {
[$t::example(), $($ts::example()),*]
}
}
array_example_impls!{($n - 1), $($ts)*}
};
{$n:expr,} => {
impl<T> AbiExample for [T; $n] {
fn example() -> Self { [] }
}
};
impl<const N: usize, T: AbiExample> AbiExample for [T; N] {
fn example() -> Self {
std::array::from_fn(|_| T::example())
}
}
Comment on lines +158 to 162
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elegance of const generics...

off-topic: really wish Default is reimplementd like this as soon as possible...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, I get so annoyed at the lack of Default whenever I use a fixed byte-buffer size


array_example_impls! {32, T T T T T T T T T T T T T T T T T T T T T T T T T T T T T T T T}

// Source: https://github.com/rust-lang/rust/blob/ba18875557aabffe386a2534a1aa6118efb6ab88/src/libcore/default.rs#L137
macro_rules! example_impls {
($t:ty, $v:expr) => {
Expand Down Expand Up @@ -232,7 +237,14 @@ impl<T: BlockType> AbiExample for BitVec<T> {
}

impl<T: BlockType> IgnoreAsHelper for BitVec<T> {}
impl<T: BlockType> EvenAsOpaque 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 ...:
// derive or implement AbiExample/AbiEnumVisitor for
// bv::bit_vec::inner::Inner<u64>
impl<T: BlockType> EvenAsOpaque for BitVec<T> {
const TYPE_NAME_MATCHER: &'static str = "bv::bit_vec::inner::";
}

pub(crate) fn normalize_type_name(type_name: &str) -> String {
type_name.chars().filter(|c| *c != '&').collect()
Expand Down Expand Up @@ -329,13 +341,38 @@ impl<T: AbiExample> AbiExample for std::sync::Arc<T> {
}
}

// When T is weakly owned by the likes of `std::{sync, rc}::Weak`s, we need to uphold the ownership
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the explanation here.
I think this makes sense because if we try serializing a Weak it will upgrade, and always return None without this leakage.

However, I'm not super clear on why we need to have the implementations for Weak at all.
The struct I see it used, which has now become AbiExample, is PinnedVec; but on the PinnedVec we have:

    #[serde(skip)]
    recycler: Weak<RecyclerX<PinnedVec<T>>>,

so shouldn't this Weak be ignored in any serialization? Does that not include the digests we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I'm not super clear on why we need to have the implementations for Weak at all.
...

another good question. yeah, some unspoken nuances are lying here... ;) i did my best to answer that in patch: fd6c28d

I think this makes sense because if we try serializing a Weak it will upgrade, and always return None without this leakage.

yep, i noticed this bug in Weak::example() later after creating this pr and this commit specifically fixed: f5bdb30

as you correctly pointed out, Weak isn't used for actual abi digesting currently. but for perfection, i did the extra effort by adding unit test as well, should we serialize Weak in distant future... ;)

// of T in some way at least during abi digesting... However, there's no easy way. Stashing them
// into static is confronted with Send/Sync issue. Stashing them into thread_local is confronted
// with not enough (T + 'static) lifetime bound.. So, just leak the examples. This should be
// tolerated, considering ::example() should ever be called inside tests, not in production code...
fn leak_and_inhibit_drop<'a, T>(t: T) -> &'a mut T {
Box::leak(Box::new(t))
}

impl<T: AbiExample> AbiExample for std::sync::Weak<T> {
fn example() -> Self {
info!("AbiExample for (Arc's Weak<T>): {}", type_name::<Self>());
// leaking is needed otherwise Arc::upgrade() will always return None...
std::sync::Arc::downgrade(leak_and_inhibit_drop(std::sync::Arc::new(T::example())))
}
}

impl<T: AbiExample> AbiExample for std::rc::Rc<T> {
fn example() -> Self {
info!("AbiExample for (Rc<T>): {}", type_name::<Self>());
std::rc::Rc::new(T::example())
}
}

impl<T: AbiExample> AbiExample for std::rc::Weak<T> {
fn example() -> Self {
info!("AbiExample for (Rc's Weak<T>): {}", type_name::<Self>());
// leaking is needed otherwise Rc::upgrade() will always return None...
std::rc::Rc::downgrade(leak_and_inhibit_drop(std::rc::Rc::new(T::example())))
}
}

impl<T: AbiExample> AbiExample for std::sync::Mutex<T> {
fn example() -> Self {
info!("AbiExample for (Mutex<T>): {}", type_name::<Self>());
Expand Down Expand Up @@ -457,6 +494,13 @@ impl AbiExample for std::path::PathBuf {
}
}

#[cfg(not(target_os = "solana"))]
impl AbiExample for std::time::SystemTime {
fn example() -> Self {
std::time::SystemTime::UNIX_EPOCH
}
}

use std::net::{IpAddr, Ipv4Addr, SocketAddr};
impl AbiExample for SocketAddr {
fn example() -> Self {
Expand All @@ -470,13 +514,22 @@ impl AbiExample for IpAddr {
}
}

// This is a control flow indirection needed for digesting all variants of an enum
// This is a control flow indirection needed for digesting all variants of an enum.
//
// All of types (including non-enums) will be processed by this trait, albeit the
// name of this trait.
// 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).
pub trait AbiEnumVisitor: Serialize {
fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult;
}

pub trait IgnoreAsHelper {}
pub trait EvenAsOpaque {}
pub trait EvenAsOpaque {
const TYPE_NAME_MATCHER: &'static str;
}

impl<T: Serialize + ?Sized> AbiEnumVisitor for T {
default fn visit_for_abi(&self, _digester: &mut AbiDigester) -> DigestResult {
Expand All @@ -489,7 +542,9 @@ impl<T: Serialize + ?Sized> AbiEnumVisitor for T {

impl<T: Serialize + ?Sized + AbiExample> AbiEnumVisitor for T {
default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult {
info!("AbiEnumVisitor for (default): {}", type_name::<T>());
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....
T::example()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instantiating T::example() was a bug, which was uncovered with AbiExample for PacketFlags. surprised this isn't spotted for almost 3.5 years..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, i was wrong... 35dd937

.serialize(digester.create_new())
.map_err(DigestError::wrap_by_type::<T>)
Expand All @@ -501,7 +556,7 @@ impl<T: Serialize + ?Sized + AbiExample> AbiEnumVisitor for T {
// relevant test: TestVecEnum
impl<T: Serialize + ?Sized + AbiEnumVisitor> AbiEnumVisitor for &T {
default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult {
info!("AbiEnumVisitor for (&default): {}", type_name::<T>());
info!("AbiEnumVisitor for &T: {}", type_name::<T>());
// Don't call self.visit_for_abi(...) to avoid the infinite recursion!
T::visit_for_abi(self, digester)
}
Expand All @@ -521,9 +576,13 @@ impl<T: Serialize + IgnoreAsHelper> AbiEnumVisitor for &T {
// inability of implementing AbiExample for private structs from other crates
impl<T: Serialize + IgnoreAsHelper + EvenAsOpaque> AbiEnumVisitor for &T {
default fn visit_for_abi(&self, digester: &mut AbiDigester) -> DigestResult {
info!("AbiEnumVisitor for (IgnoreAsOpaque): {}", type_name::<T>());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this IgnoreAsOpaque was just typo...

let top_scope = type_name::<T>().split("::").next().unwrap();
self.serialize(digester.create_new_opaque(top_scope))
let type_name = type_name::<T>();
let matcher = T::TYPE_NAME_MATCHER;
info!(
"AbiEnumVisitor for (EvenAsOpaque): {}: matcher: {}",
type_name, matcher
);
self.serialize(digester.create_new_opaque(matcher))
.map_err(DigestError::wrap_by_type::<T>)
}
}
Expand Down
5 changes: 5 additions & 0 deletions perf/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ log = { workspace = true }
rand = { workspace = true }
rayon = { workspace = true }
serde = { workspace = true }
solana-frozen-abi = { workspace = true }
solana-frozen-abi-macro = { workspace = true }
solana-metrics = { workspace = true }
solana-rayon-threadlimit = { workspace = true }
solana-sdk = { workspace = true }
Expand All @@ -40,6 +42,9 @@ rand_chacha = { workspace = true }
solana-logger = { workspace = true }
test-case = { workspace = true }

[build-dependencies]
rustc_version = { workspace = true }

[[bench]]
name = "sigverify"

Expand Down
Loading