Don't forward packets received from TPU forwards port

[jstarry]

Problem

Transactions received on the TPU forwards port should only be forwarded one hop but are actually forwarded multiple hops until they expire.

Summary of Changes

• Mark packets received from the TPU forwards port as "forwarded"
• Skip forwarding packets that were already forwarded once

Fixes #

[jstarry]

No test yet but just want to see if we agree on this direction. Any detractors? I think there's risk of tx land rate to drop with this change but send tx service should make up for that.

[codecov]

Codecov Report

Merging #22078 (2ccf965) into master (ddde356) will decrease coverage by 0.0%.
The diff coverage is 78.0%.

@@            Coverage Diff            @@
##           master   #22078     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         518      518             
  Lines      145734   145812     +78     
=========================================
+ Hits       118554   118595     +41     
- Misses      27180    27217     +37     

[t-nelson]

Logic changes looks good to me

[behzadnouri]

My suggestion is not to add any port-based logic to the validator:

• This was already recognized in Validators should not make decisions based on which ports data was received #6672
  That specific issue is about shreds but similar ideas can apply other places.
• The thing is it is ridiculously easy for a bad actor to get around any port-based decision. So at the end of the day the logic will only be enforced on the good actors, counterproductively, increasing the effectiveness of the spams/attacks done by the bad actors.
  • Not having a port-based logic is just one less knob an attacker can abuse, so it is one less attack surface.
• FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET and max_tx_fwd_delay are sort of arbitrary. If these values are off with respect to state of the cluster, then with this commit forwarded txs are likely to just be dropped erroneously.

Besides the problems with port-based decisions, it is not very evident that forwarded transactions should be treated this way. There was actually a counter argument on discord that forwarded txs should be prioritized because:

bots that aggressively broadcast to the next n leaders have a definite advantage over user tx just send to the next two
https://discord.com/channels/428295358100013066/738253477331075162/918866791718080562

I am not necessarily in favor of that argument, but just pointing out nuances involved. Primarily I am in favor just not adding any port-based logic to the validator.

[jstarry]

Thanks for weighing in @behzadnouri!

Validators should not make decisions based on which ports data was received

Agreed 100%, this is not secure if we don't check if the packet came from a legitimate source. In this case, after the changes in this PR, any client (including validators) can choose to either send to the TPU or the TPU forwards port. If they send to the TPU port, well behaved validators will try to process and forward if needed. If a packet is sent to the TPU forwards port, well-behaved validators will try to process and will not forward.

The thing is it is ridiculously easy for a bad actor to get around any port-based decision. So at the end of the day the logic will only be enforced on the good actors, counterproductively, increasing the effectiveness of the spams/attacks done by the bad actors.

There is not much incentive for a validator to change this behavior so we can assume that everyone will exclusively use the TPU port for sending transactions and then well-behaved validators protect each other by only sending messages one hop. So this change actually helps a lot because it guarantees that bad actors can't amplify their access to blocks by spamming TPU ports on upcoming leaders and having those leaders continue forwarding any unprocessed packets.

Besides the problems with port-based decisions, it is not very evident that forwarded transactions should be treated this way.

I agree that it's not very clear here what the right thing to do is. I think this will need to be resolved in a few different changes. We already have the data budget in place which might be enough so that we don't need this change. But given that clients already retry their transactions using the send-tx-service, I don't think validators would choose to have this extra forwarding overhead if given the choice.

[t-nelson]

Forwarding my reply on this topic from Discord

I see this as fixing improper handling of good actor behavior. We're going to have to sign forwards to do the qos stuff safely anyway. This bandaid seems like the right first step

Further, this change brings behavior in line with how a majority of the eng team thought TPU forwards worked today.

[jstarry]

@sakridge how do you feel about this change? Any particular testing you would like done?

[Alibalala]

好的，已收到

[sakridge]

@sakridge how do you feel about this change? Any particular testing you would like done?

In general I like it, I would rather push more forwarding into the client. It would be nice to have some kind of idea of how it affects the system though. Maybe we can experiment with it on testnet first? Send a bunch of transactions and then can we instrument and see what kind of traffic we are accepting through this path today?