v2.1: Fix reserve minimal compute units for builtins (backport of #3799) #3931

mergify · 2024-12-05T03:51:02Z

Problem

Implementing solana-foundation/solana-improvement-documents#170 by defining MAX_BUILTIN_ALLOCATION_COMPUTE_UNIT_LIMIT to 3K CUs, then use it to allocate builtin instructions' CU Meters for VM and cost tracking for leaders.

Summary of Changes

When calculates default tx cu limits, use MAX_BUILTIN_ALLOCATION_COMPUTE_UNIT_LIMIT per builtin instruction, including compute-budget program instructions.
Cost model reads cu limits from RuntimeTransaction's static_meta, replacing a localized implementation that isn't consistent with compute-budget.
Changes are behind Feature gate
updated existing tests to allow additional feature_set parameters (touch many files)

Feature Gate Issue: #2562

This is an automatic backport of pull request #3799 done by [Mergify](https://mergify.com).

mergify · 2024-12-05T03:51:04Z

Cherry-pick of 3e9af14 has failed:

On branch mergify/bp/v2.1/pr-3799
Your branch is up to date with 'origin/v2.1'.

You are currently cherry-picking commit 3e9af14f3a.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   compute-budget/src/compute_budget_limits.rs
	modified:   cost-model/src/transaction_cost.rs
	modified:   programs/sbf/tests/programs.rs
	modified:   runtime-transaction/benches/process_compute_budget_instructions.rs
	new file:   runtime-transaction/src/builtin_programs_filter.rs
	modified:   runtime-transaction/src/compute_budget_instruction_details.rs
	modified:   runtime-transaction/src/compute_budget_program_id_filter.rs
	modified:   runtime-transaction/src/instructions_processor.rs
	modified:   runtime-transaction/src/runtime_transaction.rs
	modified:   runtime/src/bank.rs
	modified:   runtime/src/bank/tests.rs
	modified:   sdk/feature-set/src/lib.rs
	modified:   sdk/program/src/message/sanitized.rs
	modified:   sdk/program/src/message/versions/sanitized.rs
	modified:   svm-transaction/src/svm_message.rs
	modified:   svm-transaction/src/svm_message/sanitized_message.rs
	modified:   svm-transaction/src/svm_message/sanitized_transaction.rs
	modified:   svm/src/transaction_processor.rs
	modified:   transaction-view/src/resolved_transaction_view.rs
	modified:   transaction-view/src/transaction_view.rs

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   builtins-default-costs/src/lib.rs
	both modified:   core/src/banking_stage/consumer.rs
	both modified:   core/src/banking_stage/immutable_deserialized_packet.rs
	deleted by us:   core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs
	both modified:   cost-model/src/cost_model.rs
	deleted by us:   programs/compute-budget-bench/benches/compute_budget.rs
	both modified:   runtime-transaction/src/lib.rs
	deleted by us:   runtime-transaction/src/runtime_transaction/sdk_transactions.rs
	both modified:   runtime/src/prioritization_fee_cache.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

core/src/banking_stage/transaction_scheduler/scheduler_controller.rs

apfitzge

Only minor comment about a test change.

Conflict resolutions look correct to me, let's give Starry a chance to review.

core/src/banking_stage/transaction_scheduler/scheduler_controller.rs

cost-model/src/cost_model.rs

bw-solana

Seems fine to me, but this is obviously a very large backport, so I'm paranoid. Have we run any canaries on mainnet/testnet (with feature disabled, of course)?

cost-model/src/transaction_cost.rs

runtime-transaction/src/builtin_programs_filter.rs

cost-model/src/cost_model.rs

tao-stones

Yea, it is a large backport, but most files are touched to pass additional feature_set parameter. But still be good to run it on canary, I will grab a box to run it.

runtime-transaction/src/builtin_programs_filter.rs

cost-model/src/cost_model.rs

tao-stones · 2025-01-07T15:36:58Z

Seems fine to me, but this is obviously a very large backport, so I'm paranoid. Have we run any canaries on mainnet/testnet (with feature disabled, of course)?

The branch build was running on ds2 since Epoch 723, blocks it produced are 312342780 and after (right of cursor line in the chart below). It looks healthy so far.

bw-solana

LGTM.

Let's let @t-nelson take a look before we merge tho

- Add feature gate, issue #2562; - Implement SIMD-170; --------- Co-authored-by: Justin Starry <justin.m.starry@gmail.com> (cherry picked from commit 3e9af14)

anza-team · 2025-01-09T22:12:54Z

😱 New commits were pushed while the automerge label was present.

t-nelson

pain. this could have been done so much more reviewably

t-nelson · 2025-01-11T01:25:46Z

core/src/banking_stage/immutable_deserialized_packet.rs

@@ -40,6 +41,12 @@ pub enum DeserializedPacketError {
    FailedFilter(#[from] PacketFilterFailure),
 }

+lazy_static::lazy_static! {
+    // Make a dummy feature_set with all features enabled to


why? could have easily resolved the feature set higher in the call stack and passed a bool down instead of introducing an entire sentinel feature set instance

Considered that too, since we plan to remove immutable_deserialized_packet soon m(when legacy "scheduler" is removed), adding dummy here is easier for backporting than piping a bool through more files.

t-nelson · 2025-01-11T02:31:01Z

core/src/banking_stage/immutable_deserialized_packet.rs

+lazy_static::lazy_static! {
+    // Make a dummy feature_set with all features enabled to
+    // fetch compute_unit_price and compute_unit_limit for legacy leader.
+    static ref FEATURE_SET: FeatureSet = FeatureSet::all_enabled();


why all and not just the one we care about here? what if someone naively decides to leverage the arg in at an intermediary call site for something unrelated?

yea a lazy step for dummy.

t-nelson · 2025-01-11T03:22:59Z

runtime-transaction/src/builtin_programs_filter.rs

+    // array of slots for all possible static and sanitized program_id_index,
+    // each slot indicates if a program_id_index has not been checked (eg, None),
+    // or already checked with result (eg, Some(ProgramKind)) that can be reused.
+    program_kind: [Option<ProgramKind>; FILTER_SIZE as usize],


is this not super clumsy if we need to change static account limit?

The size of array is the largest possible pubkeys a packet can have: (PACKET_DATA_SIZE / core::mem::size_of::<Pubkey>()) as u8;, increase static account limits should not change its value.

mergify bot requested a review from a team as a code owner December 5, 2024 03:51

mergify bot added conflicts feature-gate Pull Request adds or modifies a runtime feature gate labels Dec 5, 2024

mergify bot assigned tao-stones Dec 5, 2024

tao-stones force-pushed the mergify/bp/v2.1/pr-3799 branch 2 times, most recently from f1d1130 to 33377db Compare December 11, 2024 21:06

tao-stones mentioned this pull request Dec 12, 2024

v2.1: Accounting migrating builtin programs default Compute Unit Limit with feature status (backport of #3975) #4091

Open

tao-stones force-pushed the mergify/bp/v2.1/pr-3799 branch from 2ccc80e to b869c2a Compare December 13, 2024 15:59

tao-stones requested review from jstarry and apfitzge December 13, 2024 16:00

tao-stones force-pushed the mergify/bp/v2.1/pr-3799 branch from b869c2a to cb4e3b9 Compare December 13, 2024 17:09

apfitzge reviewed Dec 16, 2024

View reviewed changes

core/src/banking_stage/transaction_scheduler/scheduler_controller.rs Show resolved Hide resolved

apfitzge previously approved these changes Dec 16, 2024

View reviewed changes

jstarry previously approved these changes Jan 6, 2025

View reviewed changes

core/src/banking_stage/transaction_scheduler/scheduler_controller.rs Show resolved Hide resolved

cost-model/src/cost_model.rs Outdated Show resolved Hide resolved

tao-stones dismissed stale reviews from jstarry and apfitzge via 4472e56 January 6, 2025 15:28

tao-stones added automerge automerge Merge this Pull Request automatically once CI passes and removed conflicts labels Jan 6, 2025

jstarry previously approved these changes Jan 6, 2025

View reviewed changes

bw-solana reviewed Jan 6, 2025

View reviewed changes

cost-model/src/transaction_cost.rs Show resolved Hide resolved

runtime-transaction/src/builtin_programs_filter.rs Show resolved Hide resolved

cost-model/src/cost_model.rs Show resolved Hide resolved

tao-stones reviewed Jan 6, 2025

View reviewed changes

runtime-transaction/src/builtin_programs_filter.rs Show resolved Hide resolved

cost-model/src/cost_model.rs Show resolved Hide resolved

bw-solana previously approved these changes Jan 7, 2025

View reviewed changes

tao-stones requested a review from t-nelson January 8, 2025 01:47

tao-stones and others added 3 commits January 9, 2025 16:12

Fix reserve minimal compute units for builtins (#3799)

32de336

- Add feature gate, issue #2562; - Implement SIMD-170; --------- Co-authored-by: Justin Starry <justin.m.starry@gmail.com> (cherry picked from commit 3e9af14)

Fix conflicts manually

77467c4

avoid getting program_ids that are not used after all

f144108

tao-stones dismissed stale reviews from bw-solana and jstarry via f144108 January 9, 2025 22:12

tao-stones force-pushed the mergify/bp/v2.1/pr-3799 branch from 4472e56 to f144108 Compare January 9, 2025 22:12

anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Jan 9, 2025

apfitzge approved these changes Jan 9, 2025

View reviewed changes

t-nelson approved these changes Jan 11, 2025

View reviewed changes

tao-stones merged commit 6ea38fc into v2.1 Jan 11, 2025
30 checks passed

tao-stones deleted the mergify/bp/v2.1/pr-3799 branch January 11, 2025 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2.1: Fix reserve minimal compute units for builtins (backport of #3799) #3931

v2.1: Fix reserve minimal compute units for builtins (backport of #3799) #3931

mergify bot commented Dec 5, 2024

mergify bot commented Dec 5, 2024

apfitzge left a comment

bw-solana left a comment

tao-stones left a comment

tao-stones commented Jan 7, 2025

bw-solana left a comment

anza-team commented Jan 9, 2025

t-nelson left a comment

t-nelson Jan 11, 2025

tao-stones Jan 11, 2025 •

edited

Loading

t-nelson Jan 11, 2025

tao-stones Jan 11, 2025

t-nelson Jan 11, 2025

tao-stones Jan 11, 2025

v2.1: Fix reserve minimal compute units for builtins (backport of #3799) #3931

v2.1: Fix reserve minimal compute units for builtins (backport of #3799) #3931

Conversation

mergify bot commented Dec 5, 2024

Problem

Summary of Changes

mergify bot commented Dec 5, 2024

apfitzge left a comment

Choose a reason for hiding this comment

bw-solana left a comment

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

tao-stones commented Jan 7, 2025

bw-solana left a comment

Choose a reason for hiding this comment

anza-team commented Jan 9, 2025

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson Jan 11, 2025

Choose a reason for hiding this comment

tao-stones Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

t-nelson Jan 11, 2025

Choose a reason for hiding this comment

tao-stones Jan 11, 2025

Choose a reason for hiding this comment

t-nelson Jan 11, 2025

Choose a reason for hiding this comment

tao-stones Jan 11, 2025

Choose a reason for hiding this comment

tao-stones Jan 11, 2025 •

edited

Loading