Skip to content

Remove BankStart#7351

Merged
apfitzge merged 4 commits intoanza-xyz:masterfrom
apfitzge:kill_bank_start
Aug 11, 2025
Merged

Remove BankStart#7351
apfitzge merged 4 commits intoanza-xyz:masterfrom
apfitzge:kill_bank_start

Conversation

@apfitzge
Copy link
Copy Markdown

@apfitzge apfitzge commented Aug 6, 2025

Problem

  • BankStart is currently returned in BufferedPacketDecision
    • this forces us to take a lock on the poh_recorder to make decision
    • we want to get leader-bank w/o any locking

Summary of Changes

  • Remove BankStart
    • this causes the removal several metrics related to leader bank detection time

Fixes #

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.17647% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.0%. Comparing base (5d1651c) to head (33d2684).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #7351    +/-   ##
========================================
  Coverage    83.0%    83.0%            
========================================
  Files         800      800            
  Lines      362381   362262   -119     
========================================
- Hits       300792   300703    -89     
+ Misses      61589    61559    -30     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@apfitzge apfitzge marked this pull request as ready for review August 6, 2025 18:30
@apfitzge apfitzge requested a review from tao-stones August 6, 2025 18:30
@apfitzge
Copy link
Copy Markdown
Author

apfitzge commented Aug 6, 2025

Loss of metrics is main possible "issue" here, but along with #7355 this will let scheduler not lock poh at all.
I have not looked at these metrics in probably a year, I'm not sure if anyone has.

apfitzge added a commit to apfitzge/agave that referenced this pull request Aug 6, 2025
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too worried about lost metrics either 😄 Before going into details, want to check with you if "not using bank_creation_time" is an intended improvement, not side effect.

Comment thread poh/src/poh_recorder.rs
pub type WorkingBankEntry = (Arc<Bank>, (Entry, u64));

#[derive(Debug, Clone)]
pub struct BankStart {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: iirc, BankStart (mainly it's bank_creation_time) was added to limit banking_stage from packing too many transactions due to poh wall-clock drifting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With block CU limits, i don't think overpacking is still an issue, at least not an issue to be mitigated with wall clock. So I think in general this is a positive change. Just want to call it out.

Comment thread core/src/banking_stage/vote_worker.rs
@apfitzge
Copy link
Copy Markdown
Author

apfitzge commented Aug 7, 2025

Before going into details, want to check with you if "not using bank_creation_time" is an intended improvement, not side effect.

It is more of a side-effect. I'd have likely just left it in if there was an easy way to do so.
We want a lock-free way to get the working bank from PoH, but I can't easily get the bank with that time; at least not in a clean way. The obj we'd have to store insidd the ArcSwapOption would be a Arc<(Arc<Bank>, Arc<Instant>)> instead of just an Arc<Bank> which seemed needlessly complicated for keeping the time to me.

Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@apfitzge apfitzge merged commit d792c9d into anza-xyz:master Aug 11, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants