PohRecorder: SharedWorkingBank by apfitzge · Pull Request #7280 · anza-xyz/agave

apfitzge · 2025-08-01T19:55:54Z

Problem

LeaderBankNotifier has an inner lock that is seeing contention
We don't need to lock poh recorder to get the working bank

Summary of Changes

Add SharedWorkingBank to PohRecorder
- arc-swappable bank controlled by the recorder
- can be shared on other threads, in the wrapper that only gives read-access
Delete LeaderBankNotifier

Fixes #

This reverts commit ea2f9db.

apfitzge · 2025-08-01T19:57:08Z

-            .upgrade()
+    /// Get the current poh working bank with a timeout.
+    fn get_working_bank_with_timeout(&self) -> Option<Arc<Bank>> {
+        const TIMEOUT: Duration = Duration::from_millis(50);


kept the timeout the same in this PR. I think we may want to remove this in the near future.

for my own understanding since this is the first thing that jumped out: when/why
did we need such a long timeout?

originally this was here because txs can (still can unless #7170 lands) be "leaked" over to the next bank.
Bank::new_from_parent can take unexpectedly long times during some cases.

I think if #7170 lands, we can just remove any timeout - either we have the bank or we don't.

apfitzge · 2025-08-01T19:58:14Z

@alessandrod once merged we can use this in DecisionMaker to remove the poh lock optimistically when we are leader.

We'll need some additional changes to remove the lock to get time until leader when we're not leader. I think that's also quite straight-forward though!

apfitzge · 2025-08-01T19:59:39Z

    tick_cache: Vec<(Entry, u64)>, // cache of entry and its tick_height
    working_bank: Option<WorkingBank>,
    working_bank_sender: Sender<WorkingBankEntry>,
+    shared_working_bank: SharedWorkingBank,


I split this from working_bank because of unified-scheduler type messyness. Would have had like 3 or 4 levels of Arc indirection and would have required splitting the bank from the other working bank meta anyway.

This was the easier path.

this seems fine, but maybe add a comment explaining that working_bank and
shared_working_bank are the same thing? Just in the (unlikely) case someone goes
and reworks working bank and doesn't notice the shared one or something

codecov-commenter · 2025-08-01T21:11:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.8%. Comparing base (87769e8) to head (84a3616).
⚠️ Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7280     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         803      802      -1     
  Lines      364439   364255    -184     
=========================================
- Hits       302080   301870    -210     
- Misses      62359    62385     +26

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alessandrod

lgtm, just a couple nits

alessandrod · 2025-08-05T12:21:48Z

-            .upgrade()
+    /// Get the current poh working bank with a timeout.
+    fn get_working_bank_with_timeout(&self) -> Option<Arc<Bank>> {
+        const TIMEOUT: Duration = Duration::from_millis(50);


for my own understanding since this is the first thing that jumped out: when/why
did we need such a long timeout?

alessandrod · 2025-08-05T12:23:33Z

                tick_cache: vec![],
                working_bank: None,
                working_bank_sender,
+                shared_working_bank: SharedWorkingBank::default(),


nit: I'd rather have this be SharedWorkingBank::empty() or something

IMO default is an anti pattern when the default isn't obvious, and in this case
it isn't clear what a default SharedWorkingBank is

alessandrod · 2025-08-05T12:35:21Z

-            .get_or_wait_for_in_progress(Duration::from_millis(50))
-            .upgrade()
+    /// Get the current poh working bank with a timeout.
+    fn get_working_bank_with_timeout(&self) -> Option<Arc<Bank>> {


nit: we must kill the get_ prefixes at some point (thing(), set_thing() is
idiomatic rust)

alessandrod · 2025-08-05T12:41:17Z

    tick_cache: Vec<(Entry, u64)>, // cache of entry and its tick_height
    working_bank: Option<WorkingBank>,
    working_bank_sender: Sender<WorkingBankEntry>,
+    shared_working_bank: SharedWorkingBank,


this seems fine, but maybe add a comment explaining that working_bank and
shared_working_bank are the same thing? Just in the (unlikely) case someone goes
and reworks working bank and doesn't notice the shared one or something

…emain consistent

apfitzge added 7 commits July 31, 2025 12:29

add poh arc-swap dependency

8225e3e

PohRecorder::shared_working_bank

31e9864

add core arc-swap dependency

ea2f9db

ConsumeWorker - replace LeaderBankNotifier with shared_working_bank

e981759

remove LeaderBankNotifier

105954e

SharedWorkingBank wrapper

89aa61a

Revert "add core arc-swap dependency"

e1c37a8

This reverts commit ea2f9db.

apfitzge changed the title ~~add poh arc-swap dependency~~ PohRecorder: SharedWorkingBank Aug 1, 2025

apfitzge commented Aug 1, 2025

View reviewed changes

apfitzge marked this pull request as ready for review August 2, 2025 00:58

apfitzge requested review from alessandrod, bw-solana and jstarry August 4, 2025 14:21

apfitzge mentioned this pull request Aug 4, 2025

BankForks - SharableBank for root #7300

Merged

alessandrod previously approved these changes Aug 5, 2025

View reviewed changes

apfitzge added 3 commits August 5, 2025 09:51

SharedWorkingBank - remove Default, add empty()

7c4ffe7

add documentation about working_bank/shared_working_bank needing to r…

dc14734

…emain consistent

remove get_ prefix for working_bank accessors in ConsumeWorker

84a3616

apfitzge dismissed alessandrod’s stale review via 84a3616 August 5, 2025 14:58

apfitzge requested a review from alessandrod August 5, 2025 15:00

alessandrod approved these changes Aug 5, 2025

View reviewed changes

apfitzge merged commit dc0f51a into anza-xyz:master Aug 5, 2025
52 checks passed

apfitzge mentioned this pull request Oct 1, 2025

ConsumeWorker: improve bank check/switch efficiency #8291

Merged

mergify Bot mentioned this pull request Oct 2, 2025

v3.0: ConsumeWorker: improve bank check/switch efficiency (backport of #8291) #8315

Closed

Conversation

apfitzge commented Aug 1, 2025

Problem

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apfitzge commented Aug 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alessandrod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apfitzge Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Aug 1, 2025 •

edited

Loading

apfitzge Aug 5, 2025 •

edited

Loading