Feature gate builtin consumes static units during processing instruction #30702

tao-stones · 2023-03-13T23:49:36Z

Problem

For below mentioned feature gated issue, PR #30680 presented one possible solution to charge default units if (and only if) builtins don't manually deduct units after process_instruction(). @Lichtso pointed out two issues with it:

the prerequisite add default_cost as mandatory field for Builtin #30639 conflicts with the undergoing refactoring works; (detail here)
the preferred practice is to deduct units before processing, not after.

@Lichtso and I huddled offline, proposed to update all builtins to consume statically defined units at beginning of process_instruction. The pros: above mentioned 2 concerns are addressed; the cons: it is not easy to abandon builtin costs (re)defined in runtime/src/block_cost_limits.rs, which adds cost of maintenance. (with previous approach, i was planning to remove that HashMap from block_cost_limit, instead fetch builtin cost from bank.builtins.default_compute_unit_cost)

Summary of Changes

add feature gate
implement second proposal.

Feature Gate Issue: #30620

joncinque

The change looks good to me! To prevent this in the future, do you want to add something in the native instruction processor which errors if no compute units are used? This can be addressed in a followup with a separate feature gate if you prefer.

tao-stones · 2023-03-16T16:20:47Z

To prevent this in the future, do you want to add something in the native instruction processor which errors if no compute units are used? This can be addressed in a followup with a separate feature gate if you prefer.

Like this idea, makes sense to include this change in same feature gate. But can you say more about it? Not super clear what "native instruction processor" refer to, and how that would prevent future native program to consume units.

As for error, would a new InstructionError::InstructionNotConsumeUnits suffice?

tao-stones · 2023-03-16T22:27:46Z

Also I wish to refactor the hardcoded number into named constants. so
this
invoke_context.consume_checked(150)?;
would becomes:
invoke_context.consume_checked(solana_sdk::system_program::DEFAULT_COMPUTE_UNIT_COST)?;
and their tests as well.
I'm thinking to add const right next to each program id declarion, so

crate::declare_id!(...);
pub const DEFAUL_COMPUT_UNIT_COST: u64 = ....;

Would this break any convention? @joncinque @Lichtso

joncinque · 2023-03-17T01:54:23Z

Like this idea, makes sense to include this change in same feature gate. But can you say more about it? Not super clear what "native instruction processor" refer to, and how that would prevent future native program to consume units.

In your other PR, you checked to see if consumed compute units was 0 before deducting the default amount at https://github.com/solana-labs/solana/pull/30680/files#diff-9ac236825ce9b87946d6820e3ec3096bc841818c66037b38b6d98142318f4b0aR783-R791

How about returning an error if consumed compute units is 0?

joncinque · 2023-03-17T01:55:23Z

Also I wish to refactor the hardcoded number into named constants

If it's only used in one place, I think it's fine to keep the hardcoded amounts, but it's a matter of taste

tao-stones · 2023-03-17T06:29:17Z

In your other PR, you checked to see if consumed compute units was 0 before ...

Aaah, gotcha! The word "native" threw me off, thought it's something else 😜 Thanks for explaining,

Also I wish to refactor the hardcoded number into named constants

If it's only used in one place, I think it's fine to keep the hardcoded amounts, but it's a matter of taste

Yeah ok, let's leave this one out for now. It can be done as separately without needing another feature gate.

tao-stones · 2023-03-17T06:31:35Z

you checked to see if consumed compute units was 0 ...

Aaah, gotcha. Sorry "native" threw me off early, thought referring to something else 😜 . comment 254da8b does it.

tao-stones · 2023-03-17T22:54:07Z

program-runtime/src/invoke_context.rs

@@ -769,6 +773,17 @@ impl<'a> InvokeContext<'a> {
                let post_remaining_units = self.get_remaining();
                *compute_units_consumed = pre_remaining_units.saturating_sub(post_remaining_units);

+                // startign from feature `native_programs_consume_cu`, all builtin programs should consume


Adding this to enforce future native programs to consume units.

tao-stones · 2023-03-17T22:59:24Z

program-test/src/lib.rs

+        // programs that do not consume units.
+        let mut deactivate_feature_set = HashSet::default();
+        deactivate_feature_set.insert(solana_sdk::feature_set::native_programs_consume_cu::id());
+


Existing mock/test programs do not consume units, the enforcement added above not only breaks all tests under program-test/tests and some zk-token-proof tests, it also breaks downstream tests (such as spl). I deactivated this feature for ProgramTest here to continue support existing tests.

No problem, that makes sense for these, especially in "native" mode. We could eventually change program-test to consume some default amount in "native" mode.

tao-stones · 2023-03-17T23:01:58Z

runtime/src/bank.rs

@@ -281,7 +281,7 @@ impl RentDebits {
 }

 pub type BankStatusCache = StatusCache<Result<()>>;
-#[frozen_abi(digest = "3qia1Zm8X66bzFaBuC8ahz3hADRRATyUPRV36ZzrSois")]
+#[frozen_abi(digest = "FT6XYtzchg9h2JACEKKf8FZD5skSnPzKY4YrJ1jUWr8U")]


abi change due to adding InstructionError

codecov · 2023-03-18T02:08:17Z

Codecov Report

Merging #30702 (d661745) into master (5a05e9b) will decrease coverage by 0.1%.
The diff coverage is 59.5%.

@@            Coverage Diff            @@
##           master   #30702     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         726      726             
  Lines      204809   204914    +105     
=========================================
+ Hits       167027   167080     +53     
- Misses      37782    37834     +52

tao-stones · 2023-03-18T02:23:02Z

@joncinque @Lichtso adding check to enforce builtin to consume units broke a ton tests all over place :) Took time to use the opportunity to learn some details about runtime and program_test. This PR is cleaned up, ready for another review.

joncinque

Thanks for fixing up all those tests, this is looking really close!

joncinque · 2023-03-20T18:53:41Z

program-test/src/lib.rs

+        // programs that do not consume units.
+        let mut deactivate_feature_set = HashSet::default();
+        deactivate_feature_set.insert(solana_sdk::feature_set::native_programs_consume_cu::id());
+


No problem, that makes sense for these, especially in "native" mode. We could eventually change program-test to consume some default amount in "native" mode.

program-test/src/lib.rs

program-runtime/src/invoke_context.rs

programs/sbf/tests/programs.rs

sdk/program/src/instruction.rs

joncinque · 2023-03-20T21:15:48Z

program-runtime/src/invoke_context.rs

+                        .feature_set
+                        .is_active(&native_programs_consume_cu::id())
+                {
+                    return Err(InstructionError::BuiltinProgramsMustConsumeUnits);


Now that I think about it more -- consuming 0 units is a programmer error and not a user error, so we could be even more aggressive and assert.

On the flipside, since the runtime is used by other developers, like in all of the program tests, they should probably get a nice error rather than an assert. I'm leaning toward keeping this as an error, but I wanted to bring up the discussion to see how you thought.

Great observation! I agree it'd be a programmer error. Also agree runtime is library other developers use; so it'd be great, as good SDK, to return meaningful errors for unsupported user invokes. I'd vote for keep error too.

joncinque

Looks good to me, but be sure to get feedback from @Lichtso too, to make sure that it interacts properly with any runtime changes

…truction()

- return error if builtin did not consume units, as a way to enforce builtin to consume units; - updated related tests

…re to continue support existing mock/test programs that do not consume units

tao-stones added the feature-gate Pull Request adds or modifies a runtime feature gate label Mar 13, 2023

tao-stones mentioned this pull request Mar 13, 2023

builtin program consumes default cost if instruction not manually deduct units #30680

Closed

tao-stones requested a review from Lichtso March 14, 2023 00:05

Lichtso previously approved these changes Mar 14, 2023

View reviewed changes

tao-stones requested a review from joncinque March 14, 2023 15:30

joncinque previously approved these changes Mar 15, 2023

View reviewed changes

tao-stones mentioned this pull request Mar 16, 2023

Revert "add default_cost as mandatory field for Builtin (#30639)" #30749

Merged

tao-stones dismissed stale reviews from joncinque and Lichtso via ce1c510 March 16, 2023 15:08

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch from ce1c510 to e121b44 Compare March 16, 2023 16:01

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch from e121b44 to 33af2ef Compare March 16, 2023 18:19

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch from 33af2ef to 90f05ad Compare March 16, 2023 22:34

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch 2 times, most recently from da7a35d to b5f3f27 Compare March 17, 2023 20:13

tao-stones commented Mar 17, 2023

View reviewed changes

tao-stones requested review from Lichtso and joncinque March 18, 2023 02:20

joncinque reviewed Mar 20, 2023

View reviewed changes

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch 2 times, most recently from a77cb21 to 6c199dc Compare March 21, 2023 03:52

tao-stones requested a review from joncinque March 21, 2023 05:42

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch from 6c199dc to de8a840 Compare March 21, 2023 19:34

joncinque previously approved these changes Mar 23, 2023

View reviewed changes

Lichtso previously approved these changes Mar 24, 2023

View reviewed changes

tao-stones added 6 commits March 24, 2023 14:28

add feature gate

41ffc56

builtins consume statically defined units at beginning of process_ins…

ca99c93

…truction()

update tests

0ed05cf

- Add new instructionError;

040cffb

- return error if builtin did not consume units, as a way to enforce builtin to consume units; - updated related tests

updated ProgramTest with desctivated native_programs_consume_cu featu…

696dd04

…re to continue support existing mock/test programs that do not consume units

review updates

d661745

tao-stones dismissed stale reviews from Lichtso and joncinque via d661745 March 24, 2023 14:28

tao-stones force-pushed the feature-gate-builtin-consumes-static-units-per-ix branch from de8a840 to d661745 Compare March 24, 2023 14:28

Lichtso approved these changes Mar 24, 2023

View reviewed changes

tao-stones merged commit 3e500d9 into solana-labs:master Mar 24, 2023

tao-stones deleted the feature-gate-builtin-consumes-static-units-per-ix branch March 24, 2023 16:31

Lichtso mentioned this pull request Mar 24, 2023

Refactor - mock_process_instruction() #30893

Merged

tao-stones mentioned this pull request May 30, 2023

Most builtin programs do not consume compute units #25401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature gate builtin consumes static units during processing instruction #30702

Feature gate builtin consumes static units during processing instruction #30702

tao-stones commented Mar 13, 2023 •

edited

Loading

joncinque left a comment

tao-stones commented Mar 16, 2023

tao-stones commented Mar 16, 2023

joncinque commented Mar 17, 2023

joncinque commented Mar 17, 2023

tao-stones commented Mar 17, 2023

tao-stones commented Mar 17, 2023

tao-stones Mar 17, 2023 •

edited

Loading

tao-stones Mar 17, 2023

joncinque Mar 20, 2023

tao-stones Mar 17, 2023

codecov bot commented Mar 18, 2023 •

edited

Loading

tao-stones commented Mar 18, 2023

joncinque left a comment

joncinque Mar 20, 2023

joncinque Mar 20, 2023

tao-stones Mar 21, 2023

joncinque left a comment

Feature gate builtin consumes static units during processing instruction #30702

Feature gate builtin consumes static units during processing instruction #30702

Conversation

tao-stones commented Mar 13, 2023 • edited Loading

Problem

Summary of Changes

joncinque left a comment

Choose a reason for hiding this comment

tao-stones commented Mar 16, 2023

tao-stones commented Mar 16, 2023

joncinque commented Mar 17, 2023

joncinque commented Mar 17, 2023

tao-stones commented Mar 17, 2023

tao-stones commented Mar 17, 2023

tao-stones Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones Mar 17, 2023

Choose a reason for hiding this comment

joncinque Mar 20, 2023

Choose a reason for hiding this comment

tao-stones Mar 17, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2023 • edited Loading

Codecov Report

tao-stones commented Mar 18, 2023

joncinque left a comment

Choose a reason for hiding this comment

joncinque Mar 20, 2023

Choose a reason for hiding this comment

joncinque Mar 20, 2023

Choose a reason for hiding this comment

tao-stones Mar 21, 2023

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

tao-stones commented Mar 13, 2023 •

edited

Loading

tao-stones Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 18, 2023 •

edited

Loading