TransactionView: ReceiveAndBuffer #3820

apfitzge · 2024-11-27T16:50:50Z

Problem

Need to ingest new tx type into banking stage

Summary of Changes

Implement a new TransactionViewReceiveAndBuffer to allow banking stage to ingest TransactionView-based transactions
Add argument to BankingStage::new to switch between transaction representations
Hook up CLI for validator and relevant benchmarks
Run all BankingStage and SchedulerController tests using both options

Fixes #

apfitzge · 2024-12-02T17:13:29Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

+    type Transaction = RuntimeTransaction<ResolvedTransactionView<Bytes>>;
+    type Container = TransactionViewStateContainer;
+
+    fn receive_and_buffer_packets(


NOTE: this does not run the bank.check_transactions because I prioritized safety around not leaking Bytes in the container, which is much trickier if we do batching.
We should probably follow this up with a call to clean immediately after, so that we check age on transactions we actually inserted, which is likely better anyway. However, I'd like to profile with and without that change first.

We also should make sure we are checking the status cache to make sure we're not scheduling transactions that have already been processed right?

Bank::check_transactions checks the age and status cache. so it'll clean out both types before we get to the processing.

Yeah sorry my comment wasn't clear. I bring it up because it's kind of a bug to not check status cache before scheduling right?

We do check before scheduling. I'm just not checking before buffering in this PR.

I think with some of the other clean ups we did during review it may be easier to add in. Let me give it a shot this afternoon, and I'll comment here on what I got.

Okay, taking a look at this. I think we can do it relatively easily during buffering; but it may require some refactoring to do it in the optimal point.

We'd really want to check age/status immediately after we calculate the message hash; but the type we have at that point RuntimeTransaction<SanitizedTransactionView> doesn't implement SVMMessage (addresses not resolved).
That way we don't need to reach into accounts-db (slower) before we discard too old or already processed txs.

I think it's probably better to refactor the internals of check_transactions so we can call it as long as we have the signature/message hash (for SVMMessage we can have fn that just grabs those and calls the actual impl).

^ Given those additional changes, think will leave it as a follow-up PR.

Works for me

apfitzge · 2024-12-03T16:05:45Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

+        if container.is_empty()
+            && matches!(
+                decision,
+                BufferedPacketsDecision::Forward | BufferedPacketsDecision::ForwardAndHold


removed Hold branch here. Sometimes after a leader slot, we do not have a bank ready in Consume for our next leader slot. We should still avoid the recv_timeout in that case. Also added a check that container is not empty - if container has items, then we should try to process them instead of sleeping for packets

pgarg66

Looks good from SVM point of view.

bw-solana

No major qualms with what you have. Left a few comments to consider

core/src/banking_stage.rs

validator/src/main.rs

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

jstarry

Still reviewing but posting some feedback along the way

core/src/banking_stage/transaction_scheduler/transaction_state.rs

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

transaction-view/src/transaction_data.rs

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

core/src/banking_stage/transaction_scheduler/prio_graph_scheduler.rs

core/src/banking_stage.rs

apfitzge · 2024-12-10T15:14:27Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

    fn receive_and_buffer_packets(
        &mut self,
        container: &mut Self::Container,
        timing_metrics: &mut SchedulerTimingMetrics,
        count_metrics: &mut SchedulerCountMetrics,
        decision: &BufferedPacketsDecision,
-    ) -> bool;
+    ) -> Result<usize, ()>;


This change was to improve the testing which was seeing some inconsistent failures, due to timing out in this call.

Those tests now loop until this call returns Ok(0) - or an error.

Natural question - Why did this only pop up for TransactionView?

The receive_and_buffer timeout for view is done differently than for sdk types.
The sdk types have a receive timeout, i.e. a time limit to receive packets, and then will deserialize all packets it received regardless of the time that takes.

The view type has an overall timeout so that it does not take longer than specified time for all operations - receive AND parsing.

Since receiving takes almost no time/work, this consistently would receive all the batches sent in the sdk variant. and then it would take however long to deserialize them - whether or not OS scheduler decides to rug the thread doesn't matter. If the view variant of the test gets rugged during parsing, then it can time out and not receive the next message.

Why do we have different timeout implementations between the two transaction structs? It feels more correct to have the timeout apply to both receive and buffer but it will likely mean that we buffer fewer transactions before processing right?

I plan on also changing the sdk approach to be timed out on receive AND buffer, since I agree this definitely seems more correct...but only after forwarding and ImmutableDeserializedPacket is removed; all the current receiving code needs to be reworked but doesn't make sense to rework it until IDP is dead imo.

But i've held off removing forwarding on getting this merged first, since when i posted there was some other stuff that needed to get merged for forwarding removal; in hindsight it may have been the wrong order.

Ok thanks for the context. So is it ok that we start processing transactions without buffering so many? Seems like the main loop in the scheduler controller is fairly lightweight besides doing work for the pre_graph_filter inside process_transactions. If we end up running more shorter scheduler loops we will still be putting later received transactions into the top of the priograph if they pay high pri fees right?

The vast majority of packets we receive come in slightly before we become leader.

I had a heatmap of this at one time, and things may have changed. (I should probably grab some trace data and re-generate it!)
There used to be a small steady-state of packets coming in in the minute before the first leader slot. Then around 5-10 slots before becoming leader theres a huge spike of traffic, but it fell off very quickly with much fewer packets arriving during our leader slots.

All to say, it should be fine to run shorter calls to schedule if our loop is fast enough at returning to it. We essentially always want to keep some amount of transactions queued to each worker thread once we are leader, so the calls between schedule should just be enough that the scheduler is able to keep up with how fast the workers process and queue more up as they complete the work.

apfitzge · 2024-12-30T19:10:26Z

@tao-stones @jstarry if you are back from vacation(s) would really appreciate review on this.
Somewhat significant changes to banking and I've been holding up some other stuff until this gets merged to avoid having 100 conflicts

tao-stones

lgtm

core/src/banking_stage/packet_filter.rs

apfitzge · 2025-01-09T20:03:53Z

@jstarry - rebased to fix merge conflicts with #4123

apfitzge · 2025-01-14T13:54:41Z

@pgarg66 adding you to review for @anza-xyz/svm

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch 4 times, most recently from 2876a3f to c868ff5 Compare November 27, 2024 17:17

apfitzge commented Dec 2, 2024

View reviewed changes

apfitzge marked this pull request as ready for review December 2, 2024 18:50

apfitzge requested a review from a team as a code owner December 2, 2024 18:50

apfitzge requested review from jstarry and tao-stones December 2, 2024 18:50

apfitzge self-assigned this Dec 2, 2024

apfitzge requested a review from bw-solana December 2, 2024 20:23

apfitzge commented Dec 3, 2024

View reviewed changes

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from f87bd2f to 2d9cc88 Compare December 3, 2024 17:54

pgarg66 reviewed Dec 3, 2024

View reviewed changes

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 2d9cc88 to e310dd7 Compare December 5, 2024 17:06

apfitzge marked this pull request as draft December 5, 2024 22:06

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 4e927a9 to a8c7a41 Compare December 6, 2024 14:00

apfitzge marked this pull request as ready for review December 6, 2024 15:29

bw-solana reviewed Dec 9, 2024

View reviewed changes

jstarry reviewed Dec 9, 2024

View reviewed changes

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 6ccd99a to 19fb288 Compare December 9, 2024 23:13

apfitzge commented Dec 10, 2024

View reviewed changes

ryoqun mentioned this pull request Dec 11, 2024

tracking issue (brain dump): unified scheduler in block-production #3832

Open

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 2b8807d to d579b84 Compare December 11, 2024 18:32

apfitzge requested a review from jstarry December 11, 2024 20:46

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch 2 times, most recently from 05779cd to f2c9d44 Compare December 13, 2024 16:26

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from f2c9d44 to cee2ff5 Compare January 2, 2025 15:52

tao-stones previously approved these changes Jan 2, 2025

View reviewed changes

core/src/banking_stage/packet_filter.rs Show resolved Hide resolved

apfitzge added 18 commits January 9, 2025 14:02

minor sleeping efficiency improvement

e4d5b94

impl TransactionData for Arc<Vec<u8>>

81d1826

SharedBytes = Arc<Vec<u8>>

db9ade6

only warn about forwarding if not already using Sdk

1a05916

fix bug on should_forward

183a4fd

test_initialize_should_forward

da2420d

remove option

393a8f8

remove impl TransactionData for Bytes

9c41a65

get_vacant_map_entry

4e35359

Safety comment

58e3aae

remove SuccessfulInsert type

aa2bff1

type aliases

c215f86

remove explicit drop

fcccad1

Remove timeout from testing

6e394f4

should_forward_from_meta

113b9ca

typo

ecd7fb6

secp256r1

63bef62

vote-only discarding

7fe132f

apfitzge dismissed jstarry’s stale review via 7fe132f January 9, 2025 20:02

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 4c4bae1 to 7fe132f Compare January 9, 2025 20:02

apfitzge requested a review from a team as a code owner January 9, 2025 20:02

apfitzge requested a review from jstarry January 13, 2025 15:21

jstarry approved these changes Jan 14, 2025

View reviewed changes

apfitzge requested a review from pgarg66 January 14, 2025 13:54

apfitzge mentioned this pull request Jan 14, 2025

Apply more unified scheduler related core cleanups #4453

Open

pgarg66 approved these changes Jan 14, 2025

View reviewed changes

apfitzge merged commit 535ea1e into anza-xyz:master Jan 14, 2025
52 checks passed

apfitzge deleted the new_receive_and_buffer_2024_11_26 branch January 14, 2025 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransactionView: ReceiveAndBuffer #3820

TransactionView: ReceiveAndBuffer #3820

apfitzge commented Nov 27, 2024

apfitzge Dec 2, 2024

jstarry Jan 3, 2025

apfitzge Jan 9, 2025

jstarry Jan 9, 2025

apfitzge Jan 9, 2025

apfitzge Jan 9, 2025 •

edited

Loading

jstarry Jan 10, 2025

apfitzge Dec 3, 2024 •

edited

Loading

pgarg66 left a comment

bw-solana left a comment

jstarry left a comment

apfitzge Dec 10, 2024

jstarry Jan 3, 2025

apfitzge Jan 3, 2025

jstarry Jan 7, 2025

apfitzge Jan 9, 2025

apfitzge commented Dec 30, 2024

tao-stones left a comment

apfitzge commented Jan 9, 2025

apfitzge commented Jan 14, 2025

TransactionView: ReceiveAndBuffer #3820

TransactionView: ReceiveAndBuffer #3820

Conversation

apfitzge commented Nov 27, 2024

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

apfitzge Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

pgarg66 left a comment

Choose a reason for hiding this comment

bw-solana left a comment

Choose a reason for hiding this comment

jstarry left a 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

apfitzge commented Dec 30, 2024

tao-stones left a comment

Choose a reason for hiding this comment

apfitzge commented Jan 9, 2025

apfitzge commented Jan 14, 2025

apfitzge Jan 9, 2025 •

edited

Loading

apfitzge Dec 3, 2024 •

edited

Loading