Skip to content

Commit

Permalink
Skip adding builtins if they will be removed
Browse files Browse the repository at this point in the history
  • Loading branch information
@jstarry
jstarry committed Feb 18, 2022
1 parent 1351c1b commit a525b16
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 113 deletions.
11 changes: 4 additions & 7 deletions ledger/src/builtins.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
use {
solana_runtime::builtins::{ActivationType, Builtin, Builtins},
solana_sdk::pubkey::Pubkey,
};
use solana_runtime::builtins::{Builtin, BuiltinFeatureTransition, Builtins};

macro_rules! to_builtin {
($b:expr) => {
Expand Down Expand Up @@ -37,14 +34,14 @@ fn genesis_builtins(bpf_jit: bool) -> Vec<Builtin> {
]
}

/// Builtin programs activated dynamically by feature
fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> {
/// Dynamic feature transitions for builtin programs
fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
vec![]
}

pub(crate) fn get(bpf_jit: bool) -> Builtins {
Builtins {
genesis_builtins: genesis_builtins(bpf_jit),
feature_builtins: feature_builtins(),
feature_transitions: builtin_feature_transitions(),
}
}
89 changes: 30 additions & 59 deletions runtime/src/bank.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use {
accounts_update_notifier_interface::AccountsUpdateNotifier,
ancestors::{Ancestors, AncestorsForSerialization},
blockhash_queue::BlockhashQueue,
builtins::{self, ActivationType, Builtin, Builtins},
builtins::{self, BuiltinAction, BuiltinFeatureTransition, Builtins},
cost_tracker::CostTracker,
epoch_stakes::{EpochStakes, NodeVoteAccounts},
inline_spl_associated_token_account, inline_spl_token,
Expand Down Expand Up @@ -1196,9 +1196,9 @@ pub struct Bank {

compute_budget: Option<ComputeBudget>,

/// Builtin programs activated dynamically by feature
/// Dynamic feature transitions for builtin programs
#[allow(clippy::rc_buffer)]
feature_builtins: Arc<Vec<(Builtin, Pubkey, ActivationType)>>,
builtin_feature_transitions: Arc<Vec<BuiltinFeatureTransition>>,

/// Protocol-level rewards that were distributed by this bank
pub rewards: RwLock<Vec<(Pubkey, RewardInfo)>>,
Expand Down Expand Up @@ -1367,7 +1367,7 @@ impl Bank {
is_delta: AtomicBool::default(),
builtin_programs: BuiltinPrograms::default(),
compute_budget: Option::<ComputeBudget>::default(),
feature_builtins: Arc::<Vec<(Builtin, Pubkey, ActivationType)>>::default(),
builtin_feature_transitions: Arc::<Vec<BuiltinFeatureTransition>>::default(),
rewards: RwLock::<Vec<(Pubkey, RewardInfo)>>::default(),
cluster_type: Option::<ClusterType>::default(),
lazy_rent_collection: AtomicBool::default(),
Expand Down Expand Up @@ -1703,7 +1703,7 @@ impl Bank {
signature_count: AtomicU64::new(0),
builtin_programs,
compute_budget: parent.compute_budget,
feature_builtins: parent.feature_builtins.clone(),
builtin_feature_transitions: parent.builtin_feature_transitions.clone(),
hard_forks: parent.hard_forks.clone(),
rewards: RwLock::new(vec![]),
cluster_type: parent.cluster_type,
Expand Down Expand Up @@ -1989,7 +1989,7 @@ impl Bank {
is_delta: AtomicBool::new(fields.is_delta),
builtin_programs: new(),
compute_budget: None,
feature_builtins: new(),
builtin_feature_transitions: new(),
rewards: new(),
cluster_type: Some(genesis_config.cluster_type),
lazy_rent_collection: new(),
Expand Down Expand Up @@ -5514,8 +5514,8 @@ impl Bank {
.genesis_builtins
.extend_from_slice(&additional_builtins.genesis_builtins);
builtins
.feature_builtins
.extend_from_slice(&additional_builtins.feature_builtins);
.feature_transitions
.extend_from_slice(&additional_builtins.feature_transitions);
}
if !debug_do_not_add_builtins {
for builtin in builtins.genesis_builtins {
Expand All @@ -5531,7 +5531,7 @@ impl Bank {
}
}
}
self.feature_builtins = Arc::new(builtins.feature_builtins);
self.builtin_feature_transitions = Arc::new(builtins.feature_transitions);

self.apply_feature_activations(true, debug_do_not_add_builtins);
}
Expand Down Expand Up @@ -6238,29 +6238,9 @@ impl Bank {
debug!("Added program {} under {:?}", name, program_id);
}

/// Replace a builtin instruction processor if it already exists
pub fn replace_builtin(
&mut self,
name: &str,
program_id: &Pubkey,
process_instruction: ProcessInstructionWithContext,
) {
debug!("Replacing program {} under {:?}", name, program_id);
self.add_builtin_account(name, program_id, true);
if let Some(entry) = self
.builtin_programs
.vec
.iter_mut()
.find(|entry| entry.program_id == *program_id)
{
entry.process_instruction = process_instruction;
}
debug!("Replaced program {} under {:?}", name, program_id);
}

/// Remove a builtin instruction processor if it already exists
pub fn remove_builtin(&mut self, name: &str, program_id: &Pubkey) {
debug!("Removing program {} under {:?}", name, program_id);
pub fn remove_builtin(&mut self, program_id: &Pubkey) {
debug!("Removing program {}", program_id);
// Don't remove the account since the bank expects the account state to
// be idempotent
if let Some(position) = self
Expand All @@ -6271,7 +6251,7 @@ impl Bank {
{
self.builtin_programs.vec.remove(position);
}
debug!("Removed program {} under {:?}", name, program_id);
debug!("Removed program {}", program_id);
}

pub fn add_precompile(&mut self, program_id: &Pubkey) {
Expand Down Expand Up @@ -6451,7 +6431,7 @@ impl Bank {
}

if !debug_do_not_add_builtins {
self.ensure_feature_builtins(init_finish_or_warp, &new_feature_activations);
self.apply_builtin_feature_transitions(init_finish_or_warp, &new_feature_activations);
self.reconfigure_token2_native_mint();
}
self.ensure_no_storage_rewards_pool();
Expand Down Expand Up @@ -6508,33 +6488,34 @@ impl Bank {
newly_activated
}

fn ensure_feature_builtins(
fn apply_builtin_feature_transitions(
&mut self,
init_or_warp: bool,
new_feature_activations: &HashSet<Pubkey>,
) {
let feature_builtins = self.feature_builtins.clone();
for (builtin, feature, activation_type) in feature_builtins.iter() {
let should_populate = init_or_warp && self.feature_set.is_active(feature)
|| !init_or_warp && new_feature_activations.contains(feature);
if should_populate {
match activation_type {
ActivationType::NewProgram => self.add_builtin(
&builtin.name,
&builtin.id,
builtin.process_instruction_with_context,
),
ActivationType::NewVersion => self.replace_builtin(
let feature_set = self.feature_set.clone();
let should_apply_action_for_feature = |feature_id: &Pubkey| -> bool {
if init_or_warp {
feature_set.is_active(feature_id)
} else {
new_feature_activations.contains(feature_id)
}
};

let builtin_feature_transitions = self.builtin_feature_transitions.clone();
for transition in builtin_feature_transitions.iter() {
if let Some(builtin_action) = transition.to_action(&should_apply_action_for_feature) {
match builtin_action {
BuiltinAction::Add(builtin) => self.add_builtin(
&builtin.name,
&builtin.id,
builtin.process_instruction_with_context,
),
ActivationType::RemoveProgram => {
self.remove_builtin(&builtin.name, &builtin.id)
}
BuiltinAction::Remove { program_id } => self.remove_builtin(&program_id),
}
}
}

for precompile in get_precompiles() {
#[allow(clippy::blocks_in_if_conditions)]
if precompile.feature.map_or(false, |ref feature_id| {
Expand Down Expand Up @@ -13350,16 +13331,6 @@ pub(crate) mod tests {
mock_ix_processor,
);
assert_eq!(bank.get_account_modified_slot(&program_id).unwrap().1, slot);

Arc::get_mut(&mut bank).unwrap().replace_builtin(
"mock_program v2",
&program_id,
mock_ix_processor,
);
assert_eq!(
bank.get_account_modified_slot(&program_id).unwrap().1,
bank.slot()
);
}

#[test]
Expand Down
127 changes: 80 additions & 47 deletions runtime/src/builtins.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,61 @@ macro_rules! with_program_logging {
};
}

#[derive(AbiExample, Debug, Clone)]
pub enum ActivationType {
NewProgram,
NewVersion,
RemoveProgram,
/// State transition enum used for adding and removing builtin programs through
/// feature activations.
#[derive(Debug, Clone)]
pub enum BuiltinFeatureTransition {
/// Add a builtin program if a feature is activated.
Add {
builtin: Builtin,
feature_id: Pubkey,
},
/// Remove a builtin program if a feature is activated or
/// retain a previously added builtin.
RemoveOrRetain {
previous_builtin: Builtin,
removal_feature_id: Pubkey,
},
}

/// Actions taken by a bank when managing the list of active builtin programs.
#[derive(Debug, Clone)]
pub enum BuiltinAction {
Add(Builtin),
Remove { program_id: Pubkey },
}

impl BuiltinFeatureTransition {
pub fn to_action(
&self,
should_apply_action_for_feature: &impl Fn(&Pubkey) -> bool,
) -> Option<BuiltinAction> {
match self {
Self::Add {
builtin,
feature_id,
} => {
if should_apply_action_for_feature(feature_id) {
Some(BuiltinAction::Add(builtin.clone()))
} else {
None
}
}
Self::RemoveOrRetain {
previous_builtin,
removal_feature_id,
} => {
if should_apply_action_for_feature(removal_feature_id) {
Some(BuiltinAction::Remove {
program_id: previous_builtin.id,
})
} else {
// Retaining is no different from adding a new builtin.
Some(BuiltinAction::Add(previous_builtin.clone()))
}
}
}
}
}

#[derive(Clone)]
Expand Down Expand Up @@ -101,8 +151,8 @@ pub struct Builtins {
/// Builtin programs that are always available
pub genesis_builtins: Vec<Builtin>,

/// Builtin programs activated or deactivated dynamically by feature
pub feature_builtins: Vec<(Builtin, Pubkey, ActivationType)>,
/// Dynamic feature transitions for builtin programs
pub feature_transitions: Vec<BuiltinFeatureTransition>,
}

/// Builtin programs that are always available
Expand Down Expand Up @@ -133,11 +183,6 @@ fn genesis_builtins() -> Vec<Builtin> {
solana_sdk::secp256k1_program::id(),
dummy_process_instruction,
),
Builtin::new(
"ed25519_program",
solana_sdk::ed25519_program::id(),
dummy_process_instruction,
),
]
}

Expand All @@ -150,73 +195,61 @@ fn dummy_process_instruction(
Ok(())
}

/// Builtin programs activated dynamically by feature
///
/// Note: If the feature_builtin is intended to replace another builtin program, it must have a new
/// name.
/// This is to enable the runtime to determine categorically whether the builtin update has
/// occurred, and preserve idempotency in Bank::add_native_program across genesis, snapshot, and
/// normal child Bank creation.
/// https://github.com/solana-labs/solana/blob/84b139cc94b5be7c9e0c18c2ad91743231b85a0d/runtime/src/bank.rs#L1723
fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> {
/// Dynamic feature transitions for builtin programs
fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
vec![
(
Builtin::new(
BuiltinFeatureTransition::Add {
builtin: Builtin::new(
"compute_budget_program",
solana_sdk::compute_budget::id(),
solana_compute_budget_program::process_instruction,
),
feature_set::add_compute_budget_program::id(),
ActivationType::NewProgram,
),
feature_id: feature_set::add_compute_budget_program::id(),
},
// TODO when feature `prevent_calling_precompiles_as_programs` is
// cleaned up also remove "secp256k1_program" from the main builtins
// list
(
Builtin::new(
BuiltinFeatureTransition::RemoveOrRetain {
previous_builtin: Builtin::new(
"secp256k1_program",
solana_sdk::secp256k1_program::id(),
dummy_process_instruction,
),
feature_set::prevent_calling_precompiles_as_programs::id(),
ActivationType::RemoveProgram,
),
removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
},
// TODO when feature `prevent_calling_precompiles_as_programs` is
// cleaned up also remove "ed25519_program" from the main builtins
// list
(
Builtin::new(
BuiltinFeatureTransition::RemoveOrRetain {
previous_builtin: Builtin::new(
"ed25519_program",
solana_sdk::ed25519_program::id(),
dummy_process_instruction,
),
feature_set::prevent_calling_precompiles_as_programs::id(),
ActivationType::RemoveProgram,
),
(
Builtin::new(
removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
},
BuiltinFeatureTransition::Add {
builtin: Builtin::new(
"address_lookup_table_program",
solana_address_lookup_table_program::id(),
solana_address_lookup_table_program::processor::process_instruction,
),
feature_set::versioned_tx_message_enabled::id(),
ActivationType::NewProgram,
),
(
Builtin::new(
feature_id: feature_set::versioned_tx_message_enabled::id(),
},
BuiltinFeatureTransition::Add {
builtin: Builtin::new(
"zk_token_proof_program",
solana_zk_token_sdk::zk_token_proof_program::id(),
with_program_logging!(solana_zk_token_proof_program::process_instruction),
),
feature_set::zk_token_sdk_enabled::id(),
ActivationType::NewProgram,
),
feature_id: feature_set::zk_token_sdk_enabled::id(),
},
]
}

pub(crate) fn get() -> Builtins {
Builtins {
genesis_builtins: genesis_builtins(),
feature_builtins: feature_builtins(),
feature_transitions: builtin_feature_transitions(),
}
}

0 comments on commit a525b16

Please sign in to comment.