Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Fix compute budget bump #21238

Merged
merged 2 commits into from
Nov 11, 2021
Merged

Fix compute budget bump #21238

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
5089ed6
Fix compute budget bump
jackcmay Nov 10, 2021
3909e59
feedback
jackcmay Nov 11, 2021
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
53 changes: 25 additions & 28 deletions program-runtime/src/instruction_processor.rs
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -362,38 +362,35 @@ impl InstructionProcessor {
invoke_context: &mut dyn InvokeContext, invoke_context: &mut dyn InvokeContext,
) -> Result<(), InstructionError> { ) -> Result<(), InstructionError> {
let keyed_accounts = invoke_context.get_keyed_accounts()?; let keyed_accounts = invoke_context.get_keyed_accounts()?;
if let Ok(root_account) = keyed_account_at_index(keyed_accounts, 0) { let root_account = keyed_account_at_index(keyed_accounts, 0)?;
let root_id = root_account.unsigned_key(); let root_id = root_account.unsigned_key();
let owner_id = &root_account.owner()?; let owner_id = &root_account.owner()?;
if solana_sdk::native_loader::check_id(owner_id) { if solana_sdk::native_loader::check_id(owner_id) {
for (id, process_instruction) in &self.programs { for (id, process_instruction) in &self.programs {
if id == root_id { if id == root_id {
// Call the builtin program // Call the builtin program
return process_instruction( return process_instruction(
1, // root_id to be skipped 1, // root_id to be skipped
instruction_data,
invoke_context,
);
}
}
if !invoke_context.is_feature_active(&remove_native_loader::id()) {
// Call the program via the native loader
return self.native_loader.process_instruction(
0,
instruction_data, instruction_data,
invoke_context, invoke_context,
); );
} }
} else { }
for (id, process_instruction) in &self.programs { if !invoke_context.is_feature_active(&remove_native_loader::id()) {
if id == owner_id { // Call the program via the native loader
// Call the program via a builtin loader return self
return process_instruction( .native_loader
0, // no root_id was provided .process_instruction(0, instruction_data, invoke_context);
instruction_data, }
invoke_context, } else {
); for (id, process_instruction) in &self.programs {
} if id == owner_id {
// Call the program via a builtin loader
return process_instruction(
0, // no root_id was provided
instruction_data,
invoke_context,
);
} }
} }
} }
Expand Down
108 changes: 102 additions & 6 deletions program-runtime/src/invoke_context.rs
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use solana_sdk::{
account::{AccountSharedData, ReadableAccount}, account::{AccountSharedData, ReadableAccount},
compute_budget::ComputeBudget, compute_budget::ComputeBudget,
feature_set::{ feature_set::{
demote_program_write_locks, do_support_realloc, remove_native_loader, tx_wide_compute_cap, demote_program_write_locks, do_support_realloc, neon_evm_compute_budget,
FeatureSet, remove_native_loader, requestable_heap_size, tx_wide_compute_cap, FeatureSet,
}, },
hash::Hash, hash::Hash,
ic_logger_msg, ic_msg, ic_logger_msg, ic_msg,
Expand Down Expand Up @@ -103,6 +103,7 @@ pub struct ThisInvokeContext<'a> {
sysvars: &'a [(Pubkey, Vec<u8>)], sysvars: &'a [(Pubkey, Vec<u8>)],
logger: Rc<RefCell<dyn Logger>>, logger: Rc<RefCell<dyn Logger>>,
compute_budget: ComputeBudget, compute_budget: ComputeBudget,
current_compute_budget: ComputeBudget,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is current_compute_budget and how is it different from compute_budget ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the bump is only for a single instruction compute_budget which is the original is used to reset current_compute_budget for additional instructions that don't meet the bump criteria.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no further concerns I'll commit it

compute_meter: Rc<RefCell<dyn ComputeMeter>>, compute_meter: Rc<RefCell<dyn ComputeMeter>>,
executors: Rc<RefCell<Executors>>, executors: Rc<RefCell<Executors>>,
instruction_recorders: Option<&'a [InstructionRecorder]>, instruction_recorders: Option<&'a [InstructionRecorder]>,
Expand Down Expand Up @@ -137,6 +138,7 @@ impl<'a> ThisInvokeContext<'a> {
programs, programs,
sysvars, sysvars,
logger: ThisLogger::new_ref(log_collector), logger: ThisLogger::new_ref(log_collector),
current_compute_budget: compute_budget,
compute_budget, compute_budget,
compute_meter, compute_meter,
executors, executors,
Expand Down Expand Up @@ -195,7 +197,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> {
return Err(InstructionError::CallDepth); return Err(InstructionError::CallDepth);
} }


if let Some(index_of_program_id) = program_indices.last() { let program_id = if let Some(index_of_program_id) = program_indices.last() {
let program_id = &self.accounts[*index_of_program_id].0; let program_id = &self.accounts[*index_of_program_id].0;
let contains = self let contains = self
.invoke_stack .invoke_stack
Expand All @@ -210,11 +212,32 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> {
// Reentrancy not allowed unless caller is calling itself // Reentrancy not allowed unless caller is calling itself
return Err(InstructionError::ReentrancyNotAllowed); return Err(InstructionError::ReentrancyNotAllowed);
} }
} *program_id
} else {
return Err(InstructionError::UnsupportedProgramId);
Copy link
Contributor

Choose a reason for hiding this comment

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

@jackcmay @Lichtso it looks like this new err has made master and v1.8 incompatible. On v1.8 it's possible to invoke the native loader directly from a tx in which case program_indices is empty and the tx can still be processed successfully.

Here is an example tx that showcases the different behavior. The tx invokes the native loader in a way that runs the code path for a 0 lamport system ix transfer with the program_id set to the native loader id: https://explorer.solana.com/tx/inspector?message=AQABAxqaPUT28ZZYeGqKkcOfzL7aLBsifPzASKGnxP4QwCbm%252Bk%252BRkBm4gzLzoCATvtlllmIDwh%252B8XErqP9gYsdFvpjUFh4S%252FFIukKC%252BwEldIiKnxU6B9rfdlwEVcmpcDgAAAAL1QHO0HDnEf1DbxkmiM2jg37QuLnqbQDF9g5Y7HZgN3AQICAAEMAgAAAAAAAAAAAAAA

It was constructed with the following JS:

    // create this to copy out the ix data    
    const ix = SystemProgram.transfer({
        fromPubkey: MY_KEY.publicKey,
        toPubkey: NEW_KEY.publicKey,
        lamports: 0,
    });
    
    const tx = new Transaction().add(new TransactionInstruction({
        programId: new PublicKey("NativeLoader1111111111111111111111111111111"),
        data: ix.data,
        keys: [{
            pubkey: MY_KEY.publicKey,
            isSigner: false,
            isWritable: false,
        }, {
            pubkey: NEW_KEY.publicKey,
            isSigner: false,
            isWritable: true,
        }],
    }));

Copy link
Contributor

@Lichtso Lichtso Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned here I think we need to either add an empty transaction to the test corpus or feature gate and forbid them. Because they are breaking the assumption that there is at least one executable account loaded to provide a program_id.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of feature gating and forbidding them. But we will still need to revert behavior and add a test case for this in the meantime until we enable that feature switch.

Copy link
Contributor

@Lichtso Lichtso Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed in both, the temporary test and the plan to remove them.
It also makes life a little easier in terms of edge cases which we need to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change that are causing this behavior were an optimization to get the program_id in push. We don't have to do that and instead allow empty indices and get the program id from the message instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are tracking this in a separate issue: #21346

};


if self.invoke_stack.is_empty() { if self.invoke_stack.is_empty() {
let mut compute_budget = self.compute_budget;
if !self.is_feature_active(&tx_wide_compute_cap::id())
&& self.is_feature_active(&neon_evm_compute_budget::id())
&& program_id == crate::neon_evm_program::id()
{
// Bump the compute budget for neon_evm
compute_budget.max_units = compute_budget.max_units.max(500_000);
}
if !self.is_feature_active(&requestable_heap_size::id())
&& self.is_feature_active(&neon_evm_compute_budget::id())
&& program_id == crate::neon_evm_program::id()
{
// Bump the compute budget for neon_evm
compute_budget.heap_size = Some(256_usize.saturating_mul(1024));
}
self.current_compute_budget = compute_budget;

if !self.feature_set.is_active(&tx_wide_compute_cap::id()) { if !self.feature_set.is_active(&tx_wide_compute_cap::id()) {
self.compute_meter = ThisComputeMeter::new_ref(self.compute_budget.max_units); self.compute_meter =
ThisComputeMeter::new_ref(self.current_compute_budget.max_units);
} }


self.pre_accounts = Vec::with_capacity(instruction.accounts.len()); self.pre_accounts = Vec::with_capacity(instruction.accounts.len());
Expand Down Expand Up @@ -484,7 +507,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> {
self.sysvars self.sysvars
} }
fn get_compute_budget(&self) -> &ComputeBudget { fn get_compute_budget(&self) -> &ComputeBudget {
&self.compute_budget &self.current_compute_budget
} }
fn set_blockhash(&mut self, hash: Hash) { fn set_blockhash(&mut self, hash: Hash) {
self.blockhash = hash; self.blockhash = hash;
Expand Down Expand Up @@ -1087,4 +1110,77 @@ mod tests {
invoke_context.pop(); invoke_context.pop();
} }
} }

#[test]
fn test_invoke_context_compute_budget() {
let accounts = vec![
(
solana_sdk::pubkey::new_rand(),
Rc::new(RefCell::new(AccountSharedData::default())),
),
(
crate::neon_evm_program::id(),
Rc::new(RefCell::new(AccountSharedData::default())),
),
];

let noop_message = Message::new(
&[Instruction::new_with_bincode(
accounts[0].0,
&MockInstruction::NoopSuccess,
vec![AccountMeta::new_readonly(accounts[0].0, false)],
)],
None,
);
let neon_message = Message::new(
&[Instruction::new_with_bincode(
crate::neon_evm_program::id(),
&MockInstruction::NoopSuccess,
vec![AccountMeta::new_readonly(accounts[0].0, false)],
)],
None,
);

let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&tx_wide_compute_cap::id());
feature_set.deactivate(&requestable_heap_size::id());
let mut invoke_context = ThisInvokeContext::new_mock_with_sysvars_and_features(
&accounts,
&[],
&[],
Arc::new(feature_set),
);

invoke_context
.push(&noop_message, &noop_message.instructions[0], &[0], None)
.unwrap();
assert_eq!(
*invoke_context.get_compute_budget(),
ComputeBudget::default()
);
invoke_context.pop();

invoke_context
.push(&neon_message, &neon_message.instructions[0], &[1], None)
.unwrap();
let expected_compute_budget = ComputeBudget {
max_units: 500_000,
heap_size: Some(256_usize.saturating_mul(1024)),
..ComputeBudget::default()
};
assert_eq!(
*invoke_context.get_compute_budget(),
expected_compute_budget
);
invoke_context.pop();

invoke_context
.push(&noop_message, &noop_message.instructions[0], &[0], None)
.unwrap();
assert_eq!(
*invoke_context.get_compute_budget(),
ComputeBudget::default()
);
invoke_context.pop();
}
} }
1 change: 1 addition & 0 deletions program-runtime/src/lib.rs
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ pub mod instruction_recorder;
pub mod invoke_context; pub mod invoke_context;
pub mod log_collector; pub mod log_collector;
pub mod native_loader; pub mod native_loader;
pub mod neon_evm_program;
0 runtime/src/neon_evm_program.rs → program-runtime/src/neon_evm_program.rs
View file
Open in desktop
File renamed without changes.
1 change: 0 additions & 1 deletion runtime/src/lib.rs
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ pub mod in_mem_accounts_index;
pub mod inline_spl_token_v2_0; pub mod inline_spl_token_v2_0;
pub mod loader_utils; pub mod loader_utils;
pub mod message_processor; pub mod message_processor;
pub mod neon_evm_program;
pub mod non_circulating_supply; pub mod non_circulating_supply;
mod nonce_keyed_account; mod nonce_keyed_account;
mod pubkey_bins; mod pubkey_bins;
Expand Down
21 changes: 1 addition & 20 deletions runtime/src/message_processor.rs
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@ use solana_program_runtime::{
use solana_sdk::{ use solana_sdk::{
account::{AccountSharedData, WritableAccount}, account::{AccountSharedData, WritableAccount},
compute_budget::ComputeBudget, compute_budget::ComputeBudget,
feature_set::{ feature_set::{prevent_calling_precompiles_as_programs, FeatureSet},
neon_evm_compute_budget, prevent_calling_precompiles_as_programs, requestable_heap_size,
tx_wide_compute_cap, FeatureSet,
},
hash::Hash, hash::Hash,
message::Message, message::Message,
precompiles::is_precompile, precompiles::is_precompile,
Expand Down Expand Up @@ -107,22 +104,6 @@ impl MessageProcessor {
} }
} }


let mut compute_budget = compute_budget;
if !invoke_context.is_feature_active(&tx_wide_compute_cap::id())
&& invoke_context.is_feature_active(&neon_evm_compute_budget::id())
&& *program_id == crate::neon_evm_program::id()
{
// Bump the compute budget for neon_evm
compute_budget.max_units = compute_budget.max_units.max(500_000);
}
if !invoke_context.is_feature_active(&requestable_heap_size::id())
&& invoke_context.is_feature_active(&neon_evm_compute_budget::id())
&& *program_id == crate::neon_evm_program::id()
{
// Bump the compute budget for neon_evm
compute_budget.heap_size = Some(256 * 1024);
}

invoke_context.set_instruction_index(instruction_index); invoke_context.set_instruction_index(instruction_index);
let result = invoke_context let result = invoke_context
.push(message, instruction, program_indices, None) .push(message, instruction, program_indices, None)
Expand Down
24 changes: 24 additions & 0 deletions sdk/src/feature_set.rs
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -373,6 +373,18 @@ impl FeatureSet {
inactive: HashSet::new(), inactive: HashSet::new(),
} }
} }

/// Activate a feature
pub fn activate(&mut self, feature_id: &Pubkey, slot: u64) {
self.inactive.remove(feature_id);
self.active.insert(*feature_id, slot);
}

/// Deactivate a feature
pub fn deactivate(&mut self, feature_id: &Pubkey) {
self.active.remove(feature_id);
self.inactive.insert(*feature_id);
}
} }


#[cfg(test)] #[cfg(test)]
Expand Down Expand Up @@ -433,4 +445,16 @@ mod test {
.collect() .collect()
); );
} }

#[test]
fn test_feature_set_activate_deactivate() {
let mut feature_set = FeatureSet::default();

let feature = Pubkey::new_unique();
assert!(!feature_set.is_active(&feature));
feature_set.activate(&feature, 0);
assert!(feature_set.is_active(&feature));
feature_set.deactivate(&feature);
assert!(!feature_set.is_active(&feature));
}
} }