earlier fee payer validation #34666

apfitzge · 2024-01-05T17:32:48Z

Problem

Expired, already processed, and transactions that cannot pay fees can still take locks.

Summary of Changes

Check these conditions before taking locks
No significant affect on max or avg TPS in bench-tps
- The account loading will bring the fee-paying account into cache (if not already there), which will typically make the subsequent load during actual processing faster

I made a patched bench-tps to spam 60% invalid fee-paying txs. Saw degraded perf on master, much better perf with this change:

master: max= 9744, avg=1714
thispr: max=20065, avg=9018

bench-tps patch/hack: invalid-transfer-generation.patch

Blocked by #34652

Fixes #

codecov · 2024-01-05T18:56:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (95f888a) 81.8% compared to head (c6c73d1) 81.8%.
Report is 26 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34666   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         824      824           
  Lines      222717   222742   +25     
=======================================
+ Hits       182287   182380   +93     
+ Misses      40430    40362   -68

apfitzge · 2024-01-10T18:35:15Z

core/src/banking_stage/consumer.rs

@@ -684,6 +709,39 @@ impl Consumer {
        }
    }

+    pub fn check_fee_payer_unlocked(


put this into a separate fn because I actually will be doing same operation in the new scheduler (#34699)

apfitzge · 2024-01-10T18:36:25Z

core/src/banking_stage/unprocessed_transaction_storage.rs

@@ -177,12 +180,31 @@ fn consume_scan_should_process_packet(
            return ProcessingDecision::Never;
        }

+        if !payload.account_locks.check_locks(message) {


did a separate check_locks, then take_locks so that we will not check fee-payers that we know can't even be in this batch.

apfitzge · 2024-01-11T18:38:41Z

core/src/banking_stage/unprocessed_transaction_storage.rs

@@ -177,6 +180,26 @@ fn consume_scan_should_process_packet(
            return ProcessingDecision::Never;
        }

+        // Only check fee-payer if we can actually take locks


renewed hatred for all the edge-cases of MI 😭

t-nelson

r+ @taozhu-chicago's review. ping me if he doesn't get to it before you're unavailable tomorrow and i'll approve

core/src/banking_stage/unprocessed_transaction_storage.rs

tao-stones

the functions make sense to me, my comments are mainly concerns of execution time, which can be monitored with additional metrics. Ok to merge it now, add metrics later.

tao-stones · 2024-01-12T03:04:11Z

core/src/banking_stage/consumer.rs

+        let mut error_counters = TransactionErrorMetrics::default();
+        let pre_results = vec![Ok(()); txs.len()];
+        let check_results =
+            bank.check_transactions(txs, &pre_results, MAX_PROCESSING_AGE, &mut error_counters);


It'd potentially submit more transactions to bank for checking, which read-locks status_cache. Might be useful to add elapse metrics for this call, perhaps in separate PR.

tao-stones · 2024-01-12T03:07:38Z

core/src/banking_stage/consumer.rs

+
+        let check_results = check_results
+            .into_iter()
+            .zip(txs)


is zip(txs) needed?

tao-stones · 2024-01-12T03:12:13Z

core/src/banking_stage/consumer.rs

+            process_compute_budget_instructions(message.program_instructions_iter())?.into();
+        let fee = bank.fee_structure.calculate_fee(
+            message,
+            bank.get_lamports_per_signature(),


I noted early it should fetch lamports_per_signature for durable transactions from nonce account. Then @t-nelson just clarified that durable transaction's fee is calculated based on fee rate at execution. So this should be just fine.

tao-stones · 2024-01-12T03:20:33Z

core/src/banking_stage/consumer.rs

@@ -684,6 +709,39 @@ impl Consumer {
        }
    }

+    pub fn check_fee_payer_unlocked(


A lot of lifting in this function, the saving is remove bad transactions upfront, and warms up good transactions' payer accounts, the extra time spent here is still a concern, especially if a lot of transactions going through this check. Can we add another metrics to monitor it?

tao-stones · 2024-01-12T03:21:53Z

core/src/banking_stage/consumer.rs

-                bank.transfer(5_000, &genesis_config_info.mint_keypair, &keypair.pubkey())
-                    .unwrap();
+                bank.transfer(
+                    100_000,


why change from 5_000 to 100_000?

apfitzge · 2024-01-12T04:24:14Z

@taozhu-chicago pushed minor fix to undo unintended change on the funding, and remove .zip(txs).

Will add metrics in a follow-up PR.

tao-stones

lgtm

apfitzge mentioned this pull request Jan 8, 2024

TransactionScheduler: Add funded check before graph insertion #34699

Merged

apfitzge added 2 commits January 9, 2024 09:05

initial checks before locking accounts

925f92d

accumulate error metrics

6aa6146

apfitzge force-pushed the earlier_fee_payer_validation branch from daf0209 to 6aa6146 Compare January 9, 2024 17:05

apfitzge marked this pull request as ready for review January 9, 2024 17:07

apfitzge requested review from t-nelson and tao-stones and removed request for t-nelson and tao-stones January 9, 2024 23:02

apfitzge marked this pull request as draft January 10, 2024 17:06

MI selection validates fee-payer

fa221e8

apfitzge commented Jan 10, 2024

View reviewed changes

apfitzge marked this pull request as ready for review January 10, 2024 18:39

apfitzge requested review from tao-stones and t-nelson January 10, 2024 18:39

Fix priority guard

382f9af

apfitzge commented Jan 11, 2024

View reviewed changes

t-nelson reviewed Jan 11, 2024

View reviewed changes

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

clarify comment separation

b58468d

tao-stones previously approved these changes Jan 12, 2024

View reviewed changes

apfitzge added 2 commits January 11, 2024 20:19

undo change

465fdca

remove zip(txs)

c6c73d1

apfitzge dismissed tao-stones’s stale review via c6c73d1 January 12, 2024 04:23

apfitzge requested a review from tao-stones January 12, 2024 04:24

tao-stones approved these changes Jan 12, 2024

View reviewed changes

apfitzge merged commit be5337a into solana-labs:master Jan 12, 2024
35 checks passed

apfitzge deleted the earlier_fee_payer_validation branch January 12, 2024 16:22

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

earlier fee payer validation #34666

earlier fee payer validation #34666

apfitzge commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

apfitzge Jan 10, 2024

apfitzge Jan 10, 2024

apfitzge Jan 11, 2024

t-nelson left a comment

tao-stones left a comment

tao-stones Jan 12, 2024

tao-stones Jan 12, 2024

tao-stones Jan 12, 2024

tao-stones Jan 12, 2024

tao-stones Jan 12, 2024

apfitzge commented Jan 12, 2024

tao-stones left a comment

earlier fee payer validation #34666

earlier fee payer validation #34666

Conversation

apfitzge commented Jan 5, 2024 • edited Loading

Problem

Summary of Changes

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

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

apfitzge commented Jan 12, 2024

tao-stones left a comment

Choose a reason for hiding this comment

apfitzge commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading