Skip to content

Remove SVM dependency on agave-feature-set #5841

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

Merged
merged 3 commits into from
Apr 18, 2025
Merged

Remove SVM dependency on agave-feature-set #5841

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb102f7
Isolate runtime specific features via RuntimeFeatures struct
pgarg66 Apr 15, 2025
9f58db3
test cleanup
pgarg66 Apr 16, 2025
7281edb
address review feedback
pgarg66 Apr 17, 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
3 changes: 1 addition & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cli/src/program.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3023,7 +3023,7 @@ fn verify_elf(
) -> Result<(), Box<dyn std::error::Error>> {
// Verify the program
let program_runtime_environment = create_program_runtime_environment_v1(
&feature_set,
&feature_set.runtime_features(),
&SVMTransactionExecutionBudget::default(),
true,
false,
Expand Down
2 changes: 1 addition & 1 deletion cli/src/program_v4.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ pub fn process_deploy_program(
}
let program_runtime_environment =
solana_bpf_loader_program::syscalls::create_program_runtime_environment_v1(
&feature_set,
&feature_set.runtime_features(),
&SVMTransactionExecutionBudget::default(),
true,
false,
Expand Down
1 change: 1 addition & 0 deletions feature-set/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ solana-frozen-abi-macro = { workspace = true, optional = true, features = [
"frozen-abi",
] }
solana-hash = { workspace = true }
solana-program-runtime = { workspace = true }
solana-pubkey = { workspace = true, default-features = false }
solana-sha256-hasher = { workspace = true }

Expand Down
60 changes: 60 additions & 0 deletions feature-set/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use {
ahash::{AHashMap, AHashSet},
solana_epoch_schedule::EpochSchedule,
solana_hash::Hash,
solana_program_runtime::invoke_context::RuntimeFeatures,
solana_pubkey::Pubkey,
solana_sha256_hasher::Hasher,
std::sync::LazyLock,
Expand Down Expand Up @@ -98,6 +99,65 @@ impl FeatureSet {
self.activated_slot(&reduce_stake_warmup_cooldown::id())
.map(|slot| epoch_schedule.get_epoch(slot))
}

pub fn runtime_features(&self) -> RuntimeFeatures {
RuntimeFeatures {
lift_cpi_caller_restriction: self.activated_slot(&lift_cpi_caller_restriction::id()),
Copy link

Choose a reason for hiding this comment

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

This seems like a lot of repeated code. Do you think we can abstract this initialization in a macro? The struct members are named the same as the features.

Copy link
Author

Choose a reason for hiding this comment

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

It'll remove the possibility of naming the feature ID with a different name than what we call in SVM repo. It's easier to call it the same since the features already exist. But, when SVM is a separate repo, it might be extra work to enforce it.

We can probably add a macro in a separate PR, so if we don't like the rigidity in the future we can just revert that PR.

move_precompile_verification_to_svm: self
.activated_slot(&move_precompile_verification_to_svm::id()),
remove_accounts_executable_flag_checks: self
.activated_slot(&remove_accounts_executable_flag_checks::id()),
bpf_account_data_direct_mapping: self
.activated_slot(&bpf_account_data_direct_mapping::id()),
enable_bpf_loader_set_authority_checked_ix: self
.activated_slot(&enable_bpf_loader_set_authority_checked_ix::id()),
enable_loader_v4: self.activated_slot(&enable_loader_v4::id()),
deplete_cu_meter_on_vm_failure: self
.activated_slot(&deplete_cu_meter_on_vm_failure::id()),
abort_on_invalid_curve: self.activated_slot(&abort_on_invalid_curve::id()),
blake3_syscall_enabled: self.activated_slot(&blake3_syscall_enabled::id()),
curve25519_syscall_enabled: self.activated_slot(&curve25519_syscall_enabled::id()),
disable_deploy_of_alloc_free_syscall: self
.activated_slot(&disable_deploy_of_alloc_free_syscall::id()),
disable_fees_sysvar: self.activated_slot(&disable_fees_sysvar::id()),
disable_sbpf_v0_execution: self.activated_slot(&disable_sbpf_v0_execution::id()),
enable_alt_bn128_compression_syscall: self
.activated_slot(&enable_alt_bn128_compression_syscall::id()),
enable_alt_bn128_syscall: self.activated_slot(&enable_alt_bn128_syscall::id()),
enable_big_mod_exp_syscall: self.activated_slot(&enable_big_mod_exp_syscall::id()),
enable_get_epoch_stake_syscall: self
.activated_slot(&enable_get_epoch_stake_syscall::id()),
enable_poseidon_syscall: self.activated_slot(&enable_poseidon_syscall::id()),
enable_sbpf_v1_deployment_and_execution: self
.activated_slot(&enable_sbpf_v1_deployment_and_execution::id()),
enable_sbpf_v2_deployment_and_execution: self
.activated_slot(&enable_sbpf_v2_deployment_and_execution::id()),
enable_sbpf_v3_deployment_and_execution: self
.activated_slot(&enable_sbpf_v3_deployment_and_execution::id()),
get_sysvar_syscall_enabled: self.activated_slot(&get_sysvar_syscall_enabled::id()),
last_restart_slot_sysvar: self.activated_slot(&last_restart_slot_sysvar::id()),
reenable_sbpf_v0_execution: self.activated_slot(&reenable_sbpf_v0_execution::id()),
remaining_compute_units_syscall_enabled: self
.activated_slot(&remaining_compute_units_syscall_enabled::id()),
remove_bpf_loader_incorrect_program_id: self
.activated_slot(&remove_bpf_loader_incorrect_program_id::id()),
move_stake_and_move_lamports_ixs: self
.activated_slot(&move_stake_and_move_lamports_ixs::id()),
stake_raise_minimum_delegation_to_1_sol: self
.activated_slot(&stake_raise_minimum_delegation_to_1_sol::id()),
deprecate_legacy_vote_ixs: self.activated_slot(&deprecate_legacy_vote_ixs::id()),
mask_out_rent_epoch_in_vm_serialization: self
.activated_slot(&mask_out_rent_epoch_in_vm_serialization::id()),
simplify_alt_bn128_syscall_error_codes: self
.activated_slot(&simplify_alt_bn128_syscall_error_codes::id()),
fix_alt_bn128_multiplication_input_length: self
.activated_slot(&fix_alt_bn128_multiplication_input_length::id()),
loosen_cpi_size_restriction: self.activated_slot(&loosen_cpi_size_restriction::id()),
increase_tx_account_lock_limit: self
.activated_slot(&increase_tx_account_lock_limit::id()),
disable_rent_fees_collection: self.activated_slot(&disable_rent_fees_collection::id()),
}
}
}

pub mod deprecate_rewards_sysvar {
Expand Down
6 changes: 5 additions & 1 deletion ledger/src/leader_schedule_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,11 @@ mod tests {
&vote_account,
&validator_identity,
bootstrap_validator_stake_lamports()
+ solana_stake_program::get_minimum_delegation(&bank.feature_set),
+ solana_stake_program::get_minimum_delegation(
bank.feature_set.is_active(
&agave_feature_set::stake_raise_minimum_delegation_to_1_sol::id(),
),
),
);
let node_pubkey = validator_identity.pubkey();

Expand Down
1 change: 0 additions & 1 deletion program-runtime/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ license = { workspace = true }
edition = { workspace = true }

[dependencies]
agave-feature-set = { workspace = true }
base64 = { workspace = true }
bincode = { workspace = true }
enum-iterator = { workspace = true }
Expand Down
124 changes: 108 additions & 16 deletions program-runtime/src/invoke_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ use {
stable_log,
sysvar_cache::SysvarCache,
},
agave_feature_set::{
lift_cpi_caller_restriction, remove_accounts_executable_flag_checks, FeatureSet,
},
solana_account::{create_account_shared_data_for_test, AccountSharedData},
solana_clock::Slot,
solana_epoch_schedule::EpochSchedule,
Expand Down Expand Up @@ -44,6 +41,87 @@ use {
},
};

#[derive(Clone, Copy, Default)]
pub struct RuntimeFeatures {
pub lift_cpi_caller_restriction: Option<Slot>,
pub move_precompile_verification_to_svm: Option<Slot>,
pub remove_accounts_executable_flag_checks: Option<Slot>,
pub bpf_account_data_direct_mapping: Option<Slot>,
pub enable_bpf_loader_set_authority_checked_ix: Option<Slot>,
pub enable_loader_v4: Option<Slot>,
pub deplete_cu_meter_on_vm_failure: Option<Slot>,
pub abort_on_invalid_curve: Option<Slot>,
pub blake3_syscall_enabled: Option<Slot>,
pub curve25519_syscall_enabled: Option<Slot>,
pub disable_deploy_of_alloc_free_syscall: Option<Slot>,
pub disable_fees_sysvar: Option<Slot>,
pub disable_sbpf_v0_execution: Option<Slot>,
pub enable_alt_bn128_compression_syscall: Option<Slot>,
pub enable_alt_bn128_syscall: Option<Slot>,
pub enable_big_mod_exp_syscall: Option<Slot>,
pub enable_get_epoch_stake_syscall: Option<Slot>,
pub enable_poseidon_syscall: Option<Slot>,
pub enable_sbpf_v1_deployment_and_execution: Option<Slot>,
pub enable_sbpf_v2_deployment_and_execution: Option<Slot>,
pub enable_sbpf_v3_deployment_and_execution: Option<Slot>,
pub get_sysvar_syscall_enabled: Option<Slot>,
pub last_restart_slot_sysvar: Option<Slot>,
pub reenable_sbpf_v0_execution: Option<Slot>,
pub remaining_compute_units_syscall_enabled: Option<Slot>,
pub remove_bpf_loader_incorrect_program_id: Option<Slot>,
pub move_stake_and_move_lamports_ixs: Option<Slot>,
pub stake_raise_minimum_delegation_to_1_sol: Option<Slot>,
pub deprecate_legacy_vote_ixs: Option<Slot>,
pub mask_out_rent_epoch_in_vm_serialization: Option<Slot>,
pub simplify_alt_bn128_syscall_error_codes: Option<Slot>,
pub fix_alt_bn128_multiplication_input_length: Option<Slot>,
pub loosen_cpi_size_restriction: Option<Slot>,
pub increase_tx_account_lock_limit: Option<Slot>,
pub disable_rent_fees_collection: Option<Slot>,
}

impl RuntimeFeatures {
pub fn all_enabled() -> Self {
Self {
lift_cpi_caller_restriction: Some(0),
move_precompile_verification_to_svm: Some(0),
remove_accounts_executable_flag_checks: Some(0),
bpf_account_data_direct_mapping: Some(0),
enable_bpf_loader_set_authority_checked_ix: Some(0),
enable_loader_v4: Some(0),
deplete_cu_meter_on_vm_failure: Some(0),
abort_on_invalid_curve: Some(0),
blake3_syscall_enabled: Some(0),
curve25519_syscall_enabled: Some(0),
disable_deploy_of_alloc_free_syscall: Some(0),
disable_fees_sysvar: Some(0),
disable_sbpf_v0_execution: Some(0),
enable_alt_bn128_compression_syscall: Some(0),
enable_alt_bn128_syscall: Some(0),
enable_big_mod_exp_syscall: Some(0),
enable_get_epoch_stake_syscall: Some(0),
enable_poseidon_syscall: Some(0),
enable_sbpf_v1_deployment_and_execution: Some(0),
enable_sbpf_v2_deployment_and_execution: Some(0),
enable_sbpf_v3_deployment_and_execution: Some(0),
get_sysvar_syscall_enabled: Some(0),
last_restart_slot_sysvar: Some(0),
reenable_sbpf_v0_execution: Some(0),
remaining_compute_units_syscall_enabled: Some(0),
remove_bpf_loader_incorrect_program_id: Some(0),
move_stake_and_move_lamports_ixs: Some(0),
stake_raise_minimum_delegation_to_1_sol: Some(0),
deprecate_legacy_vote_ixs: Some(0),
mask_out_rent_epoch_in_vm_serialization: Some(0),
simplify_alt_bn128_syscall_error_codes: Some(0),
fix_alt_bn128_multiplication_input_length: Some(0),
loosen_cpi_size_restriction: Some(0),
increase_tx_account_lock_limit: Some(0),
disable_rent_fees_collection: Some(0),
}
}
}

pub type BuiltinFunctionWithContext = BuiltinFunction<InvokeContext<'static>>;

/// Adapter so we can unify the interfaces of built-in programs and syscalls
Expand Down Expand Up @@ -147,15 +225,15 @@ pub struct EnvironmentConfig<'a> {
pub blockhash: Hash,
pub blockhash_lamports_per_signature: u64,
epoch_stake_callback: &'a dyn InvokeContextCallback,
pub feature_set: Arc<FeatureSet>,
feature_set: Arc<RuntimeFeatures>,
Copy link

Choose a reason for hiding this comment

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

Any chance we can take this opportunity to get rid of the Arc? I can't think of anywhere we actually need a thread-safe atomic reference to a set of Option<Slot>. The feature set should be frozen from the time SVM is instantiated to to the time it's torn down.

Copy link
Author

Choose a reason for hiding this comment

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

I was keeping this change for a follow on PR, as it'll increase the size of this PR. Let me see how much additional changes it'll cause. If it's reasonable, I will update it here.

Copy link

Choose a reason for hiding this comment

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

Ok, follow-up PR is also okay, but I definitely think we should take the opportunity to eliminate it while our attention is here.

sysvar_cache: &'a SysvarCache,
}
impl<'a> EnvironmentConfig<'a> {
pub fn new(
blockhash: Hash,
blockhash_lamports_per_signature: u64,
epoch_stake_callback: &'a dyn InvokeContextCallback,
feature_set: Arc<FeatureSet>,
feature_set: Arc<RuntimeFeatures>,
sysvar_cache: &'a SysvarCache,
) -> Self {
Self {
Expand Down Expand Up @@ -426,9 +504,7 @@ impl<'a> InvokeContext<'a> {

// Find and validate executables / program accounts
let callee_program_id = instruction.program_id;
let program_account_index = if self
.get_feature_set()
.is_active(&lift_cpi_caller_restriction::id())
let program_account_index = if self.get_feature_set().lift_cpi_caller_restriction.is_some()
{
self.transaction_context
.find_index_of_program_account(&callee_program_id)
Expand All @@ -446,9 +522,10 @@ impl<'a> InvokeContext<'a> {
let borrowed_program_account = instruction_context
.try_borrow_instruction_account(self.transaction_context, program_account_index)?;
#[allow(deprecated)]
if !self
if self
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
.remove_accounts_executable_flag_checks
.is_none()
&& !borrowed_program_account.is_executable()
{
ic_msg!(self, "Account {} is not executable", callee_program_id);
Expand Down Expand Up @@ -521,7 +598,8 @@ impl<'a> InvokeContext<'a> {
*borrowed_root_account.get_key()
} else if self
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
.remove_accounts_executable_flag_checks
.is_some()
{
if bpf_loader_deprecated::check_id(owner_id)
|| bpf_loader::check_id(owner_id)
Expand Down Expand Up @@ -646,17 +724,32 @@ impl<'a> InvokeContext<'a> {
}

/// Get the current feature set.
pub fn get_feature_set(&self) -> &FeatureSet {
pub fn get_feature_set(&self) -> &RuntimeFeatures {
&self.environment_config.feature_set
}

/// Set feature set.
///
/// Only use for tests and benchmarks.
pub fn mock_set_feature_set(&mut self, feature_set: Arc<FeatureSet>) {
#[cfg(feature = "dev-context-only-utils")]
pub fn mock_set_feature_set(&mut self, feature_set: Arc<RuntimeFeatures>) {
self.environment_config.feature_set = feature_set;
}

pub fn is_stake_raise_minimum_delegation_to_1_sol_active(&self) -> bool {
self.environment_config
.feature_set
.stake_raise_minimum_delegation_to_1_sol
.is_some()
}

pub fn is_deprecate_legacy_vote_ixs_active(&self) -> bool {
self.environment_config
.feature_set
.deprecate_legacy_vote_ixs
.is_some()
}

/// Get cached sysvars
pub fn get_sysvar_cache(&self) -> &SysvarCache {
self.environment_config.sysvar_cache
Expand Down Expand Up @@ -738,14 +831,13 @@ macro_rules! with_mock_invoke_context {
$transaction_accounts:expr $(,)?
) => {
use {
agave_feature_set::FeatureSet,
solana_log_collector::LogCollector,
solana_svm_callback::InvokeContextCallback,
solana_type_overrides::sync::Arc,
$crate::{
__private::{Hash, ReadableAccount, Rent, TransactionContext},
execution_budget::{SVMTransactionExecutionBudget, SVMTransactionExecutionCost},
invoke_context::{EnvironmentConfig, InvokeContext},
invoke_context::{EnvironmentConfig, InvokeContext, RuntimeFeatures},
loaded_programs::ProgramCacheForTxBatch,
sysvar_cache::SysvarCache,
},
Expand Down Expand Up @@ -783,7 +875,7 @@ macro_rules! with_mock_invoke_context {
Hash::default(),
0,
&MockInvokeContextCallback {},
Arc::new(FeatureSet::all_enabled()),
Arc::new(RuntimeFeatures::all_enabled()),
&sysvar_cache,
);
let mut program_cache_for_tx_batch = ProgramCacheForTxBatch::default();
Expand Down
3 changes: 2 additions & 1 deletion program-test/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ pub fn invoke_builtin_function(
// Serialize entrypoint parameters with SBF ABI
let mask_out_rent_epoch_in_vm_serialization = invoke_context
.get_feature_set()
.is_active(&agave_feature_set::mask_out_rent_epoch_in_vm_serialization::id());
.mask_out_rent_epoch_in_vm_serialization
.is_some();
let (mut parameter_bytes, _regions, _account_lengths) = serialize_parameters(
transaction_context,
instruction_context,
Expand Down
2 changes: 1 addition & 1 deletion programs/bpf_loader/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ license = { workspace = true }
edition = { workspace = true }

[dependencies]
agave-feature-set = { workspace = true }
bincode = { workspace = true }
libsecp256k1 = { workspace = true }
num-traits = { workspace = true }
Expand Down Expand Up @@ -60,6 +59,7 @@ solana-epoch-schedule = { workspace = true }
solana-fee-calculator = { workspace = true }
solana-last-restart-slot = { workspace = true }
solana-program = { workspace = true }
solana-program-runtime = { workspace = true, features = ["dev-context-only-utils"] }
solana-pubkey = { workspace = true, features = ["rand"] }
solana-rent = { workspace = true }
solana-slot-hashes = { workspace = true }
Expand Down
Loading
Loading