gaspersic/solana
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Remove feature switch for demoting sysvar write locks (#18373) (#18454)

Browse Source 
Co-authored-by: Justin Starry <justin@solana.com>
This commit is contained in:
mergify[bot]
2021-07-06 23:25:28 +00:00
committed by GitHub
parent 6afeaac7a5
commit 72ed4f28b1
11 changed files with 70 additions and 184 deletions
Download Patch File Download Diff File Expand all files Collapse all files

88
runtime/src/accounts.rs
View File

@@ -180,11 +180,8 @@ impl Accounts {
false
}
fn construct_instructions_account(
message: &Message,
demote_sysvar_write_locks: bool,
) -> AccountSharedData {
let mut data = message.serialize_instructions(demote_sysvar_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 {
@@ -213,8 +210,6 @@ impl Accounts {
let mut tx_rent: TransactionRent = 0;
let mut accounts = Vec::with_capacity(message.account_keys.len());
let mut account_deps = Vec::with_capacity(message.account_keys.len());
let demote_sysvar_write_locks =
feature_set.is_active(&feature_set::demote_sysvar_write_locks::id());
let mut key_check = MessageProgramIdsCache::new(message);
let mut rent_debits = RentDebits::default();
for (i, key) in message.account_keys.iter().enumerate() {
@@ -226,16 +221,16 @@ impl Accounts {
if solana_sdk::sysvar::instructions::check_id(key)
&& feature_set.is_active(&feature_set::instructions_sysvar_enabled::id())
{
if message.is_writable(i, demote_sysvar_write_locks) {
if message.is_writable(i) {
return Err(TransactionError::InvalidAccountIndex);
}
Self::construct_instructions_account(message, demote_sysvar_write_locks)
Self::construct_instructions_account(message)
} else {
let (account, rent) = self
.accounts_db
.load_with_fixed_root(ancestors, key)
.map(|(mut account, _)| {
if message.is_writable(i, demote_sysvar_write_locks) {
if message.is_writable(i) {
let rent_due = rent_collector
.collect_from_existing_account(key, &mut account);
(account, rent_due)
@@ -858,11 +853,7 @@ impl Accounts {
/// same time
#[must_use]
#[allow(clippy::needless_collect)]
pub fn lock_accounts<'a>(
&self,
txs: impl Iterator<Item = &'a Transaction>,
demote_sysvar_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| {
@@ -872,9 +863,7 @@ impl Accounts {
return Err(TransactionError::AccountLoadedTwice);
}
Ok(tx
.message()
.get_account_keys_by_lock_type(demote_sysvar_write_locks))
Ok(tx.message().get_account_keys_by_lock_type())
})
.collect();
let mut account_locks = &mut self.account_locks.lock().unwrap();
@@ -894,7 +883,6 @@ impl Accounts {
&self,
txs: impl Iterator<Item = &'a Transaction>,
results: &[Result<()>],
demote_sysvar_write_locks: bool,
) {
let keys: Vec<_> = txs
.zip(results)
@@ -902,10 +890,7 @@ impl Accounts {
Err(TransactionError::AccountInUse) => None,
Err(TransactionError::SanitizeFailure) => None,
Err(TransactionError::AccountLoadedTwice) => None,
_ => Some(
tx.message
.get_account_keys_by_lock_type(demote_sysvar_write_locks),
),
_ => Some(tx.message.get_account_keys_by_lock_type()),
})
.collect();
let mut account_locks = self.account_locks.lock().unwrap();
@@ -927,7 +912,6 @@ impl Accounts {
rent_collector: &RentCollector,
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
fix_recent_blockhashes_sysvar_delay: bool,
demote_sysvar_write_locks: bool,
) {
let accounts_to_store = self.collect_accounts_to_store(
txs,
@@ -936,7 +920,6 @@ impl Accounts {
rent_collector,
last_blockhash_with_fee_calculator,
fix_recent_blockhashes_sysvar_delay,
demote_sysvar_write_locks,
);
self.accounts_db.store_cached(slot, &accounts_to_store);
}
@@ -961,7 +944,6 @@ impl Accounts {
rent_collector: &RentCollector,
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
fix_recent_blockhashes_sysvar_delay: bool,
demote_sysvar_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() {
@@ -1008,7 +990,7 @@ impl Accounts {
fee_payer_index = Some(i);
}
let is_fee_payer = Some(i) == fee_payer_index;
if message.is_writable(i, demote_sysvar_write_locks)
if message.is_writable(i)
&& (res.is_ok()
|| (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer)))
{
@@ -1773,10 +1755,7 @@ mod tests {
instructions,
);
let tx = Transaction::new(&[&keypair0], message, Hash::default());
let results0 = accounts.lock_accounts(
[tx.clone()].iter(),
true, // demote_sysvar_write_locks
);
let results0 = accounts.lock_accounts([tx.clone()].iter());
assert!(results0[0].is_ok());
assert_eq!(
@@ -1811,10 +1790,7 @@ mod tests {
);
let tx1 = Transaction::new(&[&keypair1], message, Hash::default());
let txs = vec![tx0, tx1];
let results1 = accounts.lock_accounts(
txs.iter(),
true, // demote_sysvar_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
@@ -1829,16 +1805,8 @@ mod tests {
2
);
accounts.unlock_accounts(
[tx].iter(),
&results0,
true, // demote_sysvar_write_locks
);
accounts.unlock_accounts(
txs.iter(),
&results1,
true, // demote_sysvar_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,
@@ -1849,10 +1817,7 @@ mod tests {
instructions,
);
let tx = Transaction::new(&[&keypair1], message, Hash::default());
let results2 = accounts.lock_accounts(
[tx].iter(),
true, // demote_sysvar_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
@@ -1921,20 +1886,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(),
true, // demote_sysvar_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,
true, // demote_sysvar_write_locks
);
accounts_clone.unlock_accounts(txs.iter(), &results);
if exit_clone.clone().load(Ordering::Relaxed) {
break;
}
@@ -1943,20 +1901,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(),
true, // demote_sysvar_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,
true, // demote_sysvar_write_locks
);
accounts_arc.unlock_accounts(txs.iter(), &results);
thread::sleep(time::Duration::from_millis(50));
}
exit.store(true, Ordering::Relaxed);
@@ -2050,7 +2001,6 @@ mod tests {
&rent_collector,
&(Hash::default(), FeeCalculator::default()),
true,
true, // demote_sysvar_write_locks
);
assert_eq!(collected_accounts.len(), 2);
assert!(collected_accounts
@@ -2430,7 +2380,6 @@ mod tests {
&rent_collector,
&(next_blockhash, FeeCalculator::default()),
true,
true, // demote_sysvar_write_locks
);
assert_eq!(collected_accounts.len(), 2);
assert_eq!(
@@ -2550,7 +2499,6 @@ mod tests {
&rent_collector,
&(next_blockhash, FeeCalculator::default()),
true,
true, // demote_sysvar_write_locks
);
assert_eq!(collected_accounts.len(), 1);
let collected_nonce_account = collected_accounts

37
runtime/src/bank.rs
View File

@@ -2589,20 +2589,15 @@ impl Bank {
tick_height % self.ticks_per_slot == 0
}
pub fn demote_sysvar_write_locks(&self) -> bool {
self.feature_set
.is_active(&feature_set::demote_sysvar_write_locks::id())
}
pub fn prepare_batch<'a, 'b>(
&'a self,
txs: impl Iterator<Item = &'b Transaction>,
) -> TransactionBatch<'a, 'b> {
let hashed_txs: Vec<HashedTransaction> = txs.map(HashedTransaction::from).collect();
let lock_results = self.rc.accounts.lock_accounts(
hashed_txs.as_transactions_iter(),
self.demote_sysvar_write_locks(),
);
let lock_results = self
.rc
.accounts
.lock_accounts(hashed_txs.as_transactions_iter());
TransactionBatch::new(lock_results, self, Cow::Owned(hashed_txs))
}
@@ -2610,10 +2605,10 @@ impl Bank {
&'a self,
hashed_txs: &'b [HashedTransaction],
) -> TransactionBatch<'a, 'b> {
let lock_results = self.rc.accounts.lock_accounts(
hashed_txs.as_transactions_iter(),
self.demote_sysvar_write_locks(),
);
let lock_results = self
.rc
.accounts
.lock_accounts(hashed_txs.as_transactions_iter());
TransactionBatch::new(lock_results, self, Cow::Borrowed(hashed_txs))
}
@@ -2688,11 +2683,9 @@ impl Bank {
pub fn unlock_accounts(&self, batch: &mut TransactionBatch) {
if batch.needs_unlock {
batch.needs_unlock = false;
self.rc.accounts.unlock_accounts(
batch.transactions_iter(),
batch.lock_results(),
self.demote_sysvar_write_locks(),
)
self.rc
.accounts
.unlock_accounts(batch.transactions_iter(), batch.lock_results())
}
}
@@ -3423,7 +3416,6 @@ impl Bank {
&self.rent_collector,
&self.last_blockhash_with_fee_calculator(),
self.fix_recent_blockhashes_sysvar_delay(),
self.demote_sysvar_write_locks(),
);
let rent_debits = self.collect_rent(executed, loaded_accounts);
@@ -7722,13 +7714,14 @@ pub(crate) mod tests {
assert_eq!(bank.get_balance(&sysvar_pubkey), 1);
bank.transfer(500, &mint_keypair, &normal_pubkey).unwrap();
bank.transfer(500, &mint_keypair, &sysvar_pubkey).unwrap();
bank.transfer(500, &mint_keypair, &sysvar_pubkey)
.unwrap_err();
assert_eq!(bank.get_balance(&normal_pubkey), 500);
assert_eq!(bank.get_balance(&sysvar_pubkey), 501);
assert_eq!(bank.get_balance(&sysvar_pubkey), 1);
let bank = Arc::new(new_from_parent(&bank));
assert_eq!(bank.get_balance(&normal_pubkey), 500);
assert_eq!(bank.get_balance(&sysvar_pubkey), 501);
assert_eq!(bank.get_balance(&sysvar_pubkey), 1);
}
#[test]

38
runtime/src/message_processor.rs
View File

@@ -8,7 +8,7 @@ use solana_sdk::{
account::{AccountSharedData, ReadableAccount, WritableAccount},
account_utils::StateMut,
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
feature_set::{demote_sysvar_write_locks, instructions_sysvar_enabled, FeatureSet},
feature_set::{instructions_sysvar_enabled, FeatureSet},
ic_logger_msg, ic_msg,
instruction::{CompiledInstruction, Instruction, InstructionError},
keyed_account::{create_keyed_accounts_unified, keyed_account_at_index, KeyedAccount},
@@ -294,7 +294,6 @@ impl<'a> ThisInvokeContext<'a> {
instruction,
executable_accounts,
accounts,
feature_set.is_active(&demote_sysvar_write_locks::id()),
);
let mut invoke_context = Self {
invoke_stack: Vec::with_capacity(bpf_compute_budget.max_invoke_depth),
@@ -410,7 +409,6 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> {
&self.rent,
caller_write_privileges,
&mut self.timings,
self.feature_set.is_active(&demote_sysvar_write_locks::id()),
logger,
)
}
@@ -622,7 +620,6 @@ impl MessageProcessor {
instruction: &'a CompiledInstruction,
executable_accounts: &'a [(Pubkey, Rc<RefCell<AccountSharedData>>)],
accounts: &'a [Rc<RefCell<AccountSharedData>>],
demote_sysvar_write_locks: bool,
) -> Vec<(bool, bool, &'a Pubkey, &'a RefCell<AccountSharedData>)> {
executable_accounts
.iter()
@@ -631,7 +628,7 @@ impl MessageProcessor {
let index = *index as usize;
(
message.is_signer(index),
message.is_writable(index, demote_sysvar_write_locks),
message.is_writable(index),
&message.account_keys[index],
&accounts[index] as &RefCell<AccountSharedData>,
)
@@ -768,7 +765,6 @@ impl MessageProcessor {
accounts,
keyed_account_indices_reordered,
caller_write_privileges,
demote_sysvar_write_locks,
) = {
let invoke_context = invoke_context.borrow();
@@ -861,7 +857,6 @@ impl MessageProcessor {
accounts,
keyed_account_indices_reordered,
caller_write_privileges,
invoke_context.is_feature_active(&demote_sysvar_write_locks::id()),
)
};
@@ -886,9 +881,7 @@ impl MessageProcessor {
{
let dst_keyed_account = &keyed_accounts[dst_keyed_account_index];
let src_keyed_account = account.borrow();
if message.is_writable(src_keyed_account_index, demote_sysvar_write_locks)
&& !src_keyed_account.executable()
{
if message.is_writable(src_keyed_account_index) && !src_keyed_account.executable() {
if dst_keyed_account.data_len()? != src_keyed_account.data().len()
&& dst_keyed_account.data_len()? != 0
{
@@ -937,13 +930,8 @@ impl MessageProcessor {
)?;
// Construct keyed accounts
let keyed_accounts = Self::create_keyed_accounts(
message,
instruction,
executable_accounts,
accounts,
invoke_context.is_feature_active(&demote_sysvar_write_locks::id()),
);
let keyed_accounts =
Self::create_keyed_accounts(message, instruction, executable_accounts, accounts);
// Invoke callee
invoke_context.push(program_id, &keyed_accounts)?;
@@ -1013,7 +1001,6 @@ impl MessageProcessor {
accounts: &[Rc<RefCell<AccountSharedData>>],
rent: &Rent,
timings: &mut ExecuteDetailsTimings,
demote_sysvar_write_locks: bool,
logger: Rc<RefCell<dyn Logger>>,
) -> Result<(), InstructionError> {
// Verify all executable accounts have zero outstanding refs
@@ -1034,7 +1021,7 @@ impl MessageProcessor {
pre_accounts[unique_index]
.verify(
program_id,
message.is_writable(account_index, demote_sysvar_write_locks),
message.is_writable(account_index),
rent,
&account,
timings,
@@ -1074,7 +1061,6 @@ impl MessageProcessor {
rent: &Rent,
caller_write_privileges: Option<&[bool]>,
timings: &mut ExecuteDetailsTimings,
demote_sysvar_write_locks: bool,
logger: Rc<RefCell<dyn Logger>>,
) -> Result<(), InstructionError> {
// Verify the per-account instruction results
@@ -1086,7 +1072,7 @@ impl MessageProcessor {
let is_writable = if let Some(caller_write_privileges) = caller_write_privileges {
caller_write_privileges[account_index]
} else {
message.is_writable(account_index, demote_sysvar_write_locks)
message.is_writable(account_index)
};
// Find the matching PreAccount
for pre_account in pre_accounts.iter_mut() {
@@ -1145,7 +1131,6 @@ impl MessageProcessor {
feature_set: Arc<FeatureSet>,
bpf_compute_budget: BpfComputeBudget,
timings: &mut ExecuteDetailsTimings,
demote_sysvar_write_locks: bool,
account_db: Arc<Accounts>,
ancestors: &Ancestors,
) -> Result<(), InstructionError> {
@@ -1191,7 +1176,6 @@ impl MessageProcessor {
accounts,
&rent_collector.rent,
timings,
demote_sysvar_write_locks,
invoke_context.get_logger(),
)?;
@@ -1221,7 +1205,6 @@ impl MessageProcessor {
account_db: Arc<Accounts>,
ancestors: &Ancestors,
) -> Result<(), TransactionError> {
let demote_sysvar_write_locks = feature_set.is_active(&demote_sysvar_write_locks::id());
for (instruction_index, instruction) in message.instructions.iter().enumerate() {
let instruction_recorder = instruction_recorders
.as_ref()
@@ -1240,7 +1223,6 @@ impl MessageProcessor {
feature_set.clone(),
bpf_compute_budget,
timings,
demote_sysvar_write_locks,
account_db.clone(),
ancestors,
)
@@ -2245,13 +2227,11 @@ mod tests {
);
// not owned account modified by the caller (before the invoke)
let demote_sysvar_write_locks =
invoke_context.is_feature_active(&demote_sysvar_write_locks::id());
let caller_write_privileges = message
.account_keys
.iter()
.enumerate()
.map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks))
.map(|(i, _)| message.is_writable(i))
.collect::<Vec<bool>>();
accounts[0].borrow_mut().data_as_mut_slice()[0] = 1;
assert_eq!(
@@ -2307,7 +2287,7 @@ mod tests {
.account_keys
.iter()
.enumerate()
.map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks))
.map(|(i, _)| message.is_writable(i))
.collect::<Vec<bool>>();
assert_eq!(
MessageProcessor::process_cross_program_instruction(