Accounting migrating builtin programs default Compute Unit Limit with feature status #3975
Conversation
builtins-default-costs/src/lib.rs
Outdated
| /// Given a program pubkey, returns: | ||
| /// - None, if it is not in BUILTIN_INSTRUCTION_COSTS dictionary; | ||
| /// - Some<None>, is builtin, but no associated migration feature ID; | ||
| /// - Some<usize>, is builtin, and its associated migration feature ID | ||
| /// index in MIGRATION_FEATURES_ID. | ||
| pub fn get_builtin_migration_feature_index(program_id: &Pubkey) -> Option<Option<usize>> { |
There was a problem hiding this comment.
would it make sense to have a simple enum:
pub enum BuiltinMigrationFeatureIndex {
NotBuiltin,
BuiltinNoMigrationFeature,
BuiltinWithMigrationFeatureIndex(usize)
}naming probably too long, but it's a bit more self-documenting than Option<Option>.
tao-stones
force-pushed
the
fix-support-migrating-builtins
branch
from
b9a6fbe to
2c01afe
Compare
| ), | ||
| ]; | ||
|
|
||
| pub const NON_MIGRATING_BUILTINS_COSTS: &[(Pubkey, BuiltinCost)] = &[ |
There was a problem hiding this comment.
Can you add the developer warning to this const as well?
tao-stones
force-pushed
the
fix-support-migrating-builtins
branch
3 times, most recently
from
bde9c18 to
b6c391b
Compare
tao-stones
force-pushed
the
fix-support-migrating-builtins
branch
from
b6c391b to
f522323
Compare
tao-stones
force-pushed
the
fix-support-migrating-builtins
branch
from
e3ded01 to
28113aa
Compare
There was a problem hiding this comment.
The changes look okay to me, but things are definitely getting pretty complex. I'm still an advocate for unifying & simplifying, but I'm not sure if I have a better solution or not.
Let me know what you guys think of #4039. I took a stab at integrating this PR in the last commit, but it's a little half-baked.
Thanks @buffalojoec for looking at it, as we chatted offline, I 100% support unifying builtin list; Like #4039 a lot (left few comments) I appreciate the concern of complex. A part of that is to add positional info to avoid Apart from that, the major difference between #4039 and this PR is unifying builtins. I'm leaning to land (and backport) this PR first to unblock SIMD-170 implementation, then work your PR to improve builtins list (at that time, there will only have small/cosmetic changes to |
… its feature gate status
…ation (Vec<>) per transaction
add explicit positional information to migrating builtin feature obj update developer notes, added static_assertion to validate no new items are added, add enum type
tao-stones
force-pushed
the
fix-support-migrating-builtins
branch
from
28113aa to
6331283
Compare
That sounds good to me, thanks! |
| @@ -31,6 +50,7 @@ pub struct ComputeBudgetInstructionDetails { | |||
| // Additional builtin program counters | |||
| num_builtin_instructions: u16, | |||
There was a problem hiding this comment.
can we add a comment on this field, or preferrably change the name, so that it's clear that this count is only for builtins that are not possibly migrated?
The math in calculate_default_compute_unit_limit makes a lot more sense if that is clearer 😄
There was a problem hiding this comment.
s/num_builtin_instructions/num_non_migratable_builtin_instructions/g
tao-stones
added
the
automerge
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
… feature status (#3975) * Accounting migrating builtin programs default Compute Unit Limit with its feature gate status * Declare Non/migrating buiiltins in const array, eleminates heap allocation (Vec<>) per transaction * updates for review commients add explicit positional information to migrating builtin feature obj update developer notes, added static_assertion to validate no new items are added, add enum type * use enum to separately define migrating and not-migrating builtins * rename for clarity (cherry picked from commit 9379fbc) # Conflicts: # builtins-default-costs/src/lib.rs # compute-budget-instruction/Cargo.toml # compute-budget-instruction/src/builtin_programs_filter.rs # runtime-transaction/src/compute_budget_instruction_details.rs
… feature status (#3975) * Accounting migrating builtin programs default Compute Unit Limit with its feature gate status * Declare Non/migrating buiiltins in const array, eleminates heap allocation (Vec<>) per transaction * updates for review commients add explicit positional information to migrating builtin feature obj update developer notes, added static_assertion to validate no new items are added, add enum type * use enum to separately define migrating and not-migrating builtins * rename for clarity (cherry picked from commit 9379fbc)
… feature status (#3975) * Accounting migrating builtin programs default Compute Unit Limit with its feature gate status * Declare Non/migrating buiiltins in const array, eleminates heap allocation (Vec<>) per transaction * updates for review commients add explicit positional information to migrating builtin feature obj update developer notes, added static_assertion to validate no new items are added, add enum type * use enum to separately define migrating and not-migrating builtins * rename for clarity (cherry picked from commit 9379fbc)
… feature status (#3975) * Accounting migrating builtin programs default Compute Unit Limit with its feature gate status * Declare Non/migrating buiiltins in const array, eleminates heap allocation (Vec<>) per transaction * updates for review commients add explicit positional information to migrating builtin feature obj update developer notes, added static_assertion to validate no new items are added, add enum type * use enum to separately define migrating and not-migrating builtins * rename for clarity (cherry picked from commit 9379fbc)
Problem
When a builtin program migrated into sbpf, it should no longer be considered as 'builtin', therefore its default Compute Budget Limit should change from 3K to 200K CU, per #3799. It is currently not supported by Compute Budget Instruction processing.
Summary of Changes
3entries) vectorMigrationBuiltinFeatureCounterto transaction static meta;try_from()feature_setto resolve if builtin has migrated duringcalculate_default_compute_unit_limit()if Compute Unit Limit was not requested.Fixes #