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

Reduce public visibility of compute-budget APIs #4601

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pgarg66
Copy link

@pgarg66 pgarg66 commented Jan 23, 2025

Problem

APIs and structs in compute-budget crate can have reduced visibility.

Summary of Changes

Update the visibility using feature config.

Fixes #

@pgarg66 pgarg66 marked this pull request as ready for review January 23, 2025 18:34
@pgarg66 pgarg66 requested review from a team as code owners January 23, 2025 18:34
@pgarg66
Copy link
Author

pgarg66 commented Jan 24, 2025

Hey @kevinheavey, 5e669b4 should address your comments in #4606.

@t-nelson
Copy link

isn't compute budget a protocol implementation detail used to configure the svm, not actually part of the svm?

alt_bn128_g2_decompress(pub),
)
)]
#[non_exhaustive]
pub struct ComputeBudget {

Choose a reason for hiding this comment

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

Could we leave these fields pub please? LiteSVM lets people overwrite the ComputeBudget fields.

I'm open to being told these fields are quite unstable and we should limit to a subset, since I doubt many users care about overwriting poseidon_cost_coefficient_a etc

@pgarg66
Copy link
Author

pgarg66 commented Jan 24, 2025

isn't compute budget a protocol implementation detail used to configure the svm, not actually part of the svm?

Ideally yes. In present form, the most of the fields are only used internally by SVM, with hardcoded values set in the constructors (default(), from() and new()):

impl Default for ComputeBudget {

Maybe we should allow the user of SVM to tweak these values, even though in Agave we are not changing most of it.

@t-nelson does that align with your thoughts?

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