Skip to content

Commit

Permalink
Revert deprecate executable feature (anza-xyz#309)
Browse files Browse the repository at this point in the history 
* revert deprecate executable feature

* add native loader account transfer test

---------

Co-authored-by: HaoranYi <[email protected]>
  • Loading branch information
@HaoranYi @willhickey
2 people authored and willhickey committed Mar 20, 2024
1 parent 78f033d commit 3043334
Show file tree
Hide file tree
Showing 26 changed files with 353 additions and 729 deletions.
15 changes: 3 additions & 12 deletions cli/src/program.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use {
},
solana_rpc_client_nonce_utils::blockhash_query::BlockhashQuery,
solana_sdk::{
account::{is_executable, Account},
account::Account,
account_utils::StateMut,
bpf_loader, bpf_loader_deprecated,
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
Expand Down Expand Up @@ -1066,15 +1066,6 @@ fn get_default_program_keypair(program_location: &Option<String>) -> Keypair {
program_keypair
}

fn is_account_executable(account: &Account) -> bool {
if account.owner == bpf_loader_deprecated::id() || account.owner == bpf_loader::id() {
account.executable
} else {
let feature_set = FeatureSet::all_enabled();
is_executable(account, &feature_set)
}
}

/// Deploy program using upgradeable loader. It also can process program upgrades
#[allow(clippy::too_many_arguments)]
fn process_program_deploy(
Expand Down Expand Up @@ -1131,7 +1122,7 @@ fn process_program_deploy(
.into());
}

if !is_account_executable(&account) {
if !account.executable {
// Continue an initial deploy
true
} else if let Ok(UpgradeableLoaderState::Program {
Expand Down Expand Up @@ -2534,7 +2525,7 @@ fn complete_partial_program_init(
) -> Result<(Vec<Instruction>, u64), Box<dyn std::error::Error>> {
let mut instructions: Vec<Instruction> = vec![];
let mut balance_needed = 0;
if is_account_executable(account) {
if account.executable {
return Err("Buffer account is already executable".into());
}
if account.owner != *loader_id && !system_program::check_id(&account.owner) {
Expand Down
44 changes: 12 additions & 32 deletions cli/tests/program.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,9 @@ use {
solana_rpc_client::rpc_client::RpcClient,
solana_rpc_client_nonce_utils::blockhash_query::BlockhashQuery,
solana_sdk::{
account::is_executable,
account_utils::StateMut,
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
commitment_config::CommitmentConfig,
feature_set::FeatureSet,
pubkey::Pubkey,
signature::{Keypair, NullSigner, Signer},
},
Expand Down Expand Up @@ -102,7 +100,7 @@ fn test_cli_program_deploy_non_upgradeable() {
let account0 = rpc_client.get_account(&program_id).unwrap();
assert_eq!(account0.lamports, minimum_balance_for_program);
assert_eq!(account0.owner, bpf_loader_upgradeable::id());
assert!(is_executable(&account0, &FeatureSet::all_enabled()));
assert!(account0.executable);

let (programdata_pubkey, _) =
Pubkey::find_program_address(&[program_id.as_ref()], &bpf_loader_upgradeable::id());
Expand All @@ -112,10 +110,7 @@ fn test_cli_program_deploy_non_upgradeable() {
minimum_balance_for_programdata
);
assert_eq!(programdata_account.owner, bpf_loader_upgradeable::id());
assert!(!is_executable(
&programdata_account,
&FeatureSet::all_enabled()
));
assert!(!programdata_account.executable);
assert_eq!(
programdata_account.data[UpgradeableLoaderState::size_of_programdata_metadata()..],
program_data[..]
Expand Down Expand Up @@ -143,7 +138,7 @@ fn test_cli_program_deploy_non_upgradeable() {
.unwrap();
assert_eq!(account1.lamports, minimum_balance_for_program);
assert_eq!(account1.owner, bpf_loader_upgradeable::id());
assert!(is_executable(&account1, &FeatureSet::all_enabled()));
assert!(account1.executable);
let (programdata_pubkey, _) = Pubkey::find_program_address(
&[custom_address_keypair.pubkey().as_ref()],
&bpf_loader_upgradeable::id(),
Expand All @@ -154,10 +149,7 @@ fn test_cli_program_deploy_non_upgradeable() {
minimum_balance_for_programdata
);
assert_eq!(programdata_account.owner, bpf_loader_upgradeable::id());
assert!(!is_executable(
&programdata_account,
&FeatureSet::all_enabled()
));
assert!(!programdata_account.executable);
assert_eq!(
programdata_account.data[UpgradeableLoaderState::size_of_programdata_metadata()..],
program_data[..]
Expand Down Expand Up @@ -385,7 +377,7 @@ fn test_cli_program_deploy_with_authority() {
let program_account = rpc_client.get_account(&program_keypair.pubkey()).unwrap();
assert_eq!(program_account.lamports, minimum_balance_for_program);
assert_eq!(program_account.owner, bpf_loader_upgradeable::id());
assert!(is_executable(&program_account, &FeatureSet::all_enabled()));
assert!(program_account.executable);
let (programdata_pubkey, _) = Pubkey::find_program_address(
&[program_keypair.pubkey().as_ref()],
&bpf_loader_upgradeable::id(),
Expand All @@ -396,10 +388,7 @@ fn test_cli_program_deploy_with_authority() {
minimum_balance_for_programdata
);
assert_eq!(programdata_account.owner, bpf_loader_upgradeable::id());
assert!(!is_executable(
&programdata_account,
&FeatureSet::all_enabled()
));
assert!(!programdata_account.executable);
assert_eq!(
programdata_account.data[UpgradeableLoaderState::size_of_programdata_metadata()..],
program_data[..]
Expand Down Expand Up @@ -433,7 +422,7 @@ fn test_cli_program_deploy_with_authority() {
let program_account = rpc_client.get_account(&program_pubkey).unwrap();
assert_eq!(program_account.lamports, minimum_balance_for_program);
assert_eq!(program_account.owner, bpf_loader_upgradeable::id());
assert!(is_executable(&program_account, &FeatureSet::all_enabled()));
assert!(program_account.executable);
let (programdata_pubkey, _) =
Pubkey::find_program_address(&[program_pubkey.as_ref()], &bpf_loader_upgradeable::id());
let programdata_account = rpc_client.get_account(&programdata_pubkey).unwrap();
Expand All @@ -442,10 +431,7 @@ fn test_cli_program_deploy_with_authority() {
minimum_balance_for_programdata
);
assert_eq!(programdata_account.owner, bpf_loader_upgradeable::id());
assert!(!is_executable(
&programdata_account,
&FeatureSet::all_enabled()
));
assert!(program_account.executable);
assert_eq!(
programdata_account.data[UpgradeableLoaderState::size_of_programdata_metadata()..],
program_data[..]
Expand All @@ -470,7 +456,7 @@ fn test_cli_program_deploy_with_authority() {
let program_account = rpc_client.get_account(&program_pubkey).unwrap();
assert_eq!(program_account.lamports, minimum_balance_for_program);
assert_eq!(program_account.owner, bpf_loader_upgradeable::id());
assert!(is_executable(&program_account, &FeatureSet::all_enabled()));
assert!(program_account.executable);
let (programdata_pubkey, _) =
Pubkey::find_program_address(&[program_pubkey.as_ref()], &bpf_loader_upgradeable::id());
let programdata_account = rpc_client.get_account(&programdata_pubkey).unwrap();
Expand All @@ -479,10 +465,7 @@ fn test_cli_program_deploy_with_authority() {
minimum_balance_for_programdata
);
assert_eq!(programdata_account.owner, bpf_loader_upgradeable::id());
assert!(!is_executable(
&programdata_account,
&FeatureSet::all_enabled()
));
assert!(program_account.executable);
assert_eq!(
programdata_account.data[UpgradeableLoaderState::size_of_programdata_metadata()..],
program_data[..]
Expand Down Expand Up @@ -548,7 +531,7 @@ fn test_cli_program_deploy_with_authority() {
let program_account = rpc_client.get_account(&program_pubkey).unwrap();
assert_eq!(program_account.lamports, minimum_balance_for_program);
assert_eq!(program_account.owner, bpf_loader_upgradeable::id());
assert!(is_executable(&program_account, &FeatureSet::all_enabled()));
assert!(program_account.executable);
let (programdata_pubkey, _) =
Pubkey::find_program_address(&[program_pubkey.as_ref()], &bpf_loader_upgradeable::id());
let programdata_account = rpc_client.get_account(&programdata_pubkey).unwrap();
Expand All @@ -557,10 +540,7 @@ fn test_cli_program_deploy_with_authority() {
minimum_balance_for_programdata
);
assert_eq!(programdata_account.owner, bpf_loader_upgradeable::id());
assert!(!is_executable(
&programdata_account,
&FeatureSet::all_enabled()
));
assert!(program_account.executable);
assert_eq!(
programdata_account.data[UpgradeableLoaderState::size_of_programdata_metadata()..],
program_data[..]
Expand Down
1 change: 0 additions & 1 deletion ledger-tool/src/program.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,6 @@ pub fn program(ledger_path: &Path, matches: &ArgMatches<'_>) {
.get_current_instruction_context()
.unwrap(),
true, // copy_account_data
&invoke_context.feature_set,
)
.unwrap();

Expand Down
14 changes: 7 additions & 7 deletions program-runtime/src/invoke_context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ impl<'a> InvokeContext<'a> {
})?;
let borrowed_program_account = instruction_context
.try_borrow_instruction_account(self.transaction_context, program_account_index)?;
if !borrowed_program_account.is_executable(&self.feature_set) {
if !borrowed_program_account.is_executable() {
ic_msg!(self, "Account {} is not executable", callee_program_id);
return Err(InstructionError::AccountNotExecutable);
}
Expand Down Expand Up @@ -802,17 +802,17 @@ mod tests {
MockInstruction::NoopFail => return Err(InstructionError::GenericError),
MockInstruction::ModifyOwned => instruction_context
.try_borrow_instruction_account(transaction_context, 0)?
.set_data_from_slice(&[1], &invoke_context.feature_set)?,
.set_data_from_slice(&[1])?,
MockInstruction::ModifyNotOwned => instruction_context
.try_borrow_instruction_account(transaction_context, 1)?
.set_data_from_slice(&[1], &invoke_context.feature_set)?,
.set_data_from_slice(&[1])?,
MockInstruction::ModifyReadonly => instruction_context
.try_borrow_instruction_account(transaction_context, 2)?
.set_data_from_slice(&[1], &invoke_context.feature_set)?,
.set_data_from_slice(&[1])?,
MockInstruction::UnbalancedPush => {
instruction_context
.try_borrow_instruction_account(transaction_context, 0)?
.checked_add_lamports(1, &invoke_context.feature_set)?;
.checked_add_lamports(1)?;
let program_id = *transaction_context.get_key_of_account_at_index(3)?;
let metas = vec![
AccountMeta::new_readonly(
Expand Down Expand Up @@ -843,7 +843,7 @@ mod tests {
}
MockInstruction::UnbalancedPop => instruction_context
.try_borrow_instruction_account(transaction_context, 0)?
.checked_add_lamports(1, &invoke_context.feature_set)?,
.checked_add_lamports(1)?,
MockInstruction::ConsumeComputeUnits {
compute_units_to_consume,
desired_result,
Expand All @@ -855,7 +855,7 @@ mod tests {
}
MockInstruction::Resize { new_len } => instruction_context
.try_borrow_instruction_account(transaction_context, 0)?
.set_data(vec![0; new_len as usize], &invoke_context.feature_set)?,
.set_data(vec![0; new_len as usize])?,
}
} else {
return Err(InstructionError::InvalidInstructionData);
Expand Down
16 changes: 8 additions & 8 deletions program-runtime/src/message_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,16 +221,16 @@ mod tests {
MockSystemInstruction::TransferLamports { lamports } => {
instruction_context
.try_borrow_instruction_account(transaction_context, 0)?
.checked_sub_lamports(lamports, &invoke_context.feature_set)?;
.checked_sub_lamports(lamports)?;
instruction_context
.try_borrow_instruction_account(transaction_context, 1)?
.checked_add_lamports(lamports, &invoke_context.feature_set)?;
.checked_add_lamports(lamports)?;
Ok(())
}
MockSystemInstruction::ChangeData { data } => {
instruction_context
.try_borrow_instruction_account(transaction_context, 1)?
.set_data(vec![data], &invoke_context.feature_set)?;
.set_data(vec![data])?;
Ok(())
}
}
Expand Down Expand Up @@ -444,14 +444,14 @@ mod tests {
MockSystemInstruction::DoWork { lamports, data } => {
let mut dup_account = instruction_context
.try_borrow_instruction_account(transaction_context, 2)?;
dup_account.checked_sub_lamports(lamports, &invoke_context.feature_set)?;
to_account.checked_add_lamports(lamports, &invoke_context.feature_set)?;
dup_account.set_data(vec![data], &invoke_context.feature_set)?;
dup_account.checked_sub_lamports(lamports)?;
to_account.checked_add_lamports(lamports)?;
dup_account.set_data(vec![data])?;
drop(dup_account);
let mut from_account = instruction_context
.try_borrow_instruction_account(transaction_context, 0)?;
from_account.checked_sub_lamports(lamports, &invoke_context.feature_set)?;
to_account.checked_add_lamports(lamports, &invoke_context.feature_set)?;
from_account.checked_sub_lamports(lamports)?;
to_account.checked_add_lamports(lamports)?;
Ok(())
}
}
Expand Down
24 changes: 8 additions & 16 deletions program-test/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ pub fn invoke_builtin_function(
.transaction_context
.get_current_instruction_context()?,
true, // copy_account_data // There is no VM so direct mapping can not be implemented here
&invoke_context.feature_set,
)?;

// Deserialize data back into instruction params
Expand Down Expand Up @@ -164,25 +163,18 @@ pub fn invoke_builtin_function(
if borrowed_account.is_writable() {
if let Some(account_info) = account_info_map.get(borrowed_account.get_key()) {
if borrowed_account.get_lamports() != account_info.lamports() {
borrowed_account
.set_lamports(account_info.lamports(), &invoke_context.feature_set)?;
borrowed_account.set_lamports(account_info.lamports())?;
}

if borrowed_account
.can_data_be_resized(account_info.data_len())
.is_ok()
&& borrowed_account
.can_data_be_changed(&invoke_context.feature_set)
.is_ok()
&& borrowed_account.can_data_be_changed().is_ok()
{
borrowed_account.set_data_from_slice(
&account_info.data.borrow(),
&invoke_context.feature_set,
)?;
borrowed_account.set_data_from_slice(&account_info.data.borrow())?;
}
if borrowed_account.get_owner() != account_info.owner {
borrowed_account
.set_owner(account_info.owner.as_ref(), &invoke_context.feature_set)?;
borrowed_account.set_owner(account_info.owner.as_ref())?;
}
}
}
Expand Down Expand Up @@ -293,17 +285,17 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
.unwrap();
if borrowed_account.get_lamports() != account_info.lamports() {
borrowed_account
.set_lamports(account_info.lamports(), &invoke_context.feature_set)
.set_lamports(account_info.lamports())
.unwrap();
}
let account_info_data = account_info.try_borrow_data().unwrap();
// The redundant check helps to avoid the expensive data comparison if we can
match borrowed_account
.can_data_be_resized(account_info_data.len())
.and_then(|_| borrowed_account.can_data_be_changed(&invoke_context.feature_set))
.and_then(|_| borrowed_account.can_data_be_changed())
{
Ok(()) => borrowed_account
.set_data_from_slice(&account_info_data, &invoke_context.feature_set)
.set_data_from_slice(&account_info_data)
.unwrap(),
Err(err) if borrowed_account.get_data() != *account_info_data => {
panic!("{err:?}");
Expand All @@ -313,7 +305,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
// Change the owner at the end so that we are allowed to change the lamports and data before
if borrowed_account.get_owner() != account_info.owner {
borrowed_account
.set_owner(account_info.owner.as_ref(), &invoke_context.feature_set)
.set_owner(account_info.owner.as_ref())
.unwrap();
}
if instruction_account.is_writable {
Expand Down
Loading

0 comments on commit 3043334

Please sign in to comment.