InvalidFeePayerFilter #32939

apfitzge · 2023-08-22T19:20:19Z

Problem

Some transactions that cannot pay fees make it into banking stage, often with repeat offenders. These transactions take time to process which can cause fewer valid transactions to make it into the block.

Summary of Changes

Add an InvalidFeePayerFilter
- Wrapper of DashMap for now, but potentially switch to a bloom filter in future (?)
Upon seeing a transaction without sufficient funds for fees, add the fee-payer to the InvalidFeePayerFilter
Multi-Iterator checks InvalidFeePayerFilter during batch selection
- this let's more quickly rule these out
Checks InvalidFeePayerFilter before forwarding
- no need to forward these transactions to the next leader
BankingStage checks InvalidFeePayerFilter when receiving packets from SigVerify
- this means these transactions cannot take up the limited buffer space
BankingStage will periodically clear the filter

Fixes #

codecov · 2023-08-23T21:42:28Z

Codecov Report

Merging #32939 (afe8dd8) into master (5f58d2d) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 93.6%.

@@           Coverage Diff            @@
##           master   #32939    +/-   ##
========================================
  Coverage    82.0%    82.0%            
========================================
  Files         796      797     +1     
  Lines      215727   215926   +199     
========================================
+ Hits       176897   177100   +203     
+ Misses      38830    38826     -4

apfitzge · 2023-08-24T17:53:29Z

I wrote a benchmark to test this code against master. The bench is a bit hacky, but I filled a single-thread of banking-stage buffer half with prioritized spam (unable to pay fees) and half with valid transactions. Benched how long it took to get through all transactions.

After observing the first spam transaction, the rest should be more quickly filtered out by the MI batch selection:

master:
test bench_banking_invalid_fee_payers               ... bench:   2,998,819 ns/iter (+/- 64,277)
test bench_banking_invalid_fee_payers               ... bench:   3,036,917 ns/iter (+/- 52,711)
test bench_banking_invalid_fee_payers               ... bench:   2,987,155 ns/iter (+/- 58,034)

invalid_fee_payer_filter:
test bench_banking_invalid_fee_payers               ... bench:   2,069,501 ns/iter (+/- 75,636)
test bench_banking_invalid_fee_payers               ... bench:   2,034,970 ns/iter (+/- 32,393)
test bench_banking_invalid_fee_payers               ... bench:   2,026,578 ns/iter (+/- 25,654)

bench is a bit flaky, with a sigsev if I try to join my poh_service...so not committing the code for bench quite yet.

apfitzge · 2023-08-24T17:55:37Z

core/src/banking_stage/consumer.rs

+                    TransactionError::AccountNotFound
+                    | TransactionError::InsufficientFundsForFee
+                    | TransactionError::InvalidAccountForFee => {
+                        self.invalid_fee_payer_filter.add(*tx.message().fee_payer());


Here is the only place we add to our filter currently.

These 3 errors:

AccountNotFound - could not find fee-payer during load

InsufficientFundsForFee - fee-payer exists, but doesn't have enough lamports

InvalidAccountForFee - this account can't pay fees

~~won't AccountNotFound get thrown for accounts besides the fee-payer?~~

:my_eyes_are_bleeding:

solana/accounts-db/src/accounts.rs

Lines 493 to 497 in dbe4017

if !validated_fee_payer {

error_counters.account_not_found += 1;

return Err(TransactionError::AccountNotFound);

}

I think that was the only instance😭
Maybe it's better to swap that error with InsufficientFunds or rename it to FeePayerAccoutNotFound so we don't lose the distinction

I think that was the only instance😭 Maybe it's better to swap that error with InsufficientFunds or rename it to FeePayerAccoutNotFound so we don't lose the distinction

could. separate pr tho

apfitzge · 2023-08-24T17:56:22Z

core/src/banking_stage/latest_unprocessed_votes.rs

@@ -285,6 +287,12 @@ impl LatestUnprocessedVotes {
                                bank.as_ref(),
                            )
                        {
+                            if invalid_fee_payer_filter


Check vote transactions for invalid fee-payers prior to forwarding.

apfitzge · 2023-08-24T17:57:14Z

core/src/banking_stage/packet_receiver.rs

+        let original_length = deserialized_packets.len();
+        let filtered_packets = deserialized_packets
+            .into_iter()
+            .filter(|packet| {


remove any known invalid-fee-payer when receiving from sigverify into banking stage.

This prevents them from taking up buffer space

apfitzge · 2023-08-24T17:57:46Z

core/src/banking_stage/unprocessed_transaction_storage.rs

    let message = &packet.transaction().get_message().message;
+    if invalid_fee_payer_filter.should_reject(&message.static_account_keys()[0]) {


Check for known invalid fee-payers during MI batch selection. Don't even attempt to load and execute these transactions.

apfitzge · 2023-08-24T17:59:47Z

Plan is to eventually also check txs prior to signature verification in SigVerifyStage so we don't even waste resources verifying them. That's a bit more involved, and think it's a good for a separate PR.

apfitzge · 2023-08-24T19:03:41Z

core/src/invalid_fee_payer_filter.rs

+}
+
+/// Simple wrapper thread that periodically resets the filter.
+pub struct InvalidFeePayerFilterThread {


hmm re-thinking after I posted the PR. I probably don't really need a separate thread for this at all.

Just add an atomic interval on the InvalidFeePayerFilter

Banking threads just call some fn that resets & reports if atomic interval should report

tao-stones

Looks good to me, filtering txs couldn't/won't pay at banking_stage's receiving, processing and forwarding points are thoughtful process to improve banking qos. Even it'd be defenseless if spammer continuously rotating invalid payer accounts, this change, imo, are still good.

Will take another round of review later

t-nelson · 2023-08-25T19:19:24Z

core/src/invalid_fee_payer_filter.rs

+
+#[derive(Default)]
+pub struct InvalidFeePayerFilter {
+    recent_invalid_fee_payers: DashSet<Pubkey>,


there's resource exhaustion attack potential with retaining the pubkeys here. consider Deduper as a high-performance, fixed-size, statistical accounting mechanism

Had that in mind as a follow up, hence why I separated a class.
I can just do in this PR tho

Looked into this a bit and could be wrong. It looks to me reseting or clearing the deduper/bloom filter would require mutability; so we'd need some sort of locking mechanism. Since this filter is shared by multiple threads, that would block other threads which is something I'm trying to avoid.

Might be better to just vacate LRU if we would exceed some capacity. That would protect us from OOMing, but potentially slow things down if we exceed capacity. vs periodic locking of something like deduper. wdyt?

isn't deduper all atomics?

Not for resetting:

pub fn maybe_reset<R: Rng>( &mut self, ...

because the underlying bits is stored in a Vec: bits: Vec<AtomicU64>,

@t-nelson are you content with this decision to just randomly evict if we see too many invalid fee payers?

performance in the bench i was running (1 very active invalid fee-payer) showed no difference between bloom and dash map internally.

seems like bloom should be preferable in that case due to constant size?

@t-nelson are you content with this decision to just randomly evict if we see too many invalid fee payers?

seems fine, yeah

seems like bloom should be preferable in that case due to constant size?

thoughts?

a pre-allocated dashmap also has a constant size, although I think larger than the double-bloom filter for a similar number of items.

Since there doesn't seem to be any time performance benefit, I was sticking with the simpler implementation, instead of maintaining 2 bloom-filters to allow us to "atomically" clear.

@t-nelson not sure if you saw the reply to your "thoughts?"

t-nelson · 2023-08-25T19:21:57Z

core/src/banking_stage/unprocessed_transaction_storage.rs

+            .iter()
+            .map(|tx| {
+                if invalid_fee_payer_filter.should_reject(&tx.message().account_keys()[0]) {
+                    Err(TransactionError::InsufficientFundsForFee)


InvalidAccountForFee would have less potential to mislead as an aggregate error here

I was about to suggest to use a new TransactionError specifically to indicate it's failed because the payer account is black listed.

Mainly thinking from UX perspective; sophisticated abusers will soon find way to get around this filter, the 2 sec blackout would probably apply more to innocent mistakes, a clear error might help in that case?

Unrecorded transaction errors do not get reported back to the users though. It'd purely be an internal metric - similar to our AccountInUse error metrics.

@t-nelson the actual errors here don't get used at all. We only check is_ok() below.
But I changed agree InvalidAccountForFee is a better dummy error here since we've effectively marked the account as invalid with the filter: d52a238

t-nelson · 2023-08-25T20:03:08Z

core/src/banking_stage/consumer.rs

+                    TransactionError::AccountNotFound
+                    | TransactionError::InsufficientFundsForFee
+                    | TransactionError::InvalidAccountForFee => {
+                        self.invalid_fee_payer_filter.add(*tx.message().fee_payer());


~~won't AccountNotFound get thrown for accounts besides the fee-payer?~~

:my_eyes_are_bleeding:

solana/accounts-db/src/accounts.rs

Lines 493 to 497 in dbe4017

if !validated_fee_payer {

error_counters.account_not_found += 1;

return Err(TransactionError::AccountNotFound);

}

core/src/invalid_fee_payer_filter.rs

t-nelson · 2023-09-07T02:56:31Z

core/src/invalid_fee_payer_filter.rs

+
+#[derive(Default)]
+pub struct InvalidFeePayerFilter {
+    recent_invalid_fee_payers: DashSet<Pubkey>,


performance in the bench i was running (1 very active invalid fee-payer) showed no difference between bloom and dash map internally.

seems like bloom should be preferable in that case due to constant size?

@t-nelson are you content with this decision to just randomly evict if we see too many invalid fee payers?

seems fine, yeah

core/src/invalid_fee_payer_filter.rs

t-nelson · 2023-09-07T02:59:55Z

core/src/banking_stage/unprocessed_transaction_storage.rs

+            .iter()
+            .map(|tx| {
+                if invalid_fee_payer_filter.should_reject(&tx.message().account_keys()[0]) {
+                    // Actual error variant is not used - Result is only used to skip additional


core eng will use the error variant for debugging. conflating this case with others may mislead those efforts

That's kind of why I added the comment here. It makes it clear that these will not show up in the regular error metrics.
The only way someone finds this is if they search for the error variant, in which case they then see this comment.

I changed it to the more appropriate variant InvalidAccountForFee - unless we want to create a new error variant not sure what action to take here.

core/src/banking_stage/unprocessed_transaction_storage.rs

core/src/banking_stage/packet_receiver.rs

t-nelson · 2023-09-07T03:09:45Z

core/src/banking_stage/consumer.rs

+                    TransactionError::AccountNotFound
+                    | TransactionError::InsufficientFundsForFee
+                    | TransactionError::InvalidAccountForFee => {
+                        self.invalid_fee_payer_filter.add(*tx.message().fee_payer());


I think that was the only instance😭 Maybe it's better to swap that error with InsufficientFunds or rename it to FeePayerAccoutNotFound so we don't lose the distinction

could. separate pr tho

core/src/banking_stage/unprocessed_transaction_storage.rs

t-nelson · 2023-09-11T19:56:25Z

core/src/banking_stage/packet_receiver.rs

@@ -50,7 +56,7 @@ impl PacketReceiver {
                    recv_timeout,
                    unprocessed_transaction_storage.max_receive_size(),
                )
-                // Consumes results if Ok, otherwise we keep the Err
+                // Consumes results if Ok, otherwise we keep the Err)


t-nelson · 2023-09-11T20:08:33Z

core/src/invalid_fee_payer_filter.rs

+
+#[derive(Default)]
+pub struct InvalidFeePayerFilter {
+    recent_invalid_fee_payers: DashSet<Pubkey>,


seems like bloom should be preferable in that case due to constant size?

thoughts?

apfitzge force-pushed the invalid_fee_payer_filter branch from 4dfebf1 to d106c68 Compare August 23, 2023 20:42

apfitzge commented Aug 24, 2023

View reviewed changes

apfitzge marked this pull request as ready for review August 24, 2023 17:58

apfitzge requested review from tao-stones and buffalu August 24, 2023 17:58

apfitzge commented Aug 24, 2023

View reviewed changes

tao-stones reviewed Aug 24, 2023

View reviewed changes

t-nelson reviewed Aug 25, 2023

View reviewed changes

apfitzge force-pushed the invalid_fee_payer_filter branch 3 times, most recently from c3a814a to 99ef4fd Compare September 5, 2023 20:46

t-nelson reviewed Sep 7, 2023

View reviewed changes

apfitzge requested a review from t-nelson September 7, 2023 22:10

t-nelson reviewed Sep 11, 2023

View reviewed changes

apfitzge added 10 commits September 20, 2023 16:37

InvalidFeePayerFilter

f0674ea

BankingStage consume adding and filtering invalid fee payer

c9430ae

forward filter

6a4d0b9

Filter received from sigverify

e1a3a5c

periodic reset & report of invalid fee payer filter

54d192f

basic test

9d2b349

use fee_payer()

6ec96fa

any non-executable error is a bannable offense

d29336f

explicit error selection for ban hammer

d204993

less restrictive ban-hammer

8bcbf38

apfitzge added 11 commits September 20, 2023 16:46

Remove invalid fee payer reporting/reseting thread

77070b8

randomly evicted upon exceeding capacity

29429de

fmt

ccf5036

fix test

6deec18

Change error variant to InvalidAccountForFee

0a1a95c

fix stupid typo

b695c5f

move interval to filter, not stats

7221380

const and comments on capacity

114af45

argument re-arrangement

46c0cbc

rename filter_invalid_fee_payers

1b8e6c2

remove trailing )

afe8dd8

apfitzge force-pushed the invalid_fee_payer_filter branch from 2fef39b to afe8dd8 Compare September 20, 2023 16:46

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

github-actions bot closed this Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InvalidFeePayerFilter #32939

InvalidFeePayerFilter #32939

apfitzge commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

apfitzge commented Aug 24, 2023

apfitzge Aug 24, 2023

t-nelson Aug 25, 2023

apfitzge Aug 25, 2023

t-nelson Sep 7, 2023

apfitzge Aug 24, 2023

apfitzge Aug 24, 2023

apfitzge Aug 24, 2023

apfitzge commented Aug 24, 2023

apfitzge Aug 24, 2023

tao-stones left a comment

t-nelson Aug 25, 2023

apfitzge Aug 25, 2023

apfitzge Aug 28, 2023

t-nelson Aug 28, 2023

apfitzge Aug 28, 2023

apfitzge Sep 6, 2023

t-nelson Sep 7, 2023

t-nelson Sep 11, 2023

apfitzge Sep 11, 2023

apfitzge Sep 20, 2023

t-nelson Aug 25, 2023

tao-stones Aug 25, 2023

apfitzge Aug 26, 2023 •

edited

Loading

apfitzge Sep 6, 2023

t-nelson Aug 25, 2023

t-nelson Sep 7, 2023

t-nelson Sep 7, 2023

apfitzge Sep 7, 2023

t-nelson Sep 7, 2023

t-nelson Sep 11, 2023

t-nelson Sep 11, 2023

	if !validated_fee_payer {
	error_counters.account_not_found += 1;
	return Err(TransactionError::AccountNotFound);
	}

		let message = &packet.transaction().get_message().message;
		if invalid_fee_payer_filter.should_reject(&message.static_account_keys()[0]) {

InvalidFeePayerFilter #32939

InvalidFeePayerFilter #32939

Conversation

apfitzge commented Aug 22, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

apfitzge commented Aug 24, 2023

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 Aug 24, 2023

Choose a reason for hiding this comment

tao-stones 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

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 Aug 26, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

apfitzge Aug 26, 2023 •

edited

Loading