Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup banking bench #24851

Merged
merged 2 commits into from
May 2, 2022
Merged

Conversation

carllin
Copy link
Contributor

@carllin carllin commented Apr 29, 2022

Problem

Banking bench has certain portions that are hard to read

Summary of Changes

Refactor and cleanup

Fixes #

@carllin carllin requested a review from tao-stones April 29, 2022 21:06
@carllin carllin added the v1.10 label Apr 29, 2022
@@ -370,7 +398,7 @@ fn main() {
bank.transaction_count(),
txs_processed
);
assert!(txs_processed < bank.transaction_count());
//assert!(txs_processed < bank.transaction_count());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didn't seem safe in the following scenario:

  1. We create a new bank and add it to poh recorder:
    bank = bank_forks.working_bank();
    insert_time.stop();
    // set cost tracker limits to MAX so it will not filter out TXs
    bank.write_cost_tracker().unwrap().set_limits(
    std::u64::MAX,
    std::u64::MAX,
    std::u64::MAX,
    );
    poh_recorder.lock().unwrap().set_bank(&bank);
  2. Before we can send the transactions for the next iteration, this bank is removed from poh recorder
  3. We send the transactions for the next iteration, none of them land
  4. check_txs returns because there's no bank:
    if poh_recorder.lock().unwrap().bank().is_none() {
  5. No transactions were landed in this bank so txs_processed == bank.transaction_count()

Copy link
Contributor

Choose a reason for hiding this comment

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

I am almost sure I never see this assert fail for described scenario. I mean why would #2 happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it can happen if all the work after set_bank() takes longer than 350ms, things like the blockhash refresh, signature clearing, etc.

@codecov
Copy link

codecov bot commented Apr 30, 2022

Codecov Report

Merging #24851 (fc4e4b2) into master (3155270) will increase coverage by 11.9%.
The diff coverage is 75.2%.

❗ Current head fc4e4b2 differs from pull request most recent head 73a5296. Consider uploading reports for the commit 73a5296 to get more accurate results

@@             Coverage Diff             @@
##           master   #24851       +/-   ##
===========================================
+ Coverage    70.0%    82.0%    +11.9%     
===========================================
  Files          36      596      +560     
  Lines        2255   165043   +162788     
  Branches      322        0      -322     
===========================================
+ Hits         1580   135439   +133859     
- Misses        560    29604    +29044     
+ Partials      115        0      -115     

tao-stones
tao-stones previously approved these changes May 2, 2022
Copy link
Contributor

@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.

nice clean up, lgtm! Not sure tho if necessary to disable assert!(txs_processed < bank.transaction_count());

@mergify mergify bot dismissed tao-stones’s stale review May 2, 2022 20:04

Pull request has been modified.

@carllin carllin added the automerge Merge this Pull Request automatically once CI passes label May 2, 2022
@mergify mergify bot merged commit e83efe6 into solana-labs:master May 2, 2022
mergify bot pushed a commit that referenced this pull request May 2, 2022
* Cleanup banking bench

* Fully remove assert

(cherry picked from commit e83efe6)

# Conflicts:
#	banking-bench/src/main.rs
carllin added a commit that referenced this pull request May 4, 2022
* Cleanup banking bench

* Fully remove assert
mergify bot added a commit that referenced this pull request May 4, 2022
* Cleanup banking bench

* Fully remove assert

Co-authored-by: carllin <[email protected]>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
* Cleanup banking bench

* Fully remove assert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants