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

[solana] Ensure window length cannot exceed checkpoint account limit #252

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

djb15
Copy link
Contributor

@djb15 djb15 commented Feb 19, 2025

Is there a better way to do this?

Adding this constraint breaks a bunch of tests and enforcing it in tests would make the tests extremely expensive (from a time point of view as we need to fill up checkpoint accounts for some tests), hence the conditional compilation

@djb15 djb15 changed the title solana: Ensure window length cannot exceed checkpoint account limit [solana] Ensure window length cannot exceed checkpoint account limit Feb 19, 2025
@djb15 djb15 force-pushed the solana/constrain-checkpoint-limit branch from b92f9e0 to 69eb209 Compare February 20, 2025 00:07
@djb15 djb15 requested review from zpoken, nvsriram and mdulin2 February 20, 2025 00:41
@djb15 djb15 force-pushed the solana/constrain-checkpoint-limit branch from 69eb209 to 5a1066b Compare February 20, 2025 01:37
nvsriram
nvsriram previously approved these changes Feb 20, 2025
@johnsaigle
Copy link

johnsaigle commented Feb 20, 2025

debug_assert!() might be a more light-weight choice if you're just doing this in once place.
https://doc.rust-lang.org/std/macro.debug_assert.html
Nevermind sorry I guess you only want to assert in prod, which is different

@djb15 djb15 force-pushed the solana/constrain-checkpoint-limit branch from 47251eb to 5a1066b Compare February 20, 2025 19:26
Copy link

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

I double-checked this and I believe you can achieve this with cargo's built-in test flag. This automatically gets set when running tests and not in production.

If you do this, you can make the change I suggested and revert the changes to Cargo.toml.

@johnsaigle johnsaigle dismissed their stale review February 21, 2025 15:56

bad suggestion!

@djb15 djb15 merged commit 0e9cfeb into main Feb 21, 2025
12 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.

3 participants