Skip to content

Commit

Permalink
Feature gate builtin consumes static units during processing instruct…
Browse files Browse the repository at this point in the history
…ion (solana-labs#30702)

* add feature gate
* builtins consume statically defined units at beginning of process_instruction()
* Add new instructionError; return error if builtin did not consume units to enforce builtin to consume units;
* updated related tests
* updated ProgramTest with deactivated native_programs_consume_cu feature to continue support existing mock/test programs that do not consume units
  • Loading branch information
tao-stones authored Mar 24, 2023
1 parent 5a05e9b commit 3e500d9
Show file tree
Hide file tree
Showing 20 changed files with 204 additions and 31 deletions.
26 changes: 23 additions & 3 deletions program-runtime/src/invoke_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ use {
solana_sdk::{
account::{AccountSharedData, ReadableAccount},
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
feature_set::{enable_early_verification_of_account_modifications, FeatureSet},
feature_set::{
enable_early_verification_of_account_modifications, native_programs_consume_cu,
FeatureSet,
},
hash::Hash,
instruction::{AccountMeta, InstructionError},
native_loader,
Expand Down Expand Up @@ -751,8 +754,9 @@ impl<'a> InvokeContext<'a> {
self.transaction_context
.set_return_data(program_id, Vec::new())?;

let is_builtin_program = builtin_id == program_id;
let pre_remaining_units = self.get_remaining();
let result = if builtin_id == program_id {
let result = if is_builtin_program {
let logger = self.get_log_collector();
stable_log::program_invoke(&logger, &program_id, self.get_stack_height());
(entry.process_instruction)(self)
Expand All @@ -769,6 +773,15 @@ impl<'a> InvokeContext<'a> {
let post_remaining_units = self.get_remaining();
*compute_units_consumed = pre_remaining_units.saturating_sub(post_remaining_units);

if is_builtin_program
&& *compute_units_consumed == 0
&& self
.feature_set
.is_active(&native_programs_consume_cu::id())
{
return Err(InstructionError::BuiltinProgramsMustConsumeComputeUnits);
}

process_executable_chain_time.stop();
saturating_add_assign!(
timings
Expand Down Expand Up @@ -1082,6 +1095,8 @@ mod tests {
assert!(!format!("{builtin_programs:?}").is_empty());
}

const MOCK_BUILTIN_COMPUTE_UNIT_COST: u64 = 1;

#[allow(clippy::integer_arithmetic)]
fn mock_process_instruction(
invoke_context: &mut InvokeContext,
Expand All @@ -1090,6 +1105,8 @@ mod tests {
let instruction_context = transaction_context.get_current_instruction_context()?;
let instruction_data = instruction_context.get_instruction_data();
let program_id = instruction_context.get_last_program_key(transaction_context)?;
// mock builtin must consume units
invoke_context.consume_checked(MOCK_BUILTIN_COMPUTE_UNIT_COST)?;
let instruction_accounts = (0..4)
.map(|instruction_account_index| InstructionAccount {
index_in_transaction: instruction_account_index,
Expand Down Expand Up @@ -1372,7 +1389,10 @@ mod tests {
// the number of compute units consumed should be a non-default which is something greater
// than zero.
assert!(compute_units_consumed > 0);
assert_eq!(compute_units_consumed, compute_units_to_consume);
assert_eq!(
compute_units_consumed,
compute_units_to_consume + MOCK_BUILTIN_COMPUTE_UNIT_COST
);
assert_eq!(result, expected_result);

invoke_context.pop().unwrap();
Expand Down
7 changes: 6 additions & 1 deletion program-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,13 +463,18 @@ impl Default for ProgramTest {
let prefer_bpf =
std::env::var("BPF_OUT_DIR").is_ok() || std::env::var("SBF_OUT_DIR").is_ok();

// deactivate feature `native_program_consume_cu` to continue support existing mock/test
// programs that do not consume units.
let deactivate_feature_set =
HashSet::from([solana_sdk::feature_set::native_programs_consume_cu::id()]);

Self {
accounts: vec![],
builtins: vec![],
compute_max_units: None,
prefer_bpf,
use_bpf_jit: false,
deactivate_feature_set: HashSet::default(),
deactivate_feature_set,
transaction_account_lock_limit: None,
}
}
Expand Down
7 changes: 7 additions & 0 deletions programs/address-lookup-table/src/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ use {
};

pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), InstructionError> {
// Consume compute units if feature `native_programs_consume_cu` is activated,
if invoke_context
.feature_set
.is_active(&feature_set::native_programs_consume_cu::id())
{
invoke_context.consume_checked(750)?;
}
let transaction_context = &invoke_context.transaction_context;
let instruction_context = transaction_context.get_current_instruction_context()?;
let instruction_data = instruction_context.get_instruction_data();
Expand Down
18 changes: 16 additions & 2 deletions programs/bpf_loader/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ use {
check_slice_translation_size, delay_visibility_of_program_deployment,
disable_deploy_of_alloc_free_syscall, enable_bpf_loader_extend_program_ix,
enable_bpf_loader_set_authority_checked_ix, enable_program_redeployment_cooldown,
limit_max_instruction_trace_length, remove_bpf_loader_incorrect_program_id,
round_up_heap_size, FeatureSet,
limit_max_instruction_trace_length, native_programs_consume_cu,
remove_bpf_loader_incorrect_program_id, round_up_heap_size, FeatureSet,
},
instruction::{AccountMeta, InstructionError},
loader_instruction::LoaderInstruction,
Expand Down Expand Up @@ -520,15 +520,29 @@ fn process_instruction_common(
let program_account =
instruction_context.try_borrow_last_program_account(transaction_context)?;

// Consume compute units if feature `native_programs_consume_cu` is activated
let native_programs_consume_cu = invoke_context
.feature_set
.is_active(&native_programs_consume_cu::id());

// Program Management Instruction
if native_loader::check_id(program_account.get_owner()) {
drop(program_account);
let program_id = instruction_context.get_last_program_key(transaction_context)?;
return if bpf_loader_upgradeable::check_id(program_id) {
if native_programs_consume_cu {
invoke_context.consume_checked(2_370)?;
}
process_loader_upgradeable_instruction(invoke_context, use_jit)
} else if bpf_loader::check_id(program_id) {
if native_programs_consume_cu {
invoke_context.consume_checked(570)?;
}
process_loader_instruction(invoke_context, use_jit)
} else if bpf_loader_deprecated::check_id(program_id) {
if native_programs_consume_cu {
invoke_context.consume_checked(1_140)?;
}
ic_logger_msg!(log_collector, "Deprecated loader is no longer supported");
Err(InstructionError::UnsupportedProgramId)
} else {
Expand Down
12 changes: 10 additions & 2 deletions programs/compute-budget/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
use {
solana_program_runtime::invoke_context::InvokeContext,
solana_sdk::instruction::InstructionError,
solana_sdk::{feature_set, instruction::InstructionError},
};

pub fn process_instruction(_invoke_context: &mut InvokeContext) -> Result<(), InstructionError> {
pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), InstructionError> {
// Consume compute units if feature `native_programs_consume_cu` is activated,
if invoke_context
.feature_set
.is_active(&feature_set::native_programs_consume_cu::id())
{
invoke_context.consume_checked(150)?;
}

// Do nothing, compute budget instructions handled by the runtime
Ok(())
}
8 changes: 8 additions & 0 deletions programs/config/src/config_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), Ins
let instruction_context = transaction_context.get_current_instruction_context()?;
let data = instruction_context.get_instruction_data();

// Consume compute units if feature `native_programs_consume_cu` is activated,
if invoke_context
.feature_set
.is_active(&feature_set::native_programs_consume_cu::id())
{
invoke_context.consume_checked(450)?;
}

let key_list: ConfigKeys = limited_deserialize(data)?;
let config_account_key = transaction_context.get_key_of_account_at_index(
instruction_context.get_index_of_instruction_account_in_transaction(0)?,
Expand Down
8 changes: 7 additions & 1 deletion programs/sbf/tests/programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1470,7 +1470,7 @@ fn test_program_sbf_compute_budget() {
);
let message = Message::new(
&[
ComputeBudgetInstruction::set_compute_unit_limit(1),
ComputeBudgetInstruction::set_compute_unit_limit(150),
Instruction::new_with_bincode(program_id, &0, vec![]),
],
Some(&mint_keypair.pubkey()),
Expand Down Expand Up @@ -2185,7 +2185,13 @@ fn test_program_sbf_disguised_as_sbf_loader() {
mint_keypair,
..
} = create_genesis_config(50);

let mut bank = Bank::new_for_tests(&genesis_config);
// disable native_programs_consume_cu feature to allow test program
// not consume units.
let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&solana_sdk::feature_set::native_programs_consume_cu::id());
bank.feature_set = Arc::new(feature_set);
let (name, id, entrypoint) = solana_bpf_loader_program!();
bank.add_builtin(&name, &id, entrypoint);
bank.deactivate_feature(
Expand Down
8 changes: 8 additions & 0 deletions programs/stake/src/stake_instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), Ins

trace!("process_instruction: {:?}", data);

// Consume compute units if feature `native_programs_consume_cu` is activated,
if invoke_context
.feature_set
.is_active(&feature_set::native_programs_consume_cu::id())
{
invoke_context.consume_checked(750)?;
}

let get_stake_account = || {
let me = instruction_context.try_borrow_instruction_account(transaction_context, 0)?;
if *me.get_owner() != id() {
Expand Down
10 changes: 10 additions & 0 deletions programs/vote/src/vote_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), Ins

trace!("process_instruction: {:?}", data);

// Consume compute units if feature `native_programs_consume_cu` is activated,
// Citing `runtime/src/block_cost_limit.rs`, vote has statically defined 2_100
// units; can consume based on instructions in the future like `bpf_loader` does.
if invoke_context
.feature_set
.is_active(&feature_set::native_programs_consume_cu::id())
{
invoke_context.consume_checked(2_100)?;
}

let mut me = instruction_context.try_borrow_instruction_account(transaction_context, 0)?;
if *me.get_owner() != id() {
return Err(InstructionError::InvalidAccountOwner);
Expand Down
29 changes: 23 additions & 6 deletions programs/zk-token-proof/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use {
bytemuck::Pod,
solana_program_runtime::{ic_msg, invoke_context::InvokeContext},
solana_sdk::{
feature_set,
instruction::{InstructionError, TRANSACTION_LEVEL_STACK_HEIGHT},
system_program,
},
Expand Down Expand Up @@ -119,12 +120,10 @@ pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), Ins
return Err(InstructionError::UnsupportedProgramId);
}

// Consume compute units since proof verification is an expensive operation
{
// TODO: Tune the number of units consumed. The current value is just a rough estimate
invoke_context.consume_checked(100_000)?;
}

// Consume compute units if feature `native_programs_consume_cu` is activated
let native_programs_consume_cu = invoke_context
.feature_set
.is_active(&feature_set::native_programs_consume_cu::id());
let transaction_context = &invoke_context.transaction_context;
let instruction_context = transaction_context.get_current_instruction_context()?;
let instruction_data = instruction_context.get_instruction_data();
Expand All @@ -137,28 +136,46 @@ pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), Ins
process_close_proof_context(invoke_context)
}
ProofInstruction::VerifyCloseAccount => {
if native_programs_consume_cu {
invoke_context.consume_checked(6_012)?;
}
ic_msg!(invoke_context, "VerifyCloseAccount");
process_verify_proof::<CloseAccountData, CloseAccountProofContext>(invoke_context)
}
ProofInstruction::VerifyWithdraw => {
if native_programs_consume_cu {
invoke_context.consume_checked(112_454)?;
}
ic_msg!(invoke_context, "VerifyWithdraw");
process_verify_proof::<WithdrawData, WithdrawProofContext>(invoke_context)
}
ProofInstruction::VerifyWithdrawWithheldTokens => {
if native_programs_consume_cu {
invoke_context.consume_checked(7_943)?;
}
ic_msg!(invoke_context, "VerifyWithdrawWithheldTokens");
process_verify_proof::<WithdrawWithheldTokensData, WithdrawWithheldTokensProofContext>(
invoke_context,
)
}
ProofInstruction::VerifyTransfer => {
if native_programs_consume_cu {
invoke_context.consume_checked(219_290)?;
}
ic_msg!(invoke_context, "VerifyTransfer");
process_verify_proof::<TransferData, TransferProofContext>(invoke_context)
}
ProofInstruction::VerifyTransferWithFee => {
if native_programs_consume_cu {
invoke_context.consume_checked(407_121)?;
}
ic_msg!(invoke_context, "VerifyTransferWithFee");
process_verify_proof::<TransferWithFeeData, TransferWithFeeProofContext>(invoke_context)
}
ProofInstruction::VerifyPubkeyValidity => {
if native_programs_consume_cu {
invoke_context.consume_checked(2_619)?;
}
ic_msg!(invoke_context, "VerifyPubkeyValidity");
process_verify_proof::<PubkeyValidityData, PubkeyValidityProofContext>(invoke_context)
}
Expand Down
10 changes: 5 additions & 5 deletions rpc/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5890,7 +5890,7 @@ pub mod tests {
"Program 11111111111111111111111111111111 success"
],
"returnData":null,
"unitsConsumed":0
"unitsConsumed":150,
}
},
"id": 1,
Expand Down Expand Up @@ -5974,7 +5974,7 @@ pub mod tests {
"Program 11111111111111111111111111111111 success"
],
"returnData":null,
"unitsConsumed":0
"unitsConsumed":150,
}
},
"id": 1,
Expand Down Expand Up @@ -6002,7 +6002,7 @@ pub mod tests {
"Program 11111111111111111111111111111111 success"
],
"returnData":null,
"unitsConsumed":0
"unitsConsumed":150,
}
},
"id": 1,
Expand Down Expand Up @@ -6051,7 +6051,7 @@ pub mod tests {
"accounts":null,
"logs":[],
"returnData":null,
"unitsConsumed":0
"unitsConsumed":0,
}
},
"id":1
Expand Down Expand Up @@ -6080,7 +6080,7 @@ pub mod tests {
"Program 11111111111111111111111111111111 success"
],
"returnData":null,
"unitsConsumed":0
"unitsConsumed":150,
}
},
"id": 1,
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ impl RentDebits {
}

pub type BankStatusCache = StatusCache<Result<()>>;
#[frozen_abi(digest = "3qia1Zm8X66bzFaBuC8ahz3hADRRATyUPRV36ZzrSois")]
#[frozen_abi(digest = "GBTLfFjModD9ykS9LV4pGi4S8eCrUj2JjWSDQLf8tMwV")]
pub type BankSlotDelta = SlotDelta<Result<()>>;

// Eager rent collection repeats in cyclic manner.
Expand Down
Loading

0 comments on commit 3e500d9

Please sign in to comment.