Skip to content

svm repo split: decouple dev deps: feature-set #7461

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+191 −26
Open

svm repo split: decouple dev deps: feature-set #7461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

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

166 changes: 166 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 @@ -161,6 +161,172 @@ impl FeatureSet {
}
}

impl From<&SVMFeatureSet> for FeatureSet {
fn from(svm_features: &SVMFeatureSet) -> Self {
Comment on lines +164 to +165
Copy link

Choose a reason for hiding this comment

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

I don't see any case when one should be converting the SVM feature set to the Agave feature set. How about marking this function as dev-context-only-utils?

Copy link

Choose a reason for hiding this comment

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

I definitely agree with this, SVM -> agave feature set conversion is lossy and should only be available in dev-context if we need it.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah but I can't without still requiring it as a dev dependency. Where would I specify I need the feature?

Copy link

Choose a reason for hiding this comment

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

Perhaps you could implement that in the test file?

Copy link
Author

Choose a reason for hiding this comment

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

SVM -> agave feature set conversion is lossy

How is it lossy? Lossy would actually be the other direction.

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps you could implement that in the test file?

If I use FeatureSet in the test file, then solana-svm will require agave-feature-set as a dev dependency.

Copy link

Choose a reason for hiding this comment

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

"lossy" is the wrong word I guess. SVM -> Agave is not a straight-forward conversion. you have to make some assumption of what the other features in agave are. Should they all be active? inactive? there's no obvious choice to me which one is "correct".

Copy link

Choose a reason for hiding this comment

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

If I use FeatureSet in the test file, then solana-svm will require agave-feature-set as a dev dependency.

I don't understand though. The only place this into is used is in the svm integration tests.
It has to be a dev dependency already with this setup, no?

Copy link

Choose a reason for hiding this comment

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

like maybe it's not a direct dev-dependency. but it's somehow dependent on it because the process_compute_budget_instructions takes it. so that function depends on FeatureSet

Copy link
Author

Choose a reason for hiding this comment

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

like maybe it's not a direct dev-dependency. but it's somehow dependent on it because the process_compute_budget_instructions takes it. so that function depends on FeatureSet

Yeah, maybe I need to go a few levels out to get rid of the dependency(ies). I'm trying to avoid Agave crates depending on SVM crates, which in turn depend on Agave crates for testing. It'll be a nightmare.

let mut active = AHashMap::new();
let mut inactive = AHashSet::from_iter((*FEATURE_NAMES).keys().cloned());

let mut activate_if = |enabled: &bool, feature_id: Pubkey| {
if *enabled {
inactive.remove(&feature_id);
active.insert(feature_id, 0);
}
};

let SVMFeatureSet {
move_precompile_verification_to_svm,
stricter_abi_and_runtime_constraints,
enable_bpf_loader_set_authority_checked_ix,
enable_loader_v4,
deplete_cu_meter_on_vm_failure,
abort_on_invalid_curve,
blake3_syscall_enabled,
curve25519_syscall_enabled,
disable_deploy_of_alloc_free_syscall,
disable_fees_sysvar,
disable_sbpf_v0_execution,
enable_alt_bn128_compression_syscall,
enable_alt_bn128_syscall,
enable_big_mod_exp_syscall,
enable_get_epoch_stake_syscall,
enable_poseidon_syscall,
enable_sbpf_v1_deployment_and_execution,
enable_sbpf_v2_deployment_and_execution,
enable_sbpf_v3_deployment_and_execution,
get_sysvar_syscall_enabled,
last_restart_slot_sysvar,
reenable_sbpf_v0_execution,
remaining_compute_units_syscall_enabled,
remove_bpf_loader_incorrect_program_id,
move_stake_and_move_lamports_ixs,
stake_raise_minimum_delegation_to_1_sol,
deprecate_legacy_vote_ixs,
mask_out_rent_epoch_in_vm_serialization,
simplify_alt_bn128_syscall_error_codes,
fix_alt_bn128_multiplication_input_length,
loosen_cpi_size_restriction,
increase_tx_account_lock_limit,
enable_extend_program_checked,
formalize_loaded_transaction_data_size,
disable_zk_elgamal_proof_program,
reenable_zk_elgamal_proof_program,
raise_cpi_nesting_limit_to_8,
} = svm_features;

activate_if(
move_precompile_verification_to_svm,
move_precompile_verification_to_svm::id(),
);
activate_if(
stricter_abi_and_runtime_constraints,
stricter_abi_and_runtime_constraints::id(),
);
activate_if(
enable_bpf_loader_set_authority_checked_ix,
enable_bpf_loader_set_authority_checked_ix::id(),
);
activate_if(enable_loader_v4, enable_loader_v4::id());
activate_if(
deplete_cu_meter_on_vm_failure,
deplete_cu_meter_on_vm_failure::id(),
);
activate_if(abort_on_invalid_curve, abort_on_invalid_curve::id());
activate_if(blake3_syscall_enabled, blake3_syscall_enabled::id());
activate_if(curve25519_syscall_enabled, curve25519_syscall_enabled::id());
activate_if(
disable_deploy_of_alloc_free_syscall,
disable_deploy_of_alloc_free_syscall::id(),
);
activate_if(disable_fees_sysvar, disable_fees_sysvar::id());
activate_if(disable_sbpf_v0_execution, disable_sbpf_v0_execution::id());
activate_if(
enable_alt_bn128_compression_syscall,
enable_alt_bn128_compression_syscall::id(),
);
activate_if(enable_alt_bn128_syscall, enable_alt_bn128_syscall::id());
activate_if(enable_big_mod_exp_syscall, enable_big_mod_exp_syscall::id());
activate_if(
enable_get_epoch_stake_syscall,
enable_get_epoch_stake_syscall::id(),
);
activate_if(enable_poseidon_syscall, enable_poseidon_syscall::id());
activate_if(
enable_sbpf_v1_deployment_and_execution,
enable_sbpf_v1_deployment_and_execution::id(),
);
activate_if(
enable_sbpf_v2_deployment_and_execution,
enable_sbpf_v2_deployment_and_execution::id(),
);
activate_if(
enable_sbpf_v3_deployment_and_execution,
enable_sbpf_v3_deployment_and_execution::id(),
);
activate_if(get_sysvar_syscall_enabled, get_sysvar_syscall_enabled::id());
activate_if(last_restart_slot_sysvar, last_restart_slot_sysvar::id());
activate_if(reenable_sbpf_v0_execution, reenable_sbpf_v0_execution::id());
activate_if(
remaining_compute_units_syscall_enabled,
remaining_compute_units_syscall_enabled::id(),
);
activate_if(
remove_bpf_loader_incorrect_program_id,
remove_bpf_loader_incorrect_program_id::id(),
);
activate_if(
move_stake_and_move_lamports_ixs,
move_stake_and_move_lamports_ixs::id(),
);
activate_if(
stake_raise_minimum_delegation_to_1_sol,
stake_raise_minimum_delegation_to_1_sol::id(),
);
activate_if(deprecate_legacy_vote_ixs, deprecate_legacy_vote_ixs::id());
activate_if(
mask_out_rent_epoch_in_vm_serialization,
mask_out_rent_epoch_in_vm_serialization::id(),
);
activate_if(
simplify_alt_bn128_syscall_error_codes,
simplify_alt_bn128_syscall_error_codes::id(),
);
activate_if(
fix_alt_bn128_multiplication_input_length,
fix_alt_bn128_multiplication_input_length::id(),
);
activate_if(
loosen_cpi_size_restriction,
loosen_cpi_size_restriction::id(),
);
activate_if(
increase_tx_account_lock_limit,
increase_tx_account_lock_limit::id(),
);
activate_if(
enable_extend_program_checked,
enable_extend_program_checked::id(),
);
activate_if(
formalize_loaded_transaction_data_size,
formalize_loaded_transaction_data_size::id(),
);
activate_if(
disable_zk_elgamal_proof_program,
disable_zk_elgamal_proof_program::id(),
);
activate_if(
reenable_zk_elgamal_proof_program,
reenable_zk_elgamal_proof_program::id(),
);
activate_if(
raise_cpi_nesting_limit_to_8,
raise_cpi_nesting_limit_to_8::id(),
);

Self { active, inactive }
}
}

pub mod deprecate_rewards_sysvar {
solana_pubkey::declare_id!("GaBtBJvmS4Arjj5W1NmFcyvPjsHN38UGYDq2MDwbs9Qu");
}
Expand Down
2 changes: 1 addition & 1 deletion svm-feature-set/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#[derive(Clone, Copy, Default)]
#[derive(Clone, Copy, Debug, Default)]
pub struct SVMFeatureSet {
pub move_precompile_verification_to_svm: bool,
pub stricter_abi_and_runtime_constraints: bool,
Expand Down
1 change: 0 additions & 1 deletion svm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ spl-generic-token = { workspace = true }
thiserror = { workspace = true }

[dev-dependencies]
agave-feature-set = { workspace = true }
agave-reserved-account-keys = { workspace = true }
agave-syscalls = { workspace = true }
assert_matches = { workspace = true }
Expand Down
47 changes: 24 additions & 23 deletions svm/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use {
program_data_size, register_builtins, MockBankCallback, MockForkGraph, EXECUTION_EPOCH,
EXECUTION_SLOT, WALLCLOCK_TIME,
},
agave_feature_set::{self as feature_set, raise_cpi_nesting_limit_to_8, FeatureSet},
solana_account::{AccountSharedData, ReadableAccount, WritableAccount, PROGRAM_OWNERS},
solana_clock::Slot,
solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions,
Expand Down Expand Up @@ -41,6 +40,7 @@ use {
TransactionProcessingEnvironment,
},
},
solana_svm_feature_set::SVMFeatureSet,
solana_svm_transaction::svm_message::SVMMessage,
solana_svm_type_overrides::sync::{Arc, RwLock},
solana_system_interface::{instruction as system_instruction, program as system_program},
Expand Down Expand Up @@ -114,10 +114,9 @@ impl SvmTestEnvironment<'_> {
..Default::default()
};

let feature_set = test_entry.feature_set();
let processing_environment = TransactionProcessingEnvironment {
blockhash: LAST_BLOCKHASH,
feature_set: feature_set.runtime_features(),
feature_set: test_entry.feature_set,
blockhash_lamports_per_signature: LAMPORTS_PER_SIGNATURE,
..TransactionProcessingEnvironment::default()
};
Expand Down Expand Up @@ -296,10 +295,10 @@ impl SvmTestEnvironment<'_> {
}

// container for a transaction batch and all data needed to run and verify it against svm
#[derive(Clone, Default, Debug)]
#[derive(Clone, Debug)]
pub struct SvmTestEntry {
// features are enabled by default; these will be disabled
pub disabled_features: Vec<Pubkey>,
// SVM feature set for this test.
pub feature_set: SVMFeatureSet,

// until LoaderV4 is live on mainnet, we default to omitting it, but can also test it
pub with_loader_v4: bool,
Expand All @@ -317,6 +316,19 @@ pub struct SvmTestEntry {
pub final_accounts: AccountsMap,
}

impl Default for SvmTestEntry {
fn default() -> Self {
Self {
feature_set: SVMFeatureSet::all_enabled(),
with_loader_v4: false,
initial_programs: Vec::new(),
initial_accounts: AccountsMap::default(),
transaction_batch: Vec::new(),
final_accounts: AccountsMap::default(),
}
}
}

impl SvmTestEntry {
pub fn with_loader_v4() -> Self {
Self {
Expand Down Expand Up @@ -450,7 +462,7 @@ impl SvmTestEntry {
let check_result = item.check_result.map(|tx_details| {
let compute_budget_limits = process_compute_budget_instructions(
SVMMessage::program_instructions_iter(&message),
&self.feature_set(),
&(&self.feature_set).into(),
);
let signature_count = message
.num_transaction_signatures()
Expand All @@ -465,8 +477,7 @@ impl SvmTestEntry {
signature_count.saturating_mul(LAMPORTS_PER_SIGNATURE),
v.get_prioritization_fee(),
),
self.feature_set()
.is_active(&raise_cpi_nesting_limit_to_8::id()),
self.feature_set.raise_cpi_nesting_limit_to_8,
)
});
CheckedTransactionDetails::new(tx_details.nonce, compute_budget)
Expand All @@ -485,16 +496,6 @@ impl SvmTestEntry {
.map(|item| item.asserts)
.collect()
}

// internal helper to map our feature list to a FeatureSet
fn feature_set(&self) -> FeatureSet {
let mut feature_set = FeatureSet::all_enabled();
for feature_id in &self.disabled_features {
feature_set.deactivate(feature_id);
}

feature_set
}
}

// one transaction in a batch plus check results for svm and asserts for tests
Expand Down Expand Up @@ -2204,8 +2205,8 @@ fn simd83_account_reallocate(formalize_loaded_transaction_data_size: bool) -> Ve
common_test_entry.add_initial_program(program_name);
if !formalize_loaded_transaction_data_size {
common_test_entry
.disabled_features
.push(feature_set::formalize_loaded_transaction_data_size::id());
.feature_set
.formalize_loaded_transaction_data_size = false;
}

let fee_payer_keypair = Keypair::new();
Expand Down Expand Up @@ -2910,8 +2911,8 @@ fn svm_inspect_nonce_load_failure(

if !formalize_loaded_transaction_data_size {
test_entry
.disabled_features
.push(feature_set::formalize_loaded_transaction_data_size::id());
.feature_set
.formalize_loaded_transaction_data_size = false;
}

let fee_payer_keypair = Keypair::new();
Expand Down
Loading