SIMD-0170: Reserve minimal CUs for builtins

[tao-stones]

No description provided.

[buffalojoec]

The motivation makes sense to me, but does this proposal take into account builtin programs who CPI to other builtin programs? The address lookup table program, for example, consumes default CUs of 750, but a few instructions (create & extend) will CPI to the system program.

It might be difficult and/or brittle to hard-code all default CUs for builtin programs including via CPI. If we were going to go this route, I'd advocate for moving away from blanket CU usage across all instructions, and instead configuring a CU value per-instruction, which might make this benchmarking process a little safer.

[tao-stones]

The motivation makes sense to me, but does this proposal take into account builtin programs who CPI to other builtin programs? The address lookup table program, for example, consumes default CUs of 750, but a few instructions (create & extend) will CPI to the system program.

It might be difficult and/or brittle to hard-code all default CUs for builtin programs including via CPI. If we were going to go this route, I'd advocate for moving away from blanket CU usage across all instructions, and instead configuring a CU value per-instruction, which might make this benchmarking process a little safer.

Excellent point. This proposal calls for "A builtin instruction that might call other instructions (CPI) would fail without explicitly requesting more CUs." (In Detailed Design section, Example 2).

Budget was moved from "per instruction" to "per transaction", it might be good idea to revisit it. Another possible option to handle "builtin program that CPIs" is the second one in Alternatives Considered. But asking user to explicitly request cu limit seems to be most straightforward atm.

[ptaffet-jump]

Overall looks pretty good. I agree with Andrew's comment

If our cost-model says that builtin program Z always uses X CUs, then that should be what is actually used by the execution, regadless of what it does internally, including CPI.

As for the UX strangeness that this causes, I'd propose one of the following two:

1. Expose a CPI-inclusive number to the cost tracker and a CPI-exclusive number to the VM. This may have to be per-instruction then.
2. Make CPIs from native programs not consume CUs.

I'd be okay with either, though there's some complexity involved with per-instruction costs (suppose you distinguish instruction by first byte, then what if the instruction data is empty or not one of the known bytes? You throw the transaction out?).

[apfitzge]

Of @ptaffet-jump's 2 suggestions, I don't feasibly see how the per-instruction will work well. Especially given the last case he mentioned - what if the ix variant is invalid?

For option 2, if @tao-stones agrees it is reasonable approach, think we'll need to bring in someone from agave VM team to comment on how difficult it would be. And also how unsafe it would be - we definitely do not want to create a bug where user txs can CPI into native programs for free.
Just CPI from native programs being free.

[tao-stones]

Thanks for all the helpful inputs. It looks like the primary issue is handling builtins that make CPIs without introducing confusing or inconsistent user experiences. The potential solutions are converging too. @ptaffet-jump's option 1 is similar to @buffalojoec pseudo-code, his option 2 is inline with @apfitzge suggestion.

I am inclined toward the first option, which avoids introducing special cases into the VM and instead focuses on making builtin programs more transparent about their compute requirements, and most the logics are implemented within builtin-default-costs crate:

1. Changes to builtin programs:

• Expose DEFAULT_COMPUTE_UNIT per instruction (instead of currently per program), similar to ZK as mentioned above.
• Expose CPI instruction Array. Additionally, builtin programs should expose an array of instructions they invoke via CPIs. For example, create_address_lookup_table instruction that makes three CPIs to the system program would expose [system_ix, system_ix, system_ix]. Others might expose an empty array.
• This makes builtin programs more transparent about what they do.

2. Changes to builtin-default-costs crate:

• Dictionary of Instruction Costs and CPIs: Maintain a static dictionary with the structure <instruction, {ix_default_compute_units, cpi_list}> to store the default compute units and associated CPI instructions for each builtin instruction.
• Helper Function for CU Calculation that calculates the appropriate number of compute units to allocate per instruction based on the dictionary data, pseudo:

fn get_cu_for_allocation( &ix ) -> Result<u64> {
    let entry = get_dictionary_entry( ix ) ? ;
    let mut allocation_size = entry.value.ix_default_compute_units;
    for cpi_ix in entry.value.cpi_list {
        let cpi_ix_cost = get_cu_for_allocation( cpi_ix ) ? ;
        alloacation_size += cpi_ix_cost;
    }
    allocation_size
}

3. Call-Site Implementation:

• Instruction Type Lookup: at the call-site, such as within the compute budget or cost model, determine the type of builtin instruction. If the instruction type cannot be determined, returns Err(invalid_instruction_data_error).
• CU Allocation Calculation: If the instruction type is valid, use function provided by the builtins-default-costs crate to calculate the correct amount of compute units to allocate for that instruction.
• transaction's program_id_index is checked with above process only once, result is cached for reuse.

4. No Changes to VM:

wdyt?

[apfitzge]

All sounds reasonable except the error handling here:

Instruction Type Lookup: at the call-site, such as within the compute budget or cost model, determine the type of builtin instruction. If the instruction type cannot be determined, returns Err(invalid_instruction_data_error).

Dropping these on invalid ix data would be an attack vector.
Seems they should just have some "fallback" program cost, which represents the cost to deserialize/match on ix data enum variant.
and then let the tx error out at runtime.

[tao-stones]

Dropping these on invalid ix data would be an attack vector.

It just returns an error at early stage of process pipeline (before execution, like compute-budget is doing currently), leaders can decide to pack them and charge the fee, or drop them. If leaders can't do this yet, probably can keep current "per program default cost" as fallback.

[apfitzge]

Dropping these on invalid ix data would be an attack vector.

It just returns an error at early stage of process pipeline (before execution, like compute-budget is doing currently), leaders can decide to pack them and charge the fee, or drop them. If leaders can't do this yet, probably can keep current "per program default cost" as fallback.

Cannot do that right now. Code would be the same that we've already implemented for #82 - code is effectively done on our side, but that SIMD has not been agreed upon yet.

[buffalojoec]

I am inclined toward the first option, which avoids introducing special cases into the VM and instead focuses on making builtin programs more transparent about their compute requirements

Yeah, I think this is the right motivation and approach IMO.

Changes to builtin programs:
...
Expose CPI instruction Array. Additionally, builtin programs should expose an array of instructions they invoke via CPIs. For example, create_address_lookup_table instruction that makes three CPIs to the system program would expose [system_ix, system_ix, system_ix]. Others might expose an empty array.
...

Unfortunately, this isn't as straightforward to represent in an array like this. Programs may not always CPI each time they're invoked. Consider an instruction that may CPI once, may CPI twice, or may not CPI at all, considering some account state or input data.

For this reason, I think we should gear the pattern(s) toward using the maximum CUs possible by an instruction. In the above example, the instruction would define MAX_CUS_WITH_CPI (or whatever) as the worst-case, ie. 2 CPIs.

I'd be okay with either, though there's some complexity involved with per-instruction costs (suppose you distinguish instruction by first byte, then what if the instruction data is empty or not one of the known bytes? You throw the transaction out?).

We also probably need to enforce standards for builtin instructions. Right now, they're all 4-byte (u32) instruction discriminators. The CU definitions should be required to map to these discriminators. On the Agave side, we can just make this a trait for builtin instructions.

A few more suggestions from my side for contributors' QoL:

• This isn't something we do now, but we should explicitly forbid CPIs from builtin programs to BPF programs. Should be posted somewhere obvious and maybe even included in this SIMD (if relevant)?
• I suggest some test suite requirement for all builtins that tests their CU declarations against the proposed runtime change. This way, if someone defines CUs wrong, the runtime should error on budget exceeded in their test.

What do you guys think?

[tao-stones]

Unfortunately, this isn't as straightforward to represent in an array like this. Programs may not always CPI each time they're invoked. Consider an instruction that may CPI once, may CPI twice, or may not CPI at all, considering some account state or input data.

For this reason, I think we should gear the pattern(s) toward using the maximum CUs possible by an instruction. In the above example, the instruction would define MAX_CUS_WITH_CPI (or whatever) as the worst-case, ie. 2 CPIs.

Thanks for bringing this up. I was assuming builtin instructions have rather fixed CPIs schema, not aware there are instances that dynamically based on account states. I only know that "create lookup table" always CPIs "system" 3 times, and "extend lookup account" CPI "system" once. Most likely I am not up to date with builtins, if there are more dynamic scenarios, then MAX_CUS_WITH_CPI is a good idea to me.

A few more suggestions from my side for contributors' QoL:

A great list of TODOs! To add to it:

• forbid builtin from nested CPIs (builtin CPIs to anothe rBuiltin that CPIs to another builtin); to extend that, possible to limit builtin to only statically CPIs at top level?
• is it possible to add static assertion, or tests, to ensure newly created builtin program, or instruction, that complies with all this buitins rules? And are included in the "dictionary"
  ( Maybe this all belong to separate SIMD)

[buffalojoec]

Thanks for bringing this up. I was assuming builtin instructions have rather fixed CPIs schema, not aware there are instances that dynamically based on account states. I only know that "create lookup table" always CPIs "system" 3 times, and "extend lookup account" CPI "system" once.

We could go through and profile all of the processors to make sure they're fixed, but we'd also have to impose this constraint on any new instructions/processors. Considering your last bullet (below), it might also be harder to programmatically enforce.

is it possible to add static assertion, or tests, to ensure newly created builtin program, or instruction, that complies with all this buitins rules? And are included in the "dictionary"
( Maybe this all belong to separate SIMD)

Yeah, I think some kind of interface (trait for Agave) for builtins and a testing standard (check instruction stack height for example) can accomplish this.

IMO we probably don't need a separate SIMD, we can introduce the constraints in this one, and mention that all builtins are already compliant as-is. Since the introduction of these constraints doesn't inherently change anything about the current protocol, I lean toward not requiring they be proposed in a new SIMD.

[tao-stones]

We could go through and profile all of the processors to make sure they're fixed, but we'd also have to impose this constraint on any new instructions/processors.

Yea, I take it back, such constraint is unnecessarily restrictive. Make builtin programs to expose worse-case CUs, as you suggested, is better.

If no other objects, I'll include updated option one to proposal.

[tao-stones]

IMO we probably don't need a separate SIMD, we can introduce the constraints in this one, and mention that all builtins are already compliant as-is. Since the introduction of these constraints doesn't inherently change anything about the current protocol, I lean toward not requiring they be proposed in a new SIMD.

For the sake of documentation, the constrains all current and future builtins should comply, and testing standard they must follow, deserve its own SIMD. Would work better for multiple clients too. ( Plus I am not the right person to draft these rules for builtins 😄 )

[ksolana]

It is likely that CUs can change in future (increase because of additional checks etc., decrease because of more optimizations etc.)

In case CU for a particular instruction changes beyond a certain tolerance, how do we propose to update it?

[jstarry]

@topointon-jump this is ready for review!

[buffalojoec]

Can you add a line here that also states the builtin must be owned by the native loader for this CU allocation to apply? Avoids any footguns when they move to BPF.

[buffalojoec]

The list can still be static, we just also want that explicit requirement.

[tao-stones]

added in 0e6e0ca

[jstarry]

I think we should update this list of builtin program id's when each of the builtin-to-bpf feature gates are activated. We're not going to check the owner of the builtin program each time during cost calculation.

[buffalojoec]

Works for me, as long as an ID can be popped out on feature activation.

[jstarry]

Yup, @tao-stones please make sure @buffalojoec gets added as a reviewer on your impl pr

[tao-stones]

yep, will do

[apfitzge]

@Benhawkins18 can we get this merged? @tao-stones is already implementing it. We have 2 folks from Anza, and @topointon-jump from Jump approving.

[Benhawkins18]

I see approvals from both jump and Anza. Merging