ForwardingStage #3222

apfitzge · 2024-10-18T18:22:46Z

Problem

Forwarding packets complicates BankingStage (block-production) tremendously in addition to slowing things down
Need some forwarding to make swqos stuff work (for now)

Summary of Changes

Add a simple and separate forwarding stage so we can remove all the logic from BlockProduction

Fixes #

KirillLykov

Thanks for this refactoring, it simplifies the networking code improvements on my side significantly. Some questions/comments:

My understanding is that before we forwarded only txs that marked to be forwarded by banking stage. Now we just forward all the traffic. If it is correct, what's the purpose to forward txs that are going to be added to the block?
Quite some forwarding metrics have been removed. What are the replacements for total_forwardable_packets and total_dropped_packets?
We use new unbound channel for the ForwardingStage. I guess the assumption is that this stage drain channel immediately and internally decides if it wants to send or drop according to the data budget. The data budget is 12MB/s of staked non-forwarded traffic, which probably means that almost all the traffic will be forwarded. The situation when the channel can grow is when we put into it data faster than we consume in the run's loop. By looking into this code, it doesn't seem to be the case. We take a read lock in poh and allocate memory for the batch. Maybe only if the load is extreme.
Separation between Votes and Txs. In the old code there were two different Forwarders: for votes and for txs. Now it is one ForwardingStage for both, the only concern about it is that if there is high txs traffic we might drop votes due to budget limitations.

KirillLykov · 2024-10-20T14:30:53Z

core/src/banking_stage/transaction_scheduler/scheduler_controller.rs

-                    .zip(fee_budget_limits_vec.drain(..))
-                    .zip(check_results)
-                    .filter(|(_, check_result)| check_result.is_ok())
+            for (((transaction, max_age), fee_budget_limits), _check_result) in transactions


Minor, I wonder if it is possible to simplify with izip!

yeah potentially possible but not gonna change it in this PR

KirillLykov · 2024-10-20T14:38:10Z

core/src/forwarding_stage.rs

+    udp_socket: UdpSocket,
+}
+
+impl<T: LikeClusterInfo> ForwardingStage<T> {


Am I right that there are two types of ForwardingStage -- one for txs and one for TpuVotes? In this, case it makes sense to extract structure to handle network -- one for txs and one for votes because they are delivered using different protocols. This can be done separately by me.

There is a single ForwardingStage that handles both votes and non-votes, although it handles them differently.

KirillLykov · 2024-10-20T14:39:45Z

core/src/forwarding_stage.rs

+            .map(|packet| packet.meta().is_simple_vote_tx())
+            .unwrap_or(false)
+    }
+}


Maybe add tests here similar to what forwarder had?

yeah this was draft because i wanted to see what broke (honestly surprised to see it is nothing), before I went back and added tests.

KirillLykov · 2024-10-20T14:48:33Z

core/src/tpu.rs

@@ -199,15 +201,22 @@ impl Tpu {
        )
        .unwrap();

+        let (forward_stage_sender, forward_stage_receiver) = unbounded();


I think it should be bound, this way we are explicit about dropping some traffic in case of network problems. What currently happens is that we add futures sending txs batches to the runtime and if there are lot of batches and sending is slow, this queue will grow unnecessary.

Agree. Backpressure handling could just be dropping txs on the floor and incrementing a counter. This would at least make it obvious if forwarding stage can't keep up

KirillLykov · 2024-10-20T15:36:40Z

core/src/sigverify.rs

-        )))?;
+        let banking_packet_batch =
+            BankingPacketBatch::new((packet_batches, Some(tracer_packet_stats_to_send)));
+        if let Some(forward_stage_sender) = &self.forward_stage_sender {


Does the order of sending matters?

if let Some(forward_stage_sender) = &self.forward_stage_sender { forward_stage_sender.send(banking_packet_batch.clone())?; } self.banking_stage_sender.send(banking_packet_batch)?;

Only in that if I had to choose a stage to have lower latency it'd definitely be banking stage

KirillLykov · 2024-10-21T11:03:53Z

core/src/banking_stage/unprocessed_transaction_storage.rs

@@ -364,25 +343,6 @@ impl UnprocessedTransactionStorage {
        }
    }

-    pub fn filter_forwardable_packets_and_add_batches(


Before forwarder used this to filter outdated transactions, now we skip this. Is this filtering done somewhere upper in the stack now?

It's not, we can add addtional filtering in ForwardingStage.

Was trying to keep it as simple as possible for v0 because what filtering we actually care about doing is somewhat questionable.

We only forward tpu votes and partnered connection's non-votes.
votes I think ideally we'd check age and possibly that they are valid staked votes.
non-votes that we forward are coming from our partnered connections, so these should be somewhat trusted in terms of content...if they are spamming old transactions then they are wasting their money paying me to forward.

apfitzge · 2024-10-21T13:22:34Z

My understanding is that before we forwarded only txs that marked to be forwarded by banking stage. Now we just forward all the traffic. If it is correct, what's the purpose to forward txs that are going to be added to the block?

We do not forward all traffic.

marked to be forwarded by banking stage

Unsure what you mean by "marked" here.
We only forward tpu votes and non-votes from our swqos side-deal partners, in either the old or new case.

Quite some forwarding metrics have been removed. What are the replacements for total_forwardable_packets and total_dropped_packets?

100% we should add back in some metrics. As I stated in a comment above, I wanted to get this through CI to see what broke without making ForwardingStage any more complicated than it needed to be. I will adds tests and metrics before putting PR into review.

We use new unbound channel for the ForwardingStage.

Let's just simplify and make it a bounded channel. We can have a fairly small bound as well I think; signature verification should take longer than just spawning off some tasks to forward (I would guess, but need to actually bench/profile this)

Separation between Votes and Txs. In the old code there were two different Forwarders: for votes and for txs. Now it is one ForwardingStage for both, the only concern about it is that if there is high txs traffic we might drop votes due to budget limitations.

While true that there were different threads doing the forwarding in the old setup, there was still a single DataBudget that was shared between them.
It is slightly different in that vote packets will be mixed in with non-votes in channel insert order, rather than a likely flip-flopping between threads.

KirillLykov · 2024-10-21T14:46:26Z

We only forward tpu votes and non-votes from our swqos side-deal partners, in either the old or new case.

@apfitzge This is something I don't fully understand from code. How do we check in the code that the transaction is from these partners? By checking that they are coming from staked connection?

We do not forward all traffic.

I might misunderstand how it was: my impression was that during the banking stage we processed some transactions during the interval of 400ms and those which we didn't manage to process during this period of time we forwarded (if not hold). I'm under the impression that now we forward regardless of the banking stage decision which confuses me.

apfitzge · 2024-10-21T16:57:59Z

By checking that they are coming from staked connection?

Exactly. It's either from a staked partner or it is a tpu vote (also staked). Both of which we may want to forward.

I might misunderstand how it was: my impression was that during the banking stage we processed some transactions during the interval of 400ms and those which we didn't manage to process during this period of time we forwarded (if not hold). I'm under the impression that now we forward regardless of the banking stage decision which confuses me.

If I am an operator and have a deal to forward some partners packets, then we just do that regardless of what banking stage is doing.
We are not trying to match the old behavior, because the old behavior is fundamentally bad for both block-production and honoring the swqos side deal(s).
If I do not process the packets it doesn't really matter if I forwarded them or not.

If we wait until after leader slots to forward, it is useless to do forwarding at that point.
If we forward a transaction we end up including. It's a jsut bit of extra network traffic.

KirillLykov · 2024-10-23T09:24:23Z

To me this PR looks a very good move forward, hence lgtm. I've done already replacement of the client code on top of this PR, so biased to have it merged. But would be good to see approvals from others.

apfitzge · 2024-10-23T14:03:33Z

@t-nelson @alessandrod or @tao-stones. Would like to get this in v2.1 so there's one less version to maintain this

apfitzge · 2024-10-23T20:51:16Z

This is a dead-simple forwarding approach.
It does not have prioritization.

Maybe we should hold off on merging this until 2.2, so we have time to add prioritization in the above stages so that this will only pop the highest priority packets when we go to forward.

KirillLykov · 2024-10-26T13:21:17Z

@apfitzge I'm under the impression that the Forwader actually sorts txs by priority. So here txs ordering by priority is skipped with an idea to add it later again? Is it because adding this priority ordering to ForwardingStage is impossible without some work done on other components?

apfitzge · 2024-10-26T13:26:39Z

@apfitzge I'm under the impression that the Forwader actually sorts txs by priority. So here txs ordering by priority is skipped with an idea to add it later again? Is it because adding this priority ordering to ForwardingStage is impossible without some work done on other components?

Yeah sorry should have put this back into draft!
I'm working on adding forwarding in this stage for now. Eventually that behavior can and should go upstream

bw-solana

🤯 this is awesome. holding to add priority ordering or is there more to it?

bw-solana · 2024-11-15T02:05:09Z

core/src/forwarding_stage.rs

+        let (success, num_packets) = if tpu_vote_batch {
+            let packets: Vec<_> = filtered_packets.into_iter().zip(repeat(addr)).collect();
+            let num_packets = packets.len();
+            let res = batch_send(&self.udp_socket, &packets);


@lijunwangs just a heads up that this is coming and we'll need to add quic support for forwarded votes. This is still in draft though

bw-solana · 2024-11-15T02:05:28Z

core/src/forwarding_stage.rs

+        }
+    }
+
+    fn run(mut self) {


This is too easy to understand. Please complicate

apfitzge · 2024-11-15T13:57:05Z

holding to add priority ordering or is there more to it?

Yeah, have been working in the background to make cost-model not need us to resolve addresses to calculate; Should make this much better w/ no* allocations.

*unfortunately found an edge-case in cost-model code the other day, and unless I write a new parser for SystemInstruction I can't get rid of the allocation. Might do this, but not yet.

apfitzge added the noCI Suppress CI on this Pull Request label Oct 18, 2024

apfitzge self-assigned this Oct 18, 2024

ForwardingStage: skeleton

41fdcf2

apfitzge force-pushed the dumb_forwarder branch from bd528e1 to 41fdcf2 Compare October 18, 2024 18:27

apfitzge added 8 commits October 18, 2024 14:22

dumb as dirt forwarder

76d511b

kill forwarding

4aa5b9f

remove more forwarding shit

4e6c428

scheduler no buffer packets

080f2dc

remove unused metrics

a0e51cb

remove unused metrics

64c3fde

remove unused function

d47d605

validator config arg better name

7bf7827

apfitzge added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request labels Oct 18, 2024

legacy clear if not close to leader

1205a8a

anza-team removed the CI Pull Request is ready to enter CI label Oct 18, 2024

apfitzge added 2 commits October 18, 2024 15:19

better spot to cache epoch stuff

a0534d0

fix bug - remove unused arc_packets vec

0f876e0

KirillLykov reviewed Oct 21, 2024

View reviewed changes

apfitzge added 8 commits October 21, 2024 09:48

use bounded channel

3dff7cb

correctly detect tpu votes

53b3a89

only get address

076eca6

lazier collection

4eeca43

minor refactoring

c3f366c

ForwardAddressGetter

d6da5af

test_tpu_vote_forwarding

9ddba48

refactor test

08f69e5

discard filter

660659f

apfitzge added 2 commits October 21, 2024 13:01

tests

f321eb5

simple metrics

fd4d8e1

apfitzge marked this pull request as ready for review October 21, 2024 19:41

apfitzge requested review from alessandrod, t-nelson and tao-stones October 21, 2024 19:41

KirillLykov approved these changes Oct 23, 2024

View reviewed changes

apfitzge mentioned this pull request Oct 29, 2024

Remove Legacy Banking Stage #3357

Open

4 tasks

apfitzge marked this pull request as draft November 4, 2024 17:32

bw-solana reviewed Nov 15, 2024

View reviewed changes

bw-solana linked an issue Jan 3, 2025 that may be closed by this pull request

Performance: Forwarding #4265

Open

bw-solana mentioned this pull request Jan 4, 2025

Performance: Forwarding #4265

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ForwardingStage #3222

ForwardingStage #3222

apfitzge commented Oct 18, 2024

KirillLykov left a comment •

edited

Loading

KirillLykov Oct 20, 2024

apfitzge Oct 21, 2024

KirillLykov Oct 20, 2024

apfitzge Oct 21, 2024

KirillLykov Oct 20, 2024

apfitzge Oct 21, 2024 •

edited

Loading

KirillLykov Oct 20, 2024

bw-solana Nov 15, 2024

KirillLykov Oct 20, 2024

apfitzge Oct 21, 2024

KirillLykov Oct 21, 2024

apfitzge Oct 21, 2024

apfitzge commented Oct 21, 2024

KirillLykov commented Oct 21, 2024 •

edited

Loading

apfitzge commented Oct 21, 2024

KirillLykov commented Oct 23, 2024

apfitzge commented Oct 23, 2024

apfitzge commented Oct 23, 2024

KirillLykov commented Oct 26, 2024

apfitzge commented Oct 26, 2024

bw-solana left a comment

bw-solana Nov 15, 2024

bw-solana Nov 15, 2024

apfitzge commented Nov 15, 2024

ForwardingStage #3222

Are you sure you want to change the base?

ForwardingStage #3222

Conversation

apfitzge commented Oct 18, 2024

Problem

Summary of Changes

KirillLykov left a comment • edited Loading

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 Oct 21, 2024 • edited Loading

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

apfitzge commented Oct 21, 2024

KirillLykov commented Oct 21, 2024 • edited Loading

apfitzge commented Oct 21, 2024

KirillLykov commented Oct 23, 2024

apfitzge commented Oct 23, 2024

apfitzge commented Oct 23, 2024

KirillLykov commented Oct 26, 2024

apfitzge commented Oct 26, 2024

bw-solana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Nov 15, 2024

KirillLykov left a comment •

edited

Loading

apfitzge Oct 21, 2024 •

edited

Loading

KirillLykov commented Oct 21, 2024 •

edited

Loading