Accounting migrating builtin programs default Compute Unit Limit with feature status

[tao-stones]

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

• Add fixed-length (currently 3 entries) vector MigrationBuiltinFeatureCounter to transaction static meta;
• Counts migrating builtin features gates during try_from()
• Check feature_set to resolve if builtin has migrated during calculate_default_compute_unit_limit() if Compute Unit Limit was not requested.

Fixes #

[tao-stones]

performance-wise: does same one lookup of AhashMap, then additional O(n) search, where n is 3.

[apfitzge]

we could store a unique index in BuiltinCost. and we could have some tests to really simply verify the indexes are contiguous on (0..N) and unique.

byte per key in the BUILTIN_INSTRUCTION_COSTS, but then we don't need to do ~3 pubkey comparisons.

Thoughts?

[tao-stones]

We can do something like this:

pub const MIGRATING_BUILTINS_COSTS: &[(Pubkey, BuiltinCost)] = &[
    (
        solana_stake_program::id(),
        BuiltinCost {
            native_cost: solana_stake_program::stake_instruction::DEFAULT_COMPUTE_UNITS,
            core_bpf_migration_feature: Some(feature_set::migrate_stake_program_to_core_bpf::id()),
            index: 0,   <=== 
        },
    ),
    (
        solana_config_program::id(),
        BuiltinCost {
            native_cost: solana_config_program::config_processor::DEFAULT_COMPUTE_UNITS,
            core_bpf_migration_feature: Some(feature_set::migrate_config_program_to_core_bpf::id()),
            index: 1,   <====
        },
    ),
    ...
];

index correctness can be validated by tests during CI, or perhaps by asserts during compile-time. But need some push to be convinced this is Clean Code.

[apfitzge]

Yeah, this is exactly what I would like 😄

"clean" be damned if it's faster and works. Tests can easily verify.

Probably best to put it inside the Option of core_bpf_migration_feature? That way it's effectively not set for non-migrating buitins.

[tao-stones]

"clean" be damned if it's faster and works. Tests can easily verify.

respectively disagree.

[tao-stones]

b6c391b added explicit positional info to BuiltinCost, const function to validate position correctness at compile test. Also added unit test per suggestion.

[pgarg66]

Looks good from SVM side.

[apfitzge]

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]

in b6c391b

[jstarry]

Can you add the developer warning to this const as well?

[tao-stones]

in b6c391b, above

[buffalojoec]

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.

[tao-stones]

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 iter, and additional scaffolding for compile-time validation (0a747da).

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 compute-budget-instruction crate).

[buffalojoec]

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 compute-budget-instruction crate).

That sounds good to me, thanks!