Skip to content

Remove must_replace parameter from add_builtin_account #662

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

Merged
LucasSte merged 1 commit into from
Apr 8, 2024
Merged

Remove must_replace parameter from add_builtin_account #662

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
2 changes: 1 addition & 1 deletion runtime/benches/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ fn do_bench_transactions(

let mut bank = Bank::new_from_parent(Arc::new(bank), &Pubkey::default(), 1);
bank.add_mockup_builtin(Pubkey::from(BUILTIN_PROGRAM_ID), MockBuiltin::vm);
bank.add_builtin_account("solana_noop_program", &Pubkey::from(NOOP_PROGRAM_ID), false);
bank.add_builtin_account("solana_noop_program", &Pubkey::from(NOOP_PROGRAM_ID));
let bank = Arc::new(bank);
let bank_client = BankClient::new_shared(bank.clone());
let transactions = create_transactions(&bank_client, &mint_keypair);
Expand Down
29 changes: 7 additions & 22 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3138,7 +3138,7 @@ impl Bank {

// Add additional builtin programs specified in the genesis config
for (name, program_id) in &genesis_config.native_instruction_processors {
self.add_builtin_account(name, program_id, false);
self.add_builtin_account(name, program_id);
}
}

Expand All @@ -3155,7 +3155,7 @@ impl Bank {

// NOTE: must hold idempotent for the same set of arguments
/// Add a builtin program account
pub fn add_builtin_account(&self, name: &str, program_id: &Pubkey, must_replace: bool) {
pub fn add_builtin_account(&self, name: &str, program_id: &Pubkey) {
let existing_genuine_program =
self.get_account_with_fixed_root(program_id)
.and_then(|account| {
Expand All @@ -3171,25 +3171,10 @@ impl Bank {
}
});

if must_replace {
// updating builtin program
match &existing_genuine_program {
None => panic!(
"There is no account to replace with builtin program ({name}, {program_id})."
),
Some(account) => {
if *name == String::from_utf8_lossy(account.data()) {
// The existing account is well formed
return;
}
}
}
} else {
// introducing builtin program
if existing_genuine_program.is_some() {
// The existing account is sufficient
return;
}
// introducing builtin program
if existing_genuine_program.is_some() {
// The existing account is sufficient
return;
}

assert!(
Expand Down Expand Up @@ -6450,7 +6435,7 @@ impl Bank {
/// Add a built-in program
pub fn add_builtin(&mut self, program_id: Pubkey, name: &str, builtin: LoadedProgram) {
debug!("Adding program {} under {:?}", name, program_id);
self.add_builtin_account(name, &program_id, false);
self.add_builtin_account(name, &program_id);
self.builtin_program_ids.insert(program_id);
self.transaction_processor
.program_cache
Expand Down
62 changes: 11 additions & 51 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6777,7 +6777,7 @@ fn test_add_builtin_account() {

assert_capitalization_diff(
&bank,
|| bank.add_builtin_account("mock_program", &program_id, false),
|| bank.add_builtin_account("mock_program", &program_id),
|old, new| {
assert_eq!(old + 1, new);
pass == 0
Expand All @@ -6793,7 +6793,7 @@ fn test_add_builtin_account() {
add_root_and_flush_write_cache(&bank.parent().unwrap());
assert_capitalization_diff(
&bank,
|| bank.add_builtin_account("mock_program", &program_id, false),
|| bank.add_builtin_account("mock_program", &program_id),
|old, new| {
assert_eq!(old, new);
pass == 1
Expand All @@ -6807,11 +6807,11 @@ fn test_add_builtin_account() {

let bank = Arc::new(new_from_parent(bank));
add_root_and_flush_write_cache(&bank.parent().unwrap());
// When replacing builtin_program, name must change to disambiguate from repeated
// invocations.
// No builtin replacement should happen if the program id is already assigned to a
// builtin.
assert_capitalization_diff(
&bank,
|| bank.add_builtin_account("mock_program v2", &program_id, true),
|| bank.add_builtin_account("mock_program v2", &program_id),
|old, new| {
assert_eq!(old, new);
pass == 2
Expand All @@ -6821,30 +6821,8 @@ fn test_add_builtin_account() {
continue;
}

assert_eq!(
bank.get_account_modified_slot(&program_id).unwrap().1,
bank.slot()
);

let bank = Arc::new(new_from_parent(bank));
add_root_and_flush_write_cache(&bank.parent().unwrap());
assert_capitalization_diff(
&bank,
|| bank.add_builtin_account("mock_program v2", &program_id, true),
|old, new| {
assert_eq!(old, new);
pass == 3
},
);
if pass == 3 {
continue;
}

// replacing with same name shouldn't update account
assert_eq!(
bank.get_account_modified_slot(&program_id).unwrap().1,
bank.parent_slot()
);
// No replacement should have happened
assert_eq!(bank.get_account_modified_slot(&program_id).unwrap().1, slot);
}
}

Expand All @@ -6862,7 +6840,7 @@ fn test_add_builtin_account_inherited_cap_while_replacing() {
let bank = Bank::new_for_tests(&genesis_config);
let program_id = solana_sdk::pubkey::new_rand();

bank.add_builtin_account("mock_program", &program_id, false);
bank.add_builtin_account("mock_program", &program_id);
if pass == 0 {
add_root_and_flush_write_cache(&bank);
assert_eq!(bank.capitalization(), bank.calculate_capitalization(true));
Expand All @@ -6883,7 +6861,7 @@ fn test_add_builtin_account_inherited_cap_while_replacing() {
continue;
}

bank.add_builtin_account("mock_program v2", &program_id, true);
bank.add_builtin_account("mock_program v2", &program_id);
add_root_and_flush_write_cache(&bank);
assert_eq!(bank.capitalization(), bank.calculate_capitalization(true));
}
Expand All @@ -6910,7 +6888,7 @@ fn test_add_builtin_account_squatted_while_not_replacing() {
continue;
}

bank.add_builtin_account("mock_program", &program_id, false);
bank.add_builtin_account("mock_program", &program_id);
add_root_and_flush_write_cache(&bank);
assert_eq!(bank.capitalization(), bank.calculate_capitalization(true));
}
Expand All @@ -6933,25 +6911,7 @@ fn test_add_builtin_account_after_frozen() {
);
bank.freeze();

bank.add_builtin_account("mock_program", &program_id, false);
}

#[test]
#[should_panic(
expected = "There is no account to replace with builtin program (mock_program, \
CiXgo2KHKSDmDnV1F6B69eWFgNAPiSBjjYvfB4cvRNre)."
)]
fn test_add_builtin_account_replace_none() {
let slot = 123;
let program_id = Pubkey::from_str("CiXgo2KHKSDmDnV1F6B69eWFgNAPiSBjjYvfB4cvRNre").unwrap();

let bank = Bank::new_from_parent(
create_simple_test_arc_bank(100_000).0,
&Pubkey::default(),
slot,
);

bank.add_builtin_account("mock_program", &program_id, true);
bank.add_builtin_account("mock_program", &program_id);
}

#[test]
Expand Down