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

Runtime: Expose builtin program IDs to crate #318

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

buffalojoec
Copy link

@buffalojoec buffalojoec commented Mar 19, 2024

This is chunk 2/7 of the broken-up PR #79.

Problem

As we prepare to migrate builtin programs to Core BPF, it's important to decouple
components such as snapshot minimization from the static BUILTINS list.

By adding a crate-only method to Bank that will allow a caller to immutably
borrow the bank's list of builtin program IDs, other runtime components can
get the bank's current list of supported builtins.

Summary of Changes

Two commits:

  1. Rename the bank's builtin_programs field to builtin_program_ids since the term "builtin" is heavily overloaded as it is, and this field is only the IDs, not the BuiltinPrototype elements, as seen elsewhere in bank.rs.
  2. Expose a crate-only method get_builtin_program_ids(&self) -> &HashSet<Pubkey> on the bank and use it in the snapshot minimizer.

@buffalojoec buffalojoec marked this pull request as ready for review March 19, 2024 17:39
@buffalojoec
Copy link
Author

A case can be made for making this method completely public, as suggested in #316.

Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

One comment about the metric name. Otherwise this lgtm.
However, it would be good to confirm with @Lichtso that we aren't missing some rationale for removing a similar bank method in solana-labs#31654 (aside from the convenience of using the static list directly).

runtime/src/bank/metrics.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 53.33333% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (54575fe) to head (34308e2).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #318     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         838      838             
  Lines      226927   226934      +7     
=========================================
- Hits       185901   185899      -2     
- Misses      41026    41035      +9     

Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Wfm, but please check in with @Lichtso /the runtime team before merge.

@buffalojoec buffalojoec merged commit f799c9f into anza-xyz:master Mar 22, 2024
37 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.

4 participants