default compute units per instruction #24899

jackcmay · 2022-05-02T17:13:48Z

Problem

Defaulting the tx-wide cap to the max overestimates what a transaction will need and later will skew transaction scheduling for transactions that don't explicitly request

Summary of Changes

Default the units to 200k per non-compute budget instruction in the transaction

Fixes #
Feature Gate Issue: #24898

program-runtime/src/compute_budget.rs

sdk/src/feature_set.rs

codecov · 2022-05-02T19:40:59Z

Codecov Report

Merging #24899 (a5570b7) into master (8ba003a) will increase coverage by 0.1%.
The diff coverage is 88.7%.

@@            Coverage Diff            @@
##           master   #24899     +/-   ##
=========================================
+ Coverage    81.8%    82.0%   +0.1%     
=========================================
  Files         632      597     -35     
  Lines      167499   165739   -1760     
  Branches      322        0    -322     
=========================================
- Hits       137169   136047   -1122     
+ Misses      30217    29692    -525     
+ Partials      113        0    -113

jstarry · 2022-05-03T10:23:54Z

docs/src/developing/programming-model/runtime.md

-compute units used by each instruction in the transaction must not exceed that
-value. The default number of maximum units allows is intentionally kept large to
-avoid breaking existing client behavior.
+default number of maximum units will calculated per instruction which means the


I don't really understand what this is trying to say

better? #24955

(cherry picked from commit e070c5c) # Conflicts: # docs/src/developing/programming-model/runtime.md # sdk/src/feature_set.rs

(cherry picked from commit e070c5c)

arcticmatt · 2022-05-03T17:39:41Z

would be good to update the docs https://docs.solana.com/developing/programming-model/runtime#transaction-wide-compute-budget

* default compute units per instruction (#24899) (cherry picked from commit e070c5c) # Conflicts: # docs/src/developing/programming-model/runtime.md # sdk/src/feature_set.rs * fix merge Co-authored-by: Jack May <[email protected]>

* default compute units per instruction (#24899) (cherry picked from commit e070c5c) * update doc Co-authored-by: Jack May <[email protected]>

t-nelson

gate lgtm. I thought we were going to swing a heavier hammer here, but I guess this is fine too.

t-nelson · 2022-05-03T22:19:18Z

program-runtime/src/compute_budget.rs

+                // Compute budget instruction must be in the 1st 3 instructions (avoid
+                // nonce marker), otherwise ignored
+                if i < 3 {
+                    match try_from_slice_unchecked(&instruction.data) {


OT, but should it be an error to specify any of these multiple times?

eh, just take the last

To be fair, it's pretty confusing when looking at the explorer and some transactions have like 10 compute budget instructions and you have to know it's the 3rd one that was applied

t-nelson · 2022-05-03T22:19:54Z

program-runtime/src/compute_budget.rs

+            requested_units
+        }
+        .unwrap_or(MAX_UNITS as u64)
+        .min(MAX_UNITS as u64);


should we really clamp here? failure is always an option

figure clamp and hope for best, max might change but in a way that would still allow the tx to succeed

yeah, just thought it might be confusing DX

t-nelson · 2022-05-03T22:25:05Z

program-runtime/src/compute_budget.rs

+
+        self.max_units = if default_units_per_instruction {
+            requested_units
+                .or_else(|| Some(num_instructions.saturating_mul(DEFAULT_UNITS as usize) as u64))


I thought we were just requesting DEFAULT_UNITS for the entire TX if unspecified, not retaining today's behavior. I guess this is OK, but I'd rather put some pressure on the offenders to change their ways

This is the smoothest transition to tx-wide caps without breaking folks and the additional fee will schedule folks appropriately. I'm still a fan of charging a requested CU based fee to encourage low CU requests. @aeyakovenko using for additional fee based motivation, open market.

I think we should still cap the number of instructions for this calculation.. Like maybe 10 instructions? If you're doing more than that, you're a bot and can add the compute budget ix yourself.

It's capped at a total maximum unit which should be sufficient, what does capping the ix's doing for us? Seems artificial to say that if there are 10 instructions it must be a bot

It's capped at a total maximum unit which should be sufficient

Ah ok, this works too! I clearly didn't read the PR 😅

This is the smoothest transition to tx-wide caps without breaking folks

Yep. I was just fully prepared to break the right folks 😉

jackcmay added the feature-gate Pull Request adds or modifies a runtime feature gate label May 2, 2022

jackcmay requested review from Lichtso and aeyakovenko May 2, 2022 17:13

default compute units per instruction

624f465

jackcmay force-pushed the default-units-per-instruction branch from 5b52cae to 624f465 Compare May 2, 2022 17:22

aeyakovenko reviewed May 2, 2022

View reviewed changes

program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved

feedback

9ca2d60

Lichtso reviewed May 2, 2022

View reviewed changes

program-runtime/src/compute_budget.rs Show resolved Hide resolved

program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved

program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved

sdk/src/feature_set.rs Outdated Show resolved Hide resolved

jackcmay added 2 commits May 2, 2022 10:45

feedback

c60d2fb

doc updates

a5570b7

jackcmay requested a review from Lichtso May 2, 2022 18:17

Lichtso approved these changes May 2, 2022

View reviewed changes

jackcmay requested a review from aeyakovenko May 2, 2022 18:41

aeyakovenko approved these changes May 2, 2022

View reviewed changes

jstarry reviewed May 3, 2022

View reviewed changes

jackcmay requested a review from t-nelson May 3, 2022 16:15

jackcmay added v1.9 labels May 3, 2022

jackcmay merged commit e070c5c into solana-labs:master May 3, 2022

jackcmay deleted the default-units-per-instruction branch May 3, 2022 16:50

mergify bot pushed a commit that referenced this pull request May 3, 2022

default compute units per instruction (#24899)

41f1d96

(cherry picked from commit e070c5c) # Conflicts: # docs/src/developing/programming-model/runtime.md # sdk/src/feature_set.rs

mergify bot mentioned this pull request May 3, 2022

default compute units per instruction (backport #24899) #24944

Merged

mergify bot pushed a commit that referenced this pull request May 3, 2022

default compute units per instruction (#24899)

4b93476

(cherry picked from commit e070c5c)

mergify bot mentioned this pull request May 3, 2022

default compute units per instruction (backport #24899) #24945

Merged

jackcmay added a commit that referenced this pull request May 3, 2022

default compute units per instruction (backport #24899) (#24945)

bc27344

* default compute units per instruction (#24899) (cherry picked from commit e070c5c) * update doc Co-authored-by: Jack May <[email protected]>

t-nelson reviewed May 3, 2022

View reviewed changes

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022

default compute units per instruction (solana-labs#24899)

7e988fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default compute units per instruction #24899

default compute units per instruction #24899

jackcmay commented May 2, 2022

codecov bot commented May 2, 2022

jstarry May 3, 2022

t-nelson May 3, 2022

arcticmatt commented May 3, 2022

t-nelson left a comment

t-nelson May 3, 2022

jackcmay May 3, 2022

jstarry May 4, 2022

t-nelson May 3, 2022

jackcmay May 3, 2022

t-nelson May 4, 2022

t-nelson May 3, 2022

jackcmay May 3, 2022

jstarry May 4, 2022

jackcmay May 4, 2022

jstarry May 4, 2022

t-nelson May 4, 2022

default compute units per instruction #24899

default compute units per instruction #24899

Conversation

jackcmay commented May 2, 2022

Problem

Summary of Changes

codecov bot commented May 2, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcticmatt commented May 3, 2022

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment