fix incorrect inefficient vote worker

[cavemanloverboy]

Problem

yes.

reached_end_of_slot is set to true after a single vote batch, which reinserts all unprocessed vote packets back into the vote storage.

(git gud @apfitzge)

Summary of Changes

do not mark reached_end_of_slot prematurely...

[apfitzge]

Put an actual description please

[cavemanloverboy]

cmon u gotta live a little we're building the future of finance

[OliverNChalk]

• Two logic bugs were introduced: Remove BankStart #7351
• One was fixed: bugfix: vote worker reached_end_of_slot set by is_complete #8474
• This fixes the other
• This solution supersedes my symptom patch: OliverNChalk@408904a
• Confirmed end of slot log spam is not reached with cavey's patch on master.

Given the fix is a single commit, the PR that lands on master should be clean and meme free. So I think we can just send this?

[sakridge]

Add a test?

[cavemanloverboy]

Add a test?

what's that?

[tao-stones]

I feel it is easier to reason if change to (_, true). Something like this:

        let reached_max_poh_height = match (
            commit_transactions_result,
            bank.is_complete(),
        ) {
            (Err(PohRecorderError::MaxHeightReached), _) | (_, true) => {
                 true
             }
            _ => false,

Also, downstream seems to use function has_reached_end_of_slot() that repeats same logic again, checking bank.is_complete() at ln 419 seems be redundant.

[cavemanloverboy]

how about we merge this fix and get the insane perf win and then worry about refactoring out redundant stuff later

[tao-stones]

Sounds reasonable. What do you think about instead of adding ! in front of bank.is_complete(), let's flip false at ln 424 to true to avoid negative of negative? (first part of original comment)

[OliverNChalk]

Explored testing this, was somewhat feasible if we isolate the process transactions function: OliverNChalk@bd9a71b

@tao-stones any thoughts on testing?
@cavemanloverboy you okay if I push these tests to your branch?

[tao-stones]

Explored testing this, was somewhat feasible if we isolate the process transactions function: OliverNChalk@bd9a71b

@tao-stones any thoughts on testing? @cavemanloverboy you okay if I push these tests to your branch?

I was originally thinking to extract logic of determine reached_max_poh_height into its own function, then test it. But your test looks great.

[cavemanloverboy]

sure push test

[OliverNChalk]

cavemanloverboy/solana#1

[OliverNChalk]

Looks good but can we rebase to squash the merge, else the merge queue will generate a default commit from the PR name which wont be as clear as the commit message you previously wrote

[cavemanloverboy]

alright gogogogo

[cavemanloverboy]

pls don't make me do anything else, im just here to fix bugs not fix your test suite coverage or satisfy your nits tyvm

do any other thing in a cleanup/followup

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.7%. Comparing base (d34495d) to head (5d1fbf4).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9786   +/-   ##
=======================================
  Coverage    82.7%    82.7%           
=======================================
  Files         901      901           
  Lines      324845   324866   +21     
=======================================
+ Hits       268722   268777   +55     
+ Misses      56123    56089   -34     

🚀 New features to boost your workflow:

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