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

Commit

Permalink
Remove demote_program_write_locks feature (#19877)
Browse files Browse the repository at this point in the history 
* Remove demote_program_write_locks feature

* Update test
  • Loading branch information
@CriesofCarrots
CriesofCarrots authored Sep 14, 2021
1 parent bf7c2f7 commit a4ebbc9
Show file tree
Hide file tree
Showing 11 changed files with 76 additions and 186 deletions.
2 changes: 1 addition & 1 deletion cli-output/src/display.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ fn format_account_mode(message: &Message, index: usize) -> String {
} else {
"-"
},
if message.is_writable(index, /*demote_program_write_locks=*/ true) {
if message.is_writable(index) {
"w" // comment for consistent rust fmt (no joking; lol)
} else {
"-"
Expand Down
5 changes: 2 additions & 3 deletions core/src/transaction_status_service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,8 @@ impl TransactionStatusService {
})
.expect("FeeCalculator must exist");
let fee = fee_calculator.calculate_fee(transaction.message());
let (writable_keys, readonly_keys) = transaction
.message
.get_account_keys_by_lock_type(bank.demote_program_write_locks());
let (writable_keys, readonly_keys) =
transaction.message.get_account_keys_by_lock_type();

let inner_instructions = inner_instructions.map(|inner_instructions| {
inner_instructions
Expand Down
7 changes: 2 additions & 5 deletions program-test/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use {
clock::{Clock, Slot},
entrypoint::{ProgramResult, SUCCESS},
epoch_schedule::EpochSchedule,
feature_set::demote_program_write_locks,
fee_calculator::{FeeCalculator, FeeRateGovernor},
genesis_config::{ClusterType, GenesisConfig},
hash::Hash,
Expand Down Expand Up @@ -256,14 +255,12 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
}
panic!("Program id {} wasn't found in account_infos", program_id);
};
let demote_program_write_locks =
invoke_context.is_feature_active(&demote_program_write_locks::id());
// TODO don't have the caller's keyed_accounts so can't validate writer or signer escalation or deescalation yet
let caller_privileges = message
.account_keys
.iter()
.enumerate()
.map(|(i, _)| message.is_writable(i, demote_program_write_locks))
.map(|(i, _)| message.is_writable(i))
.collect::<Vec<bool>>();

stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth());
Expand Down Expand Up @@ -334,7 +331,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {

// Copy writeable account modifications back into the caller's AccountInfos
for (i, account_pubkey) in message.account_keys.iter().enumerate() {
if !message.is_writable(i, demote_program_write_locks) {
if !message.is_writable(i) {
continue;
}

Expand Down
18 changes: 5 additions & 13 deletions programs/bpf_loader/src/syscalls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ use solana_sdk::{
entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS},
epoch_schedule::EpochSchedule,
feature_set::{
cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_program_write_locks,
enforce_aligned_host_addrs, keccak256_syscall_enabled, memory_ops_syscalls,
set_upgrade_authority_via_cpi_enabled, sysvar_via_syscall, update_data_on_realloc,
cpi_data_cost, cpi_share_ro_and_exec_accounts, enforce_aligned_host_addrs,
keccak256_syscall_enabled, memory_ops_syscalls, set_upgrade_authority_via_cpi_enabled,
sysvar_via_syscall, update_data_on_realloc,
},
hash::{Hasher, HASH_BYTES},
ic_msg,
Expand Down Expand Up @@ -2140,14 +2140,7 @@ fn call<'a>(
signers_seeds_len: u64,
memory_mapping: &MemoryMapping,
) -> Result<u64, EbpfError<BpfError>> {
let (
message,
executables,
accounts,
account_refs,
caller_write_privileges,
demote_program_write_locks,
) = {
let (message, executables, accounts, account_refs, caller_write_privileges) = {
let invoke_context = syscall.get_context()?;

invoke_context
Expand Down Expand Up @@ -2237,7 +2230,6 @@ fn call<'a>(
accounts,
account_refs,
caller_write_privileges,
invoke_context.is_feature_active(&demote_program_write_locks::id()),
)
};

Expand All @@ -2263,7 +2255,7 @@ fn call<'a>(
for (i, (account, account_ref)) in accounts.iter().zip(account_refs).enumerate() {
let account = account.borrow();
if let Some(mut account_ref) = account_ref {
if message.is_writable(i, demote_program_write_locks) && !account.executable() {
if message.is_writable(i) && !account.executable() {
*account_ref.lamports = account.lamports();
*account_ref.owner = *account.owner();
if account_ref.data.len() != account.data().len() {
Expand Down
91 changes: 22 additions & 69 deletions runtime/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,8 @@ impl Accounts {
false
}

fn construct_instructions_account(
message: &Message,
demote_program_write_locks: bool,
) -> AccountSharedData {
let mut data = message.serialize_instructions(demote_program_write_locks);
fn construct_instructions_account(message: &Message) -> AccountSharedData {
let mut data = message.serialize_instructions();
// add room for current instruction index.
data.resize(data.len() + 2, 0);
AccountSharedData::from(Account {
Expand Down Expand Up @@ -204,8 +201,6 @@ impl Accounts {
let mut accounts = Vec::with_capacity(message.account_keys.len());
let mut account_deps = Vec::with_capacity(message.account_keys.len());
let mut rent_debits = RentDebits::default();
let demote_program_write_locks =
feature_set.is_active(&feature_set::demote_program_write_locks::id());
let is_upgradeable_loader_present = is_upgradeable_loader_present(message);

for (i, key) in message.account_keys.iter().enumerate() {
Expand All @@ -217,13 +212,13 @@ impl Accounts {
if solana_sdk::sysvar::instructions::check_id(key)
&& feature_set.is_active(&feature_set::instructions_sysvar_enabled::id())
{
Self::construct_instructions_account(message, demote_program_write_locks)
Self::construct_instructions_account(message)
} else {
let (account, rent) = self
.accounts_db
.load(ancestors, key)
.map(|(mut account, _)| {
if message.is_writable(i, demote_program_write_locks) {
if message.is_writable(i) {
let rent_due = rent_collector
.collect_from_existing_account(&key, &mut account);
(account, rent_due)
Expand All @@ -234,10 +229,7 @@ impl Accounts {
.unwrap_or_default();

if bpf_loader_upgradeable::check_id(&account.owner) {
if demote_program_write_locks
&& message.is_writable(i, demote_program_write_locks)
&& !is_upgradeable_loader_present
{
if message.is_writable(i) && !is_upgradeable_loader_present {
error_counters.invalid_writable_account += 1;
return Err(TransactionError::InvalidWritableAccount);
}
Expand All @@ -263,10 +255,7 @@ impl Accounts {
return Err(TransactionError::InvalidProgramForExecution);
}
}
} else if account.executable
&& demote_program_write_locks
&& message.is_writable(i, demote_program_write_locks)
{
} else if account.executable && message.is_writable(i) {
error_counters.invalid_writable_account += 1;
return Err(TransactionError::InvalidWritableAccount);
}
Expand Down Expand Up @@ -785,21 +774,13 @@ impl Accounts {
Ok(())
}

fn unlock_account(
&self,
tx: &Transaction,
result: &Result<()>,
locks: &mut AccountLocks,
demote_program_write_locks: bool,
) {
fn unlock_account(&self, tx: &Transaction, result: &Result<()>, locks: &mut AccountLocks) {
match result {
Err(TransactionError::AccountInUse) => (),
Err(TransactionError::SanitizeFailure) => (),
Err(TransactionError::AccountLoadedTwice) => (),
_ => {
let (writable_keys, readonly_keys) = &tx
.message()
.get_account_keys_by_lock_type(demote_program_write_locks);
let (writable_keys, readonly_keys) = &tx.message().get_account_keys_by_lock_type();
for k in writable_keys {
locks.unlock_write(k);
}
Expand Down Expand Up @@ -828,11 +809,7 @@ impl Accounts {
/// This function will prevent multiple threads from modifying the same account state at the
/// same time
#[must_use]
pub fn lock_accounts<'a>(
&self,
txs: impl Iterator<Item = &'a Transaction>,
demote_program_write_locks: bool,
) -> Vec<Result<()>> {
pub fn lock_accounts<'a>(&self, txs: impl Iterator<Item = &'a Transaction>) -> Vec<Result<()>> {
use solana_sdk::sanitize::Sanitize;
let keys: Vec<Result<_>> = txs
.map(|tx| {
Expand All @@ -842,9 +819,7 @@ impl Accounts {
return Err(TransactionError::AccountLoadedTwice);
}

Ok(tx
.message()
.get_account_keys_by_lock_type(demote_program_write_locks))
Ok(tx.message().get_account_keys_by_lock_type())
})
.collect();
let mut account_locks = &mut self.account_locks.lock().unwrap();
Expand All @@ -863,17 +838,11 @@ impl Accounts {
&self,
txs: impl Iterator<Item = &'a Transaction>,
results: &[Result<()>],
demote_program_write_locks: bool,
) {
let mut account_locks = self.account_locks.lock().unwrap();
debug!("bank unlock accounts");
for (tx, lock_result) in txs.zip(results) {
self.unlock_account(
tx,
lock_result,
&mut account_locks,
demote_program_write_locks,
);
self.unlock_account(tx, lock_result, &mut account_locks);
}
}

Expand All @@ -890,7 +859,6 @@ impl Accounts {
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
fix_recent_blockhashes_sysvar_delay: bool,
merge_nonce_error_into_system_error: bool,
demote_program_write_locks: bool,
) {
let accounts_to_store = self.collect_accounts_to_store(
txs,
Expand All @@ -900,7 +868,6 @@ impl Accounts {
last_blockhash_with_fee_calculator,
fix_recent_blockhashes_sysvar_delay,
merge_nonce_error_into_system_error,
demote_program_write_locks,
);
self.accounts_db.store_cached(slot, &accounts_to_store);
}
Expand All @@ -926,7 +893,6 @@ impl Accounts {
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
fix_recent_blockhashes_sysvar_delay: bool,
merge_nonce_error_into_system_error: bool,
demote_program_write_locks: bool,
) -> Vec<(&'a Pubkey, &'a AccountSharedData)> {
let mut accounts = Vec::with_capacity(loaded.len());
for (i, ((raccs, _nonce_rollback), tx)) in loaded.iter_mut().zip(txs).enumerate() {
Expand Down Expand Up @@ -979,7 +945,7 @@ impl Accounts {
fee_payer_index = Some(i);
}
let is_fee_payer = Some(i) == fee_payer_index;
if message.is_writable(i, demote_program_write_locks)
if message.is_writable(i)
&& (res.is_ok()
|| (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer)))
{
Expand Down Expand Up @@ -2022,8 +1988,6 @@ mod tests {
accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2);
accounts.store_slow_uncached(0, &keypair3.pubkey(), &account3);

let demote_program_write_locks = true;

let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
let message = Message::new_with_compiled_instructions(
1,
Expand All @@ -2034,7 +1998,7 @@ mod tests {
instructions,
);
let tx = Transaction::new(&[&keypair0], message, Hash::default());
let results0 = accounts.lock_accounts([tx.clone()].iter(), demote_program_write_locks);
let results0 = accounts.lock_accounts([tx.clone()].iter());

assert!(results0[0].is_ok());
assert_eq!(
Expand Down Expand Up @@ -2069,7 +2033,7 @@ mod tests {
);
let tx1 = Transaction::new(&[&keypair1], message, Hash::default());
let txs = vec![tx0, tx1];
let results1 = accounts.lock_accounts(txs.iter(), demote_program_write_locks);
let results1 = accounts.lock_accounts(txs.iter());

assert!(results1[0].is_ok()); // Read-only account (keypair1) can be referenced multiple times
assert!(results1[1].is_err()); // Read-only account (keypair1) cannot also be locked as writable
Expand All @@ -2084,8 +2048,8 @@ mod tests {
2
);

accounts.unlock_accounts([tx].iter(), &results0, demote_program_write_locks);
accounts.unlock_accounts(txs.iter(), &results1, demote_program_write_locks);
accounts.unlock_accounts([tx].iter(), &results0);
accounts.unlock_accounts(txs.iter(), &results1);
let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
let message = Message::new_with_compiled_instructions(
1,
Expand All @@ -2096,7 +2060,7 @@ mod tests {
instructions,
);
let tx = Transaction::new(&[&keypair1], message, Hash::default());
let results2 = accounts.lock_accounts([tx].iter(), demote_program_write_locks);
let results2 = accounts.lock_accounts([tx].iter());
assert!(results2[0].is_ok()); // Now keypair1 account can be locked as writable

// Check that read-only lock with zero references is deleted
Expand Down Expand Up @@ -2132,8 +2096,6 @@ mod tests {
accounts.store_slow_uncached(0, &keypair1.pubkey(), &account1);
accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2);

let demote_program_write_locks = true;

let accounts_arc = Arc::new(accounts);

let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
Expand Down Expand Up @@ -2166,15 +2128,13 @@ mod tests {
let exit_clone = exit_clone.clone();
loop {
let txs = vec![writable_tx.clone()];
let results = accounts_clone
.clone()
.lock_accounts(txs.iter(), demote_program_write_locks);
let results = accounts_clone.clone().lock_accounts(txs.iter());
for result in results.iter() {
if result.is_ok() {
counter_clone.clone().fetch_add(1, Ordering::SeqCst);
}
}
accounts_clone.unlock_accounts(txs.iter(), &results, demote_program_write_locks);
accounts_clone.unlock_accounts(txs.iter(), &results);
if exit_clone.clone().load(Ordering::Relaxed) {
break;
}
Expand All @@ -2183,15 +2143,13 @@ mod tests {
let counter_clone = counter;
for _ in 0..5 {
let txs = vec![readonly_tx.clone()];
let results = accounts_arc
.clone()
.lock_accounts(txs.iter(), demote_program_write_locks);
let results = accounts_arc.clone().lock_accounts(txs.iter());
if results[0].is_ok() {
let counter_value = counter_clone.clone().load(Ordering::SeqCst);
thread::sleep(time::Duration::from_millis(50));
assert_eq!(counter_value, counter_clone.clone().load(Ordering::SeqCst));
}
accounts_arc.unlock_accounts(txs.iter(), &results, demote_program_write_locks);
accounts_arc.unlock_accounts(txs.iter(), &results);
thread::sleep(time::Duration::from_millis(50));
}
exit.store(true, Ordering::Relaxed);
Expand Down Expand Up @@ -2220,8 +2178,6 @@ mod tests {
accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2);
accounts.store_slow_uncached(0, &keypair3.pubkey(), &account3);

let demote_program_write_locks = true;

let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
let message = Message::new_with_compiled_instructions(
1,
Expand All @@ -2232,7 +2188,7 @@ mod tests {
instructions,
);
let tx = Transaction::new(&[&keypair0], message, Hash::default());
let results0 = accounts.lock_accounts([tx].iter(), demote_program_write_locks);
let results0 = accounts.lock_accounts([tx].iter());

assert!(results0[0].is_ok());
// Instruction program-id account demoted to readonly
Expand Down Expand Up @@ -2349,7 +2305,6 @@ mod tests {
&(Hash::default(), FeeCalculator::default()),
true,
true, // merge_nonce_error_into_system_error
true, // demote_program_write_locks
);
assert_eq!(collected_accounts.len(), 2);
assert!(collected_accounts
Expand Down Expand Up @@ -2735,7 +2690,6 @@ mod tests {
&(next_blockhash, FeeCalculator::default()),
true,
true, // merge_nonce_error_into_system_error
true, // demote_program_write_locks
);
assert_eq!(collected_accounts.len(), 2);
assert_eq!(
Expand Down Expand Up @@ -2852,7 +2806,6 @@ mod tests {
&(next_blockhash, FeeCalculator::default()),
true,
true, // merge_nonce_error_into_system_error
true, // demote_program_write_locks
);
assert_eq!(collected_accounts.len(), 1);
let collected_nonce_account = collected_accounts
Expand Down
Loading

0 comments on commit a4ebbc9

Please sign in to comment.