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

Do not run elapsed slots loop in tests #2411

Closed
wants to merge 1 commit into from

Conversation

LucasSte
Copy link

@LucasSte LucasSte commented Aug 2, 2024

Problem

A loop that calculates the number of elapsed slots between two epochs depend on the difference between the start and the end epoch:

let slots_elapsed: u64 = (account_rent_epoch..=self.epoch)

This loop can get quite slow (>20 minutes) if the difference between them is too large (around 2^40). Although this case may not happen on the network, it slows down fuzzing efforts.

Summary of Changes

I added a condition to avoid running the loop when the number of iterations is too high.

@LucasSte LucasSte marked this pull request as ready for review August 2, 2024 17:14
#[cfg(not(feature = "dev-context-only-utils"))]
let run_loop = true;

if self.epoch.saturating_sub(account_rent_epoch) > 2u64.saturating_pow(30) {

Choose a reason for hiding this comment

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

why was 2^30 chosen?

Copy link
Author

Choose a reason for hiding this comment

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

2^30 still executes quickly. 2^35 takes more than a minute.

#[cfg(feature = "dev-context-only-utils")]
let mut run_loop = true;
#[cfg(not(feature = "dev-context-only-utils"))]
let run_loop = true;
Copy link

@jeffwashington jeffwashington Aug 2, 2024

Choose a reason for hiding this comment

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

ok, so non-test cases (ie. normal validator), we will always run the loop.
Does the fuzzer run the validator (or sub-pieces) with dev-context-only-utils?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it does.

@brooksprumo
Copy link

brooksprumo commented Aug 2, 2024

My preference is to have the test work around the production code, instead of vice versa.

Is is possible to reduce the range for the epochs in the fuzzer?

@LucasSte
Copy link
Author

LucasSte commented Aug 2, 2024

My preference is to have the test work around the production code, instead of vice versa.

Is is possible to reduce the range for the epochs in the fuzzer?

Yes, I can follow this approach.
To be honest, having a loop that is susceptible to such a slowness still annoys me.

@LucasSte
Copy link
Author

LucasSte commented Aug 5, 2024

I'll follow @brooksprumo's advice to limit the range of rent epochs in the fuzzer.

@LucasSte LucasSte closed this Aug 5, 2024
@LucasSte LucasSte deleted the donot-run branch August 5, 2024 13:47
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.

3 participants