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

Use BankForks on tests - Part 4 #34271

Merged
merged 6 commits into from
Dec 6, 2023
Merged

Use BankForks on tests - Part 4 #34271

merged 6 commits into from
Dec 6, 2023

Conversation

LucasSte
Copy link
Contributor

Problem

In order for us to implement #34169 (remove WorkSlot from LoadedPrograms::extract), we need to make sure all tests create a BankFork and use a Bank instance that has been added to the fork.

Summary of Changes

This PR adds auxiliary functions to deal with BankForks and fixes tests in bench-tps, ledger, rpc, stake-accounts. This is the continuation of the work I started on #34238

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Merging #34271 (6811f48) into master (b97b3dd) will decrease coverage by 0.1%.
Report is 23 commits behind head on master.
The diff coverage is 100.0%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34271     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      220013   220120    +107     
=========================================
+ Hits       180353   180400     +47     
- Misses      39660    39720     +60     

@LucasSte LucasSte closed this Dec 1, 2023
@LucasSte LucasSte deleted the fork-part-4 branch December 1, 2023 18:53
@LucasSte LucasSte restored the fork-part-4 branch December 1, 2023 18:54
@LucasSte LucasSte reopened this Dec 1, 2023
Comment on lines 798 to 807
debug_assert!(self.fork_graph.is_some());
debug_assert_eq!(
Some(working_slot.current_epoch()),
self.fork_graph
.as_ref()
.unwrap()
.read()
.unwrap()
.slot_epoch(working_slot.current_slot())
);
Copy link
Contributor Author

@LucasSte LucasSte Dec 5, 2023

Choose a reason for hiding this comment

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

I've added the assertions to ensure no other tests are created without BankForks. Please, @pgarg66 let me know if I should remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the debug_assert_eq!. Looks like with this PR all the tests are fixed up. In the next PR you can remove the WorkingSlot, and just pass the current_slot as Slot value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I keep the first debuf_assert?
The second one ensures BankForks is properly set on the tests, because we fetch the epoch from it.

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've remove the debug_assert_eq.

@LucasSte LucasSte requested a review from pgarg66 December 5, 2023 21:32
@LucasSte LucasSte marked this pull request as ready for review December 5, 2023 21:32
@LucasSte LucasSte merged commit 1877fdb into solana-labs:master Dec 6, 2023
33 checks passed
@LucasSte LucasSte deleted the fork-part-4 branch December 6, 2023 16:32
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.

2 participants