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

SIMD-0256: Raise block limit to 60M #5171

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

SIMD-0256: Raise block limit to 60M #5171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d626d66
SIMD 256
bw-solana Mar 6, 2025
2dd419c
another test
bw-solana Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions cost-model/src/block_cost_limits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub const INSTRUCTION_DATA_BYTES_COST: u64 = 140 /*bytes per us*/ / COMPUTE_UNIT
/// data size and built-in and SBF instructions.
pub const MAX_BLOCK_UNITS: u64 = 48_000_000;
pub const MAX_BLOCK_UNITS_SIMD_0207: u64 = 50_000_000;
pub const MAX_BLOCK_UNITS_SIMD_0256: u64 = 60_000_000;

/// Number of compute units that a writable account in a block is allowed. The
/// limit is to prevent too many transactions write to same account, therefore
Expand All @@ -53,3 +54,16 @@ pub const fn simd_0207_block_limits() -> (u64, u64, u64) {
MAX_VOTE_UNITS,
)
}

/// Return the block limits that will be used upon activation of SIMD-0256.
/// Returns as
/// (account_limit, block_limit, vote_limit)
// ^ Above order is used to be consistent with the order of
// `CostTracker::set_limits`.
pub const fn simd_0256_block_limits() -> (u64, u64, u64) {
Copy link
Author

Choose a reason for hiding this comment

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

@t-nelson - this probably makes your eyes bleed... mea culpa.

Given we're going to keep doing this several more times, what would you like this to look like?

Copy link

@apfitzge apfitzge Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Given we plan to continue this chain, maybe it's better to not use a tuple, and just define a new structure:

pub struct BlockLimits {
    account_cost_limit: u64,
    block_cost_limit: u64,
    vote_cost_limit: u64,
}

refactor CostTracker::set_limit to take this object and store it internally in the CostTracker as well.

edit: fields can be private as well...so then only constructors of this are guaranteed to be from one of our functions in block_cost_limit.rs 😄

Copy link

@apfitzge apfitzge Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if we plan to backport this, would prefer to keep the simple tuple. then do refactoring only on master

(
MAX_WRITABLE_ACCOUNT_UNITS,
MAX_BLOCK_UNITS_SIMD_0256,
MAX_VOTE_UNITS,
)
}
26 changes: 25 additions & 1 deletion runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ use {
solana_builtins::{prototype::BuiltinPrototype, BUILTINS, STATELESS_BUILTINS},
solana_compute_budget::compute_budget::ComputeBudget,
solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions,
solana_cost_model::{block_cost_limits::simd_0207_block_limits, cost_tracker::CostTracker},
solana_cost_model::{
block_cost_limits::{simd_0207_block_limits, simd_0256_block_limits},
cost_tracker::CostTracker,
},
solana_feature_set::{self as feature_set, FeatureSet},
solana_fee::FeeFeatures,
solana_lattice_hash::lt_hash::LtHash,
Expand Down Expand Up @@ -4844,6 +4847,18 @@ impl Bank {
);
}

if self
.feature_set
.is_active(&feature_set::raise_block_limits_to_60m::id())
{
let (account_cost_limit, block_cost_limit, vote_cost_limit) = simd_0256_block_limits();
self.write_cost_tracker().unwrap().set_limits(
account_cost_limit,
block_cost_limit,
vote_cost_limit,
);
}

// If the accounts delta hash is still in use, start the background account hasher
if !self
.feature_set
Expand Down Expand Up @@ -6525,6 +6540,15 @@ impl Bank {
);
}

if new_feature_activations.contains(&feature_set::raise_block_limits_to_60m::id()) {
Copy link
Author

Choose a reason for hiding this comment

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

not sure exactly how we want to handle this conditional logic.. E.g. what would we want to happen if both features were activated in the same epoch?

Currently, we will write the 207 limits then the 256 limits. That seems right to me. Practically speaking, we will activate these in separate epochs, of course.

same thing applies to fn finish_init above

Copy link

@apfitzge apfitzge Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

intent is for 207, then 256. the ordering here to me makes sense; if 256 is active, we use 256 limits, regardless of the status of 207.

Copy link

Choose a reason for hiding this comment

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

we should have a test for that case as well? activate 256 first, then 207, make sure we have 256 limits.

Copy link
Author

Choose a reason for hiding this comment

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

test added

let (account_cost_limit, block_cost_limit, vote_cost_limit) = simd_0256_block_limits();
self.write_cost_tracker().unwrap().set_limits(
account_cost_limit,
block_cost_limit,
vote_cost_limit,
);
}

if new_feature_activations.contains(&feature_set::remove_accounts_delta_hash::id()) {
// If the accounts delta hash has been removed, then we no longer need to compute the
// AccountHash for modified accounts, and can stop the background account hasher.
Expand Down
133 changes: 131 additions & 2 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ use {
compute_budget::ComputeBudget,
compute_budget_limits::{self, ComputeBudgetLimits, MAX_COMPUTE_UNIT_LIMIT},
},
solana_cost_model::block_cost_limits::{MAX_BLOCK_UNITS, MAX_BLOCK_UNITS_SIMD_0207},
solana_cost_model::block_cost_limits::{
MAX_BLOCK_UNITS, MAX_BLOCK_UNITS_SIMD_0207, MAX_BLOCK_UNITS_SIMD_0256,
},
solana_feature_set::{self as feature_set, FeatureSet},
solana_inline_spl::token,
solana_logger,
Expand Down Expand Up @@ -8050,9 +8052,14 @@ fn test_reserved_account_keys() {
fn test_block_limits() {
let (bank0, _bank_forks) = create_simple_test_arc_bank(100_000);
let mut bank = Bank::new_from_parent(bank0, &Pubkey::default(), 1);

// Ensure increased block limits features are inactive.
assert!(!bank
.feature_set
.is_active(&feature_set::raise_block_limits_to_50m::id()));
assert!(!bank
.feature_set
.is_active(&feature_set::raise_block_limits_to_60m::id()));
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS,
Expand Down Expand Up @@ -8081,10 +8088,39 @@ fn test_block_limits() {
);

// Make sure the limits propagate to the child-bank.
let bank = Bank::new_from_parent(Arc::new(bank), &Pubkey::default(), 2);
let mut bank = Bank::new_from_parent(Arc::new(bank), &Pubkey::default(), 2);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0207,
"child bank should have new limit"
);

// Activate `raise_block_limits_to_60m` feature
bank.store_account(
&feature_set::raise_block_limits_to_60m::id(),
&feature::create_account(&Feature::default(), 42),
);
// apply_feature_activations for `FinishInit` will not cause the block limit to be updated
bank.apply_feature_activations(ApplyFeatureActivationsCaller::FinishInit, true);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0207,
"before activating the feature, bank should have old/default limit"
);

// apply_feature_activations for `NewFromParent` will cause feature to be activated
bank.apply_feature_activations(ApplyFeatureActivationsCaller::NewFromParent, true);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0256,
"after activating the feature, bank should have new limit"
);

// Make sure the limits propagate to the child-bank.
let bank = Bank::new_from_parent(Arc::new(bank), &Pubkey::default(), 3);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0256,
"child bank should have new limit"
);

Expand All @@ -8111,6 +8147,99 @@ fn test_block_limits() {
MAX_BLOCK_UNITS_SIMD_0207,
"bank created from genesis config should have new limit"
);

activate_feature(
&mut genesis_config,
feature_set::raise_block_limits_to_60m::id(),
);
let bank = Bank::new_for_tests(&genesis_config);
assert!(bank
.feature_set
.is_active(&feature_set::raise_block_limits_to_60m::id()));
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0256,
"bank created from genesis config should have new limit"
);
}

#[test]
fn test_block_limits_feature_priority() {
let (bank0, _bank_forks) = create_simple_test_arc_bank(100_000);
let mut bank = Bank::new_from_parent(bank0, &Pubkey::default(), 1);

// Ensure increased block limits features are inactive.
assert!(!bank
.feature_set
.is_active(&feature_set::raise_block_limits_to_50m::id()));
assert!(!bank
.feature_set
.is_active(&feature_set::raise_block_limits_to_60m::id()));
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS,
"before activating the feature, bank should have old/default limit"
);

// Activate `raise_block_limits_to_50m` feature
bank.store_account(
&feature_set::raise_block_limits_to_50m::id(),
&feature::create_account(&Feature::default(), 42),
);

// Activate `raise_block_limits_to_60m` feature
bank.store_account(
&feature_set::raise_block_limits_to_60m::id(),
&feature::create_account(&Feature::default(), 42),
);

// apply_feature_activations for `NewFromParent` will cause `raise_block_limits_to_60m` to be activated
bank.apply_feature_activations(ApplyFeatureActivationsCaller::NewFromParent, true);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0256,
"after activating the feature, bank should have new limit"
);

// Make sure the limits propagate to the child-bank.
let bank = Bank::new_from_parent(Arc::new(bank), &Pubkey::default(), 3);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0256,
"child bank should have new limit"
);

// Test starting from a genesis config with and without feature account
let (mut genesis_config, _keypair) = create_genesis_config(100_000);
// Without feature account in genesis, old limits are used.
let bank = Bank::new_for_tests(&genesis_config);
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS,
"before activating the feature, bank should have old/default limit"
);

// Activate `raise_block_limits_to_50m` feature
activate_feature(
&mut genesis_config,
feature_set::raise_block_limits_to_50m::id(),
);
// Activate `raise_block_limits_to_60m` feature
activate_feature(
&mut genesis_config,
feature_set::raise_block_limits_to_60m::id(),
);
let bank = Bank::new_for_tests(&genesis_config);

// `raise_block_limits_to_60m` feature and block limits should be active.
assert!(bank
.feature_set
.is_active(&feature_set::raise_block_limits_to_60m::id()));
assert_eq!(
bank.read_cost_tracker().unwrap().get_block_limit(),
MAX_BLOCK_UNITS_SIMD_0256,
"bank created from genesis config should have new limit"
);
}

#[test]
Expand Down
Loading