Enable frozen_abi on banking trace file #33501

ryoqun · 2023-10-03T05:43:37Z

Problem

banking trace files are desired to be soft-frozen for its abi for ease. but there's no guarantee.

Summary of Changes

Apply frozen_abi into TimedTracedEvent to detect any possible abi changes..

no versioning in the trace files are planned. this is just a head up to see how often we touch these relevant structs..

ryoqun · 2023-10-03T06:51:37Z

frozen-abi/src/abi_example.rs

@@ -490,8 +491,7 @@ 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>());
-        T::example()


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

well, i was wrong... 35dd937

ryoqun · 2023-10-03T06:52:26Z

frozen-abi/src/abi_example.rs

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

ryoqun · 2023-10-03T06:53:32Z

perf/build.rs

+    // Copied and adapted from
+    // https://github.com/Kimundi/rustc-version-rs/blob/1d692a965f4e48a8cb72e82cda953107c0d22f47/README.md#example
+    // Licensed under Apache-2.0 + MIT
+    match version_meta().unwrap().channel {
+        Channel::Stable => {
+            println!("cargo:rustc-cfg=RUSTC_WITHOUT_SPECIALIZATION");
+        }
+        Channel::Beta => {
+            println!("cargo:rustc-cfg=RUSTC_WITHOUT_SPECIALIZATION");
+        }
+        Channel::Nightly => {
+            println!("cargo:rustc-cfg=RUSTC_WITH_SPECIALIZATION");
+        }
+        Channel::Dev => {
+            println!("cargo:rustc-cfg=RUSTC_WITH_SPECIALIZATION");
+            // See https://github.com/solana-labs/solana/issues/11055
+            // We may be running the custom `rust-bpf-builder` toolchain,
+            // which currently needs `#![feature(proc_macro_hygiene)]` to
+            // be applied.
+            println!("cargo:rustc-cfg=RUSTC_NEEDS_PROC_MACRO_HYGIENE");
+        }
+    }


copy-pasta without shame because of existing build.rs for solana-perf, instead of the usual symlinking..

ryoqun · 2023-10-03T07:25:55Z

frozen-abi/src/abi_example.rs

+impl<const N: usize, T: AbiExample> AbiExample for [T; N] {
+    fn example() -> Self {
+        std::array::from_fn(|_| T::example())
+    }
 }


elegance of const generics...

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

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

ryoqun · 2023-10-03T14:07:15Z

frozen-abi/src/abi_example.rs

@@ -521,9 +526,17 @@ 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>());


this IgnoreAsOpaque was just typo...

codecov · 2023-10-03T15:23:10Z

Codecov Report

Merging #33501 (fd6c28d) into master (c9d04bc) will increase coverage by 0.0%.
Report is 26 commits behind head on master.
The diff coverage is 98.2%.

@@           Coverage Diff           @@
##           master   #33501   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         802      802           
  Lines      217872   217906   +34     
=======================================
+ Hits       178144   178206   +62     
+ Misses      39728    39700   -28

frozen-abi/src/abi_example.rs

apfitzge · 2023-10-03T16:41:59Z

frozen-abi/src/abi_example.rs

+        let scope = if let Some(scope) = T::SCOPE {
+            scope
+        } else {
+            type_name.split("::").next().unwrap()
+        };


Not sure I understand this works.

By default EvenAsOpaque will have None for SCOPE. In which case, we would split the type_name on ::.
What does type_name include? If we're splitting on :: would this result be the module it's in?

The only place I see SCOPE being set is for PacketFlags where the scope would be some "InternalBitFlags".
Why do we need to replace the scope there?

those are good questions! hope this commit helps your understanding: 95b229f

apfitzge

Changes appear to be fine, but not sure I am understanding why some were necessary.

apfitzge · 2023-10-03T17:38:19Z

Would be good if someone more familiar with the digest system also reviewed this, in addition to me. @brooksprumo maybe?

brooksprumo · 2023-10-03T17:42:19Z

Would be good if someone more familiar with the digest system also reviewed this, in addition to me. @brooksprumo maybe?

😅

What's a banking trace file, and why does/should it have a stable ABI?

apfitzge · 2023-10-03T17:51:17Z

What's a banking trace file, and why does/should it have a stable ABI?

Functionality introduced in #29196 to write all packets coming from SigVerify to BankingStage to file(s).
This has been opt-in so far, but is intended to become the default behavior with #33497

The packets w/ timestamp and labels are written in binary form to a file. We should ensure that the layout does not change, or if it does we should be aware.

brooksprumo · 2023-10-03T18:23:12Z

The packets w/ timestamp and labels are written in binary form to a file. We should ensure that the layout does not change, or if it does we should be aware.

Has this ship already sailed? I think it would be a good fit to use a more robust serialization format that is designed to handle these things. And esp, if we think Firedancer (or other clients) may want to use these trace files as well. Something like Protobuf or Flatbuffers if Firedancer is a yes, otherwise rkyv/etc could be a good choice.

ryoqun · 2023-10-04T04:53:45Z

Would be good if someone more familiar with the digest system also reviewed this, in addition to me. @brooksprumo maybe?

hehe, the whole frozen abi thing is my pet project for years. i doubt anyone knows about its internal workings in depth. ;)

that said, happy to explain it a bit. this pr added good chunk of documentation to the code.

ryoqun · 2023-10-04T05:00:40Z

The packets w/ timestamp and labels are written in binary form to a file. We should ensure that the layout does not change, or if it does we should be aware.

Has this ship already sailed? I think it would be a good fit to use a more robust serialization format that is designed to handle these things. And esp, if we think Firedancer (or other clients) may want to use these trace files as well. Something like Protobuf or Flatbuffers if Firedancer is a yes, otherwise rkyv/etc could be a good choice.

hehe, i considered these alternative serializations. but i ended up choosing serde&bincode for ease of use and good perf (1G bytes / sec). (serialization needs to be fast for banking trace).

Has this ship already sailed?

yeah, kind of. it's not must but i'd like to ensure interop with v1.16's banking trace files.

And esp, if we think Firedancer (or other clients) may want to use these trace files as well.

banking trace files are strictly internal investigation files, firedancer isn't relevant here..

apfitzge · 2023-10-05T16:10:05Z

frozen-abi/src/abi_example.rs

@@ -329,13 +323,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


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?

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... ;)

* Enable frozen_abi on banking trace file * Fix ci with really correct bugfix... * Remove tracker_callers * Fix typo... * Fix AbiExample for Arc/Rc's Weaks * Added comment for AbiExample impl of SystemTime * Simplify and document EvenAsOpaque with new usage * Minor clean-ups * Simplify SystemTime::example() with UNIX_EPOCH... * Add comment for AbiExample subtleties (cherry picked from commit 95810d8) # Conflicts: # Cargo.lock # core/src/banking_trace.rs

* Enable frozen_abi on banking trace file * Fix ci with really correct bugfix... * Remove tracker_callers * Fix typo... * Fix AbiExample for Arc/Rc's Weaks * Added comment for AbiExample impl of SystemTime * Simplify and document EvenAsOpaque with new usage * Minor clean-ups * Simplify SystemTime::example() with UNIX_EPOCH... * Add comment for AbiExample subtleties (cherry picked from commit 95810d8) # Conflicts: # Cargo.lock

* Enable frozen_abi on banking trace file * Fix ci with really correct bugfix... * Remove tracker_callers * Fix typo... * Fix AbiExample for Arc/Rc's Weaks * Added comment for AbiExample impl of SystemTime * Simplify and document EvenAsOpaque with new usage * Minor clean-ups * Simplify SystemTime::example() with UNIX_EPOCH... * Add comment for AbiExample subtleties

* Enable frozen_abi on banking trace file * Fix ci with really correct bugfix... * Remove tracker_callers * Fix typo... * Fix AbiExample for Arc/Rc's Weaks * Added comment for AbiExample impl of SystemTime * Simplify and document EvenAsOpaque with new usage * Minor clean-ups * Simplify SystemTime::example() with UNIX_EPOCH... * Add comment for AbiExample subtleties (cherry picked from commit 95810d8) # Conflicts: # Cargo.lock

…33578) * Enable frozen_abi on banking trace file (#33501) * Enable frozen_abi on banking trace file * Fix ci with really correct bugfix... * Remove tracker_callers * Fix typo... * Fix AbiExample for Arc/Rc's Weaks * Added comment for AbiExample impl of SystemTime * Simplify and document EvenAsOpaque with new usage * Minor clean-ups * Simplify SystemTime::example() with UNIX_EPOCH... * Add comment for AbiExample subtleties (cherry picked from commit 95810d8) # Conflicts: # Cargo.lock * fix conflict --------- Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

ryoqun requested a review from apfitzge October 3, 2023 05:44

ryoqun force-pushed the banking-trace-frozen-abi branch 4 times, most recently from 5674ff1 to 9714597 Compare October 3, 2023 06:33

ryoqun commented Oct 3, 2023

View reviewed changes

frozen-abi/src/abi_example.rs

Copy link

Contributor Author

ryoqun Oct 3, 2023

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

ryoqun commented Oct 3, 2023

View reviewed changes

ryoqun force-pushed the banking-trace-frozen-abi branch from 9714597 to d361605 Compare October 3, 2023 07:22

ryoqun commented Oct 3, 2023

View reviewed changes

apfitzge reviewed Oct 3, 2023

View reviewed changes

frozen-abi/src/abi_example.rs Outdated Show resolved Hide resolved

apfitzge reviewed Oct 3, 2023

View reviewed changes

ryoqun added 8 commits October 4, 2023 14:05

Enable frozen_abi on banking trace file

5e3a8a8

Fix ci with really correct bugfix...

c875a1a

Remove tracker_callers

4a34a58

Fix typo...

368d5a5

Fix AbiExample for Arc/Rc's Weaks

f5bdb30

Added comment for AbiExample impl of SystemTime

56111b7

Simplify and document EvenAsOpaque with new usage

2230884

Minor clean-ups

bd5e2cc

ryoqun force-pushed the banking-trace-frozen-abi branch from 858b5b3 to bd5e2cc Compare October 4, 2023 05:06

ryoqun mentioned this pull request Oct 4, 2023

Define generic AbiExample for OnceLock #33520

Merged

ryoqun requested a review from apfitzge October 4, 2023 05:40

Simplify SystemTime::example() with UNIX_EPOCH...

b564f05

apfitzge reviewed Oct 5, 2023

View reviewed changes

Add comment for AbiExample subtleties

fd6c28d

ryoqun requested a review from apfitzge October 6, 2023 00:53

apfitzge approved these changes Oct 6, 2023

View reviewed changes

ryoqun merged commit 95810d8 into solana-labs:master Oct 7, 2023

ryoqun added v1.16 PRs that should be backported to v1.16 v1.17 PRs that should be backported to v1.17 labels Oct 7, 2023

mergify bot mentioned this pull request Oct 7, 2023

[WIP] v1.16: Enable frozen_abi on banking trace file (backport of #33501) #33577

Closed

mergify bot mentioned this pull request Oct 7, 2023

v1.17: Enable frozen_abi on banking trace file (backport of #33501) #33578

Merged

HaoranYi mentioned this pull request Apr 8, 2024

pr634 1.17 new filter anza-xyz/agave#657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable frozen_abi on banking trace file #33501

Enable frozen_abi on banking trace file #33501

ryoqun commented Oct 3, 2023 •

edited

Loading

ryoqun Oct 3, 2023

ryoqun Oct 3, 2023

ryoqun Oct 3, 2023

ryoqun Oct 3, 2023

ryoqun Oct 3, 2023

apfitzge Oct 3, 2023

ryoqun Oct 3, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading

apfitzge Oct 3, 2023

ryoqun Oct 4, 2023

apfitzge left a comment

apfitzge commented Oct 3, 2023

brooksprumo commented Oct 3, 2023

apfitzge commented Oct 3, 2023

brooksprumo commented Oct 3, 2023

ryoqun commented Oct 4, 2023 •

edited

Loading

ryoqun commented Oct 4, 2023

apfitzge Oct 5, 2023

ryoqun Oct 6, 2023

Enable frozen_abi on banking trace file #33501

Enable frozen_abi on banking trace file #33501

Conversation

ryoqun commented Oct 3, 2023 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

apfitzge commented Oct 3, 2023

brooksprumo commented Oct 3, 2023

apfitzge commented Oct 3, 2023

brooksprumo commented Oct 3, 2023

ryoqun commented Oct 4, 2023 • edited Loading

ryoqun commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

ryoqun commented Oct 4, 2023 •

edited

Loading