purge duplicated bank prioritization fee from cache #33062

tao-stones · 2023-08-30T00:23:05Z

Problem

Duplicated banks can add inconsistent data to prioritization_fee_cache.

Summary of Changes

purge duplicated bank's priority_fee from cache during finalization
add test for the described scenario
add metrics counts to monitor such occurrences

Fixes #

tao-stones · 2023-08-30T00:24:38Z

rpc/src/optimistically_confirmed_bank_tracker.rs

@@ -296,6 +297,9 @@ impl OptimisticallyConfirmedBankTracker {
                        *highest_confirmed_slot = slot;
                    }
                    drop(w_optimistically_confirmed_bank);
+
+                    // finalize block's minimum prioritization fee cache for this bank
+                    prioritization_fee_cache.finalize_priority_fee(slot, bank_id);


finalize prioritization_fee_cache with OC-ed bank_id, purge fee from other banks from cache.

hmm, actually, i think .finalize_priority_fee() should be called just after Self::notify_slot_status(..., SlotNotification::OptimisticallyConfirmed)-ing inside OptimisticallyConfirmedBankTracker::notify_or_defer().

otherwise, we can't ensure bank is frozen (prioritization cache can potentially have updates until frozen) and all parent banks' prioritization fee caches are correctly finalized without skipping?

note that BankNotification::OptimisticallyConfirmed(slot) can have gaps and could be send before locally replaying..

so, #32692 had already bugs, it seems...

Mmm, not familiar enough with this part of code. Let me understand how it flows first.

yea, the gap between OptimisticallyConfirmed and bank frozen is real. Thanks for pointing to the correct solution. I opened a separate PR #33100 to address this bug.

tao-stones · 2023-08-30T00:25:25Z

rpc/src/optimistically_confirmed_bank_tracker.rs

@@ -356,6 +358,9 @@ impl OptimisticallyConfirmedBankTracker {
                        w_optimistically_confirmed_bank.bank = bank;
                    }
                    drop(w_optimistically_confirmed_bank);
+
+                    // finalize block's minimum prioritization fee cache for this bank
+                    prioritization_fee_cache.finalize_priority_fee(frozen_slot, frozen_bank_id);


do same when pending OC bank is frozen

tao-stones · 2023-08-30T00:29:53Z

runtime/src/prioritization_fee_cache.rs

@@ -121,20 +109,26 @@ impl PrioritizationFeeCacheMetrics {
 enum CacheServiceUpdate {
    TransactionUpdate {
        slot: Slot,
+        bank_id: BankId,


add bank_id to CacheServiceUpdate enums

ryoqun · 2023-08-30T01:23:54Z

runtime/src/prioritization_fee_cache.rs

        transaction_fee: u64,
        writable_accounts: Arc<Vec<Pubkey>>,
    },
    BankFrozen {
        slot: Slot,
+        bank_id: BankId,
    },
    Exit,


hmm, how about adding new BankDumped { slot: Slot }, instead of introducing this BankId-based 2nd level caching? And the variant can be sent inside purge_unfirmed_duplicate_slot() after plumbing &PriotizationFeeCache there.

that alternative impl is simpler and should be safe as long as solPrFeeCacSvc is single threaded, which should be because of already existing linearizable requirement of BankFrozen.

Agreed. But then I thought that if I had missed purge_unconfirmed_duplicated_slot() last time, what would make sure I won't miss something else, or new duplicated slot/bank logic being added without being aware of prioritization_fee_cache? That motivates to explore the approach of purge cache by OC event, which should be definitive and un-rollbackable. But I do agree it adds some complicity, and using bank_id as secondary key may not be ideal. Hence this is a draft. 😃 Keen to hear your thoughts on it.

yeah, i see your reasoning for bank_id approach clearly.

i spent some time on which is better. and i couldn't conclude it. so, it boils down to the author's preference.

as said bank_id takes more human effort than BankDumped approach. yet, i saw you added tests. seems you're well energized for the effort. :)

please continue. once this pr is out of draft, i'll review.

Totally agree that this approach is bit more involved, but not too bad :) I reasoned to myself that it is worth the extra effort to make p.fee cache bit more independent.

I added test thinking either approach we will need a test to capture the corner case scenario.

I'd like to add a metric counter to capture the occurrence of dup bank, then will move it out of draft.

Thanks for being patient ❤️

ryoqun · 2023-09-05T13:48:22Z

runtime/src/prioritization_fee_cache.rs

@@ -121,20 +125,26 @@ impl PrioritizationFeeCacheMetrics {
 enum CacheServiceUpdate {
    TransactionUpdate {
        slot: Slot,
+        bank_id: BankId,
        transaction_fee: u64,
        writable_accounts: Arc<Vec<Pubkey>>,
    },
    BankFrozen {


come to think of it, this variant isn't about frozen. maybe, BankFinalized?

Oh, right! fixing it.

ryoqun

generally looks good. i think #33100 needs to land first and rebase this pr then.

tao-stones · 2023-09-05T15:19:52Z

generally looks good. i think #33100 needs to land first and rebase this pr then.

Yea, this sounds a plan

tao-stones · 2023-09-06T15:03:13Z

generally looks good. i think #33100 needs to land first and rebase this pr then.

Yea, this sounds a plan

#33100 is merged and rebased.

codecov · 2023-09-06T16:25:20Z

Codecov Report

Merging #33062 (878a996) into master (90d9839) will increase coverage by 0.1%.
Report is 812 commits behind head on master.
The diff coverage is 90.5%.

@@            Coverage Diff            @@
##           master   #33062     +/-   ##
=========================================
+ Coverage    81.9%    82.1%   +0.1%     
=========================================
  Files         762      785     +23     
  Lines      207801   211280   +3479     
=========================================
+ Hits       170353   173509   +3156     
- Misses      37448    37771    +323

apfitzge

Don't see any issues, but I think the bug was found by ryoqun, so please wait for his feedback before merging

ryoqun · 2023-09-07T14:38:20Z

runtime/src/prioritization_fee_cache.rs

+            .filter_map(|(slot, slot_prioritization_fee)| {
+                slot_prioritization_fee
+                    .iter()
+                    .find_map(|prioritization_fee_read| {


nit: how about renaming prioritization_fee_read => prioritization_fee as this isn't related to Mutex anymore.

ryoqun · 2023-09-07T14:45:08Z

runtime/src/prioritization_fee_cache.rs

            measure!(Self::get_prioritization_fee(cache, slot), "cache_lock_time");

-        let (mut block_prioritization_fee, entry_lock_time) =
-            measure!(block_prioritization_fee.lock().unwrap(), "entry_lock_time");
-
        // prune cache by evicting write account entry from prioritization fee if its fee is less
        // or equal to block's minimum transaction fee, because they are irrelevant in calculating
        // block minimum fee.
        let (_, slot_finalize_time) = measure!(


nit: seems the returned Result is silently discarded after assigning it to _. how about error!()-ing, at least?

side-note: i have hidden desire for --deny clippy::let_underscore_must_use tree-wide... ;)

👍🏼 added error!(...) should Result is_err()

runtime/src/prioritization_fee_cache.rs

ryoqun · 2023-09-07T15:02:35Z

runtime/src/prioritization_fee_cache.rs

-            .unwrap()
-            .get_min_transaction_fee()
+            .get(&bank.bank_id())
+            .map(|block_fee| block_fee.get_min_transaction_fee())


calling block_fee.get_min_transaction_fee() isn't meaningless... i think you meant to call .and_then()? or remove .map(...) altogether?

i mean, i think .map(...)'s return type is Option<Option<u64>>. so, we aren't checking .is_none() on the returned value of .get_min_transaction_fee().

Good catch! Yea, should use and_then; then realized just checking if block_fee.get_min_transaction_fee() has value would make test flaky. In case a bank updates cache with multiple transactions, since each transaction applies to cache one at time, the sync_update() should make sure all transactions have applied (instead of checking if any transaction has applied). I moved metrics.accumulate_successful_transaction_update_count(1); and conveniently used it to sync updates. tbh, not too happy double-purposing metrics counter here.

oh, glad that we fixed test flakiness. :)

tbh, not too happy double-purposing metrics counter here.

same here. but i think this is acceptable, considering this is just a tiny piece of test harnesses...

ryoqun · 2023-09-07T15:10:53Z

runtime/src/prioritization_fee_cache.rs

-        while !fee.lock().unwrap().is_finalized() {
+        while !fee
+            .get(&bank_id)
+            .map_or_else(|| false, |block_fee| block_fee.is_finalized())


nit: .map_or() should be preferred here, because it's fine to eager-evaluate false.

ryoqun

lgtm with nits

ryoqun

lgtm. thanks for the extra polishes. :)

tao-stones · 2023-09-11T15:09:58Z

will added bp label when #33100 bps are merged.

* purge duplicated bank prioritization fee from cache * add test for purge dup bank * Added metrics counts to monitor anomalies * fix a flaky test (cherry picked from commit 4f4ce69) # Conflicts: # runtime/src/prioritization_fee.rs # runtime/src/prioritization_fee_cache.rs

* purge duplicated bank prioritization fee from cache * add test for purge dup bank * Added metrics counts to monitor anomalies * fix a flaky test (cherry picked from commit 4f4ce69) # Conflicts: # runtime/src/prioritization_fee_cache.rs

#33062) (#33253) * purge duplicated bank prioritization fee from cache (#33062) * purge duplicated bank prioritization fee from cache * add test for purge dup bank * Added metrics counts to monitor anomalies * fix a flaky test (cherry picked from commit 4f4ce69) # Conflicts: # runtime/src/prioritization_fee.rs # runtime/src/prioritization_fee_cache.rs * manual merge --------- Co-authored-by: Tao Zhu <[email protected]> Co-authored-by: Tao Zhu <[email protected]>

#33062) (#33254) * purge duplicated bank prioritization fee from cache (#33062) * purge duplicated bank prioritization fee from cache * add test for purge dup bank * Added metrics counts to monitor anomalies * fix a flaky test (cherry picked from commit 4f4ce69) # Conflicts: # runtime/src/prioritization_fee_cache.rs * maunal merge --------- Co-authored-by: Tao Zhu <[email protected]> Co-authored-by: Tao Zhu <[email protected]>

solana-labs#33062) (solana-labs#33253) * purge duplicated bank prioritization fee from cache (solana-labs#33062) * purge duplicated bank prioritization fee from cache * add test for purge dup bank * Added metrics counts to monitor anomalies * fix a flaky test (cherry picked from commit 4f4ce69) # Conflicts: # runtime/src/prioritization_fee.rs # runtime/src/prioritization_fee_cache.rs * manual merge --------- Co-authored-by: Tao Zhu <[email protected]> Co-authored-by: Tao Zhu <[email protected]>

tao-stones commented Aug 30, 2023

View reviewed changes

ryoqun reviewed Aug 30, 2023

View reviewed changes

tao-stones force-pushed the prioritization_fee_cache_purge_duplicated_bank branch 5 times, most recently from 07b357c to 2d097ef Compare August 31, 2023 20:16

tao-stones mentioned this pull request Sep 1, 2023

finalize prioritization_fee_cache after oc-ed bank is frozen #33100

Merged

tao-stones force-pushed the prioritization_fee_cache_purge_duplicated_bank branch from 2d097ef to b135ef7 Compare September 1, 2023 16:00

tao-stones requested review from ryoqun and apfitzge September 1, 2023 16:04

tao-stones marked this pull request as ready for review September 1, 2023 16:05

tao-stones force-pushed the prioritization_fee_cache_purge_duplicated_bank branch from b135ef7 to df8b794 Compare September 1, 2023 16:41

ryoqun reviewed Sep 5, 2023

View reviewed changes

tao-stones added 4 commits September 6, 2023 15:01

purge duplicated bank prioritization fee from cache

9b75f6b

add test for purge dup bank

a23bc02

Added metrics counts to monitor anomalies

e99bc70

rename enum

456dcbb

tao-stones force-pushed the prioritization_fee_cache_purge_duplicated_bank branch from d5a3069 to 456dcbb Compare September 6, 2023 15:02

fix merge

81b1f5f

apfitzge reviewed Sep 6, 2023

View reviewed changes

tao-stones requested a review from ryoqun September 6, 2023 21:15

ryoqun reviewed Sep 7, 2023

View reviewed changes

runtime/src/prioritization_fee_cache.rs Show resolved Hide resolved

ryoqun reviewed Sep 7, 2023

View reviewed changes

ryoqun previously approved these changes Sep 7, 2023

View reviewed changes

review update; fix a flaky test

9c0c12d

tao-stones dismissed ryoqun’s stale review via 9c0c12d September 8, 2023 03:48

tao-stones added v1.14 v1.16 PRs that should be backported to v1.16 and removed v1.14 v1.16 PRs that should be backported to v1.16 labels Sep 8, 2023

fix the test

878a996

tao-stones requested a review from ryoqun September 8, 2023 19:26

ryoqun approved these changes Sep 11, 2023

View reviewed changes

tao-stones merged commit 4f4ce69 into solana-labs:master Sep 11, 2023

tao-stones added v1.14 v1.16 PRs that should be backported to v1.16 labels Sep 14, 2023

mergify bot mentioned this pull request Sep 14, 2023

v1.14: purge duplicated bank prioritization fee from cache (backport of #33062) #33253

Merged

mergify bot mentioned this pull request Sep 14, 2023

v1.16: purge duplicated bank prioritization fee from cache (backport of #33062) #33254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

purge duplicated bank prioritization fee from cache #33062

purge duplicated bank prioritization fee from cache #33062

tao-stones commented Aug 30, 2023 •

edited

Loading

tao-stones Aug 30, 2023

ryoqun Aug 30, 2023

tao-stones Aug 30, 2023

tao-stones Sep 1, 2023

tao-stones Aug 30, 2023

tao-stones Aug 30, 2023 •

edited by ryoqun

Loading

ryoqun Aug 30, 2023

tao-stones Aug 30, 2023

ryoqun Sep 1, 2023

tao-stones Sep 1, 2023

ryoqun Sep 5, 2023

tao-stones Sep 5, 2023

ryoqun left a comment

tao-stones commented Sep 5, 2023

tao-stones commented Sep 6, 2023

codecov bot commented Sep 6, 2023 •

edited

Loading

apfitzge left a comment

ryoqun Sep 7, 2023

ryoqun Sep 7, 2023

tao-stones Sep 8, 2023

ryoqun Sep 7, 2023

ryoqun Sep 7, 2023

tao-stones Sep 8, 2023

ryoqun Sep 11, 2023

ryoqun Sep 7, 2023

ryoqun left a comment

ryoqun left a comment

tao-stones commented Sep 11, 2023

purge duplicated bank prioritization fee from cache #33062

purge duplicated bank prioritization fee from cache #33062

Conversation

tao-stones commented Aug 30, 2023 • edited Loading

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

tao-stones Aug 30, 2023 • edited by ryoqun 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

ryoqun left a comment

Choose a reason for hiding this comment

tao-stones commented Sep 5, 2023

tao-stones commented Sep 6, 2023

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

apfitzge 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

ryoqun left a comment

Choose a reason for hiding this comment

ryoqun left a comment

Choose a reason for hiding this comment

tao-stones commented Sep 11, 2023

tao-stones commented Aug 30, 2023 •

edited

Loading

tao-stones Aug 30, 2023 •

edited by ryoqun

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading