Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix reserve minimal cus for builtins #3755

Closed
wants to merge 7 commits into from
Closed

Fix reserve minimal cus for builtins #3755

wants to merge 7 commits into from

Conversation

tao-stones
Copy link

@tao-stones tao-stones commented Nov 22, 2024 •
edited
Loading

Problem

Implementing SIMD-170 by defining MAX_BUILTIN_ALLOCATION_COMPUTE_UNIT_LIMIT to 3K CUs, then use it to allocate 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)
  • add final tests to verify new behavior

Feature Gate Issue: #2562

@tao-stones tao-stones added the feature-gate Pull Request adds or modifies a runtime feature gate label Nov 22, 2024
@tao-stones tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins branch 2 times, most recently from f3ead6a to abf2430 Compare November 22, 2024 23:59
jstarry
jstarry reviewed Nov 23, 2024
View reviewed changes
cost-model/src/cost_model.rs Show resolved Hide resolved
cost-model/src/cost_model.rs
@@ -313,7 +346,9 @@ mod tests {
use {
super::*,
itertools::Itertools,
log::debug,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests here for the most part should be testing the cost model with and without the new feature enabled

programs/compute-budget-bench/benches/compute_budget.rs Outdated
solana_svm_transaction::instruction::SVMInstruction,
std::num::NonZero,
};

const ONE_PAGE: u32 = 32 * 1024;
const SIXTY_FOUR_MB: u32 = 64 * 1024 * 1024;

fn feature_set() -> FeatureSet {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benches should create the feature_set() outside of the bench run to avoid creating the feature hashmap each iteration

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating default feature_set in each bench, outside bench_function, to be friendlier for backporting. Will do followup PR to add for feature_set_enabled in [true, false] { bench_function(...) }

runtime-transaction/benches/process_compute_budget_instructions.rs Outdated
@@ -17,6 +18,10 @@ use {
const NUM_TRANSACTIONS_PER_ITER: usize = 1024;
const DUMMY_PROGRAM_ID: &str = "dummmy1111111111111111111111111111111111111";

fn feature_set() -> FeatureSet {
FeatureSet::all_enabled()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with these benches, shouldn't create the feature set hashmap on each bench iter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made same change

runtime-transaction/src/compute_budget_program_id_filter.rs Outdated Show resolved Hide resolved
runtime-transaction/src/compute_budget_program_id_filter.rs Outdated Show resolved Hide resolved
runtime-transaction/src/compute_budget_program_id_filter.rs Outdated Show resolved Hide resolved
@tao-stones tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins branch 2 times, most recently from 6b850be to 8394c7a Compare November 25, 2024 16:18
@tao-stones tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins branch from 8394c7a to 45d9672 Compare November 25, 2024 22:37
@tao-stones tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins branch from 45d9672 to 75826ae Compare November 26, 2024 06:57
@mergify Mergify
Copy link

mergify bot commented Nov 26, 2024

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @tao-stones.

@jstarry jstarry requested review from t-nelson and removed request for t-nelson November 27, 2024 17:31
@tao-stones
Copy link
Author

Adding feature_set to try_from() is to determine if a builtin program has been migrated during static_meta construction. However this change touches too many files, and also breaks design - makes StaticMeta depends on feature_set.

Close this in fav of #3799, which caches "migrating builtin info" as part of static meta, only to resolve its actual status during sanitizing where feature_set is available.

@tao-stones tao-stones closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstarry jstarry jstarry left review comments

Assignees
No one assigned
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tao-stones @jstarry