Add fully-reproducible online tracer for banking #29196

ryoqun · 2022-12-10T13:29:58Z

Problem

It's hard to reproduce/examine past banking-related performance problems which are observed only at mainnet-beta.

The underlying problem is that it's not easy to create a similar load on test/dev environments. Worse, we also don't have good visibility of actual load due to limited set of metrics, which are indirect by nature and aggregated in ad-hoc definitions.
This is especially so for banking, say unlike replay performance. That's because any discarded packets are unavailable afterwards like later investigating/benchmarking.

Lastly, the performance profile of banking isn't linear by any means as well. There's various factors affecting its throughput (account access pattern, qos, resource contentions across banking threads...)

Summary of Changes

So, let's get to bottom of this.

Namely, start to collect all of REAL trace data fed into BankingStage on production environment.
Once provided from validator-node operators, we will be able to consume these trace data later with solana-ledger-tool simulate-leader-blocks (not included in this pr)

The obvious concerns is of any overhead. So, background trace writer thread is introduced and its work is rather simple: just writing packet blobs. Casual testing shows 1GB/s write throughput is possible.

On the other hand, any critical-code path's overhead will be creating/cloning Arcs and sending PacketBatches off over crossbeam_channel. Specifically, this results in additional 100ns per Vec<PacketBatch>, which should be negligible.

Also, the trace file will be discarded 14days old or more than 14G is written. So, ram/disk usage is capped.
Hard to imagine 10G link (if employed) is fully saturated under quic, even if all of three banking channels (non-vote, tpu-vote, gossip-vote) are combined.

So, i think attack are correctly mitigated as well.

Currently, banking tracing is opt-in. but planned to be promoted to opt-out.

along side the obvious packets tracing, banking start and end events are recorded as well.

Also, backport to mb is desired. the pr is prepared in that mind (I'll spin off a preparatory cleanup pr...).
Unlike all of this other draft my prs, this pr is ready for review (except the diff size, warranting the cleanup pr....) and I've polished it up to the extreme.

spin-off pr statuses/tasks

apfitzge

Took a short look this morning, leaving some initial comments. Still need to make it through the remaining 90%.

Side note: from the looks of it, I'm guessing you and I will have a lot of conflicts in the near future as I tried splitting up BankingStage into different sub-components (DecisionMaking, Consuming, Forwarding, Committing). As I post PRs for those I'll be sure to include you so we can make coordinating easier.

core/src/banking_trace.rs

core/Cargo.toml

core/src/banking_trace.rs

core/src/packet_deserializer.rs

ryoqun · 2022-12-10T23:04:54Z

Took a short look this morning, leaving some initial comments. Still need to make it through the remaining 90%.

appreciate quick feedback! however, please remind that I'm not forcing you a weekend code reviews.. it's just that I'm working 24/7 whenever i find some spare time. ;)

Side note: from the looks of it, I'm guessing you and I will have a lot of conflicts in the near future as I tried splitting up BankingStage into different sub-components (DecisionMaking, Consuming, Forwarding, Committing). As I post PRs for those I'll be sure to include you so we can make coordinating easier.

got it. although there will be some conflicts, i don't think this tracer thing is duplicating your works. if wanted, i can bite the intense git merge conflict session. :)

core/src/banking_trace.rs

core/benches/banking_trace.rs

sdk/src/packet.rs

validator/src/cli.rs

core/src/banking_trace.rs

ryoqun · 2022-12-15T13:58:56Z

@apfitzge hey, thanks for very detailed review comments! I'll address these shortly after with joy! also, this is very hot press but the simulation code is here as requested: ryoqun#2 (it's VERY rough...)

ryoqun · 2022-12-20T14:29:27Z

@apfitzge i haven't fully replied to your all comments. however, could you look again at your convenient time as i pushed numerous commits to address them? also, i put bunch of my 2 cents. ;)

536: Mention "Bytes" on README.md and lib.rs r=jonasbb a=ryoqun I think #277 can be more promoted. :) As I wrote the current status of serialization of const generics like bytes (solana-labs/solana#29196 (comment)), i think `"Bytes"` is best solution so far. `@jonasbb` hey, this functionary saved us. however it took a while to spot this. I think `Bytes` can be promoted. :) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

mschneider · 2023-01-13T01:29:34Z

Really cool idea, I actually though we might get a similar trace by just capturing the quic network stream on a switch and stream copy it to another machine for recording. Having this as a software feature is of course way more accessible

apfitzge · 2023-01-13T20:48:23Z

Really cool idea, I actually though we might get a similar trace by just capturing the quic network stream on a switch and stream copy it to another machine for recording. Having this as a software feature is of course way more accessible

Also has an added benefit of somewhat isolating it from sigverify time, as we get the time packets enter banking stage after being verified 😄

ryoqun · 2023-01-25T04:27:36Z

core/src/banking_trace.rs

+    #[error("Integer Cast Error: {0}")]
+    IntegerCastError(#[from] std::num::TryFromIntError),
+
+    #[error("Trace directory's byte limit is too small (must be larger than {1}): {0}")]


apfitzge

lgtm - happy with how this ended up and thanks for all the hard work @ryoqun. Love the commit links you leave for resolving each conversation

Pull request has been modified.

ryoqun requested review from apfitzge and tao-stones December 10, 2022 13:29

ryoqun mentioned this pull request Dec 10, 2022

2xTx: Refactor to separate Transaction packets from MTU packets #29055

Closed

apfitzge reviewed Dec 10, 2022

View reviewed changes

ryoqun force-pushed the banking-tracer branch from 92e59ea to 23aadb1 Compare December 10, 2022 23:33

apfitzge self-requested a review December 12, 2022 15:33

apfitzge reviewed Dec 12, 2022

View reviewed changes

ryoqun force-pushed the banking-tracer branch 2 times, most recently from ca94c60 to db4732c Compare December 20, 2022 14:19

apfitzge self-requested a review December 20, 2022 23:26

ryoqun mentioned this pull request Dec 21, 2022

Banking tracer with simulator wip ryoqun/solana#2

Draft

ryoqun force-pushed the banking-tracer branch 3 times, most recently from 1482637 to b76ed3d Compare December 21, 2022 13:03

ryoqun mentioned this pull request Dec 23, 2022

Mention "Bytes" on README.md and lib.rs jonasbb/serde_with#536

Merged

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 6, 2023

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 13, 2023

ryoqun force-pushed the banking-tracer branch from bb256bc to ff05cb8 Compare January 14, 2023 13:44

ryoqun mentioned this pull request Jan 14, 2023

Bump chrono to v0.4.23 #29709

Merged

ryoqun force-pushed the banking-tracer branch from ff05cb8 to 0d2e227 Compare January 15, 2023 03:10

ryoqun mentioned this pull request Jan 15, 2023

Remove PacketDeserializer's extra overflow guard #29715

Merged

ryoqun force-pushed the banking-tracer branch from 4a5c9a5 to 3b25734 Compare January 15, 2023 12:56

ryoqun mentioned this pull request Jan 15, 2023

Rename remaining ones to replay_vote_{sender,receiver} #29716

Merged

ryoqun force-pushed the banking-tracer branch from 3b25734 to a77b6e3 Compare January 18, 2023 06:12

ryoqun added 18 commits January 25, 2023 12:47

Tweak logging and cli

874820e

Remove Bank events as it's not needed anymore

33f6fb8

Make tpu own banking tracer thread

3e83eac

Reduce diff a bit..

c4b90bb

Use latest serde_with

6c5dd57

Finally use the published rolling-file crate

88b2b50

Make test code change more consistent

907d59f

Revive dead and non-terminating test code path...

8755444

Dispose batches early now that possible

8461c95

Split off thread handle very early at ::new()

9c2d4c7

Tweak message for TooSmallDirByteLimitl

b4ea132

Remove too much of indirection

c754b4e

Remove needless pub from ::channel()

10cf905

Clarify test comments

22ff340

Avoid needless event creation if tracer is disabled

8ca2529

Write tests around file rotation and spill-over

30971c3

Remove unneeded PathBuf::clone()s...

420d95e

Introduce inner struct instead of tuple...

54042b2

ryoqun force-pushed the banking-tracer branch from 14ec1d1 to 54042b2 Compare January 25, 2023 03:50

ryoqun commented Jan 25, 2023

View reviewed changes

apfitzge previously approved these changes Jan 25, 2023

View reviewed changes

ryoqun added 2 commits January 25, 2023 16:32

Remove unused enum BankStatus...

b7ac53b

Avoid .unwrap() for the case of disabled tracer...

84e9520

ryoqun force-pushed the banking-tracer branch 2 times, most recently from 947701c to 84e9520 Compare January 25, 2023 08:26

ryoqun merged commit 40bbf99 into solana-labs:master Jan 25, 2023

ryoqun mentioned this pull request Jul 9, 2023

dcou: mod banking_trace::for_test #32428

Merged

apfitzge mentioned this pull request Oct 3, 2023

Enable frozen_abi on banking trace file #33501

Merged

ryoqun mentioned this pull request Aug 25, 2024

Introduce ledger-tool simulate-block-production anza-xyz/agave#2733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fully-reproducible online tracer for banking #29196

Add fully-reproducible online tracer for banking #29196

ryoqun commented Dec 10, 2022 •

edited

Loading

apfitzge left a comment

ryoqun commented Dec 10, 2022

ryoqun commented Dec 15, 2022 •

edited

Loading

ryoqun commented Dec 20, 2022

mschneider commented Jan 13, 2023

apfitzge commented Jan 13, 2023

ryoqun Jan 25, 2023

apfitzge left a comment

Add fully-reproducible online tracer for banking #29196

Add fully-reproducible online tracer for banking #29196

Conversation

ryoqun commented Dec 10, 2022 • edited Loading

Problem

Summary of Changes

spin-off pr statuses/tasks

apfitzge left a comment

Choose a reason for hiding this comment

ryoqun commented Dec 10, 2022

ryoqun commented Dec 15, 2022 • edited Loading

ryoqun commented Dec 20, 2022

mschneider commented Jan 13, 2023

apfitzge commented Jan 13, 2023

ryoqun Jan 25, 2023

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

ryoqun commented Dec 10, 2022 •

edited

Loading

ryoqun commented Dec 15, 2022 •

edited

Loading