Skip to content
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.

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

Clean up demote program write lock feature #21949

Merged
merged 2 commits into from
Dec 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 cli-output/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,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
3 changes: 1 addition & 2 deletions core/src/banking_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,8 +935,7 @@ impl BankingStage {
gossip_vote_sender: &ReplayVoteSender,
qos_service: &Arc<QosService>,
) -> (Result<usize, PohRecorderError>, Vec<usize>) {
let tx_costs =
qos_service.compute_transaction_costs(txs.iter(), bank.demote_program_write_locks());
let tx_costs = qos_service.compute_transaction_costs(txs.iter());

let transactions_qos_results =
qos_service.select_transactions_per_cost(txs.iter(), tx_costs.iter(), bank);
Expand Down
19 changes: 7 additions & 12 deletions core/src/qos_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,12 @@ impl QosService {
pub fn compute_transaction_costs<'a>(
&self,
transactions: impl Iterator<Item = &'a SanitizedTransaction>,
demote_program_write_locks: bool,
) -> Vec<TransactionCost> {
let mut compute_cost_time = Measure::start("compute_cost_time");
let cost_model = self.cost_model.read().unwrap();
let txs_costs: Vec<_> = transactions
.map(|tx| {
let cost = cost_model.calculate_cost(tx, demote_program_write_locks);
let cost = cost_model.calculate_cost(tx);
debug!(
"transaction {:?}, cost {:?}, cost sum {}",
tx,
Expand Down Expand Up @@ -261,7 +260,7 @@ mod tests {

let cost_model = Arc::new(RwLock::new(CostModel::default()));
let qos_service = QosService::new(cost_model.clone());
let txs_costs = qos_service.compute_transaction_costs(txs.iter(), false);
let txs_costs = qos_service.compute_transaction_costs(txs.iter());

// verify the size of txs_costs and its contents
assert_eq!(txs_costs.len(), txs.len());
Expand All @@ -271,11 +270,7 @@ mod tests {
.map(|(index, cost)| {
assert_eq!(
cost.sum(),
cost_model
.read()
.unwrap()
.calculate_cost(&txs[index], false)
.sum()
cost_model.read().unwrap().calculate_cost(&txs[index]).sum()
);
})
.collect_vec();
Expand Down Expand Up @@ -306,14 +301,14 @@ mod tests {
let transfer_tx_cost = cost_model
.read()
.unwrap()
.calculate_cost(&transfer_tx, false)
.calculate_cost(&transfer_tx)
.sum();

// make a vec of txs
let txs = vec![transfer_tx.clone(), vote_tx.clone(), transfer_tx, vote_tx];

let qos_service = QosService::new(cost_model);
let txs_costs = qos_service.compute_transaction_costs(txs.iter(), false);
let txs_costs = qos_service.compute_transaction_costs(txs.iter());

// set cost tracker limit to fit 1 transfer tx, vote tx bypasses limit check
let cost_limit = transfer_tx_cost;
Expand Down Expand Up @@ -359,7 +354,7 @@ mod tests {
.name("test-producer-1".to_string())
.spawn(move || {
debug!("thread 1 starts with {} txs", txs_1.len());
let tx_costs = qos_service_1.compute_transaction_costs(txs_1.iter(), false);
let tx_costs = qos_service_1.compute_transaction_costs(txs_1.iter());
assert_eq!(txs_count, tx_costs.len());
debug!(
"thread 1 done, generated {} count, see service count as {}",
Expand All @@ -376,7 +371,7 @@ mod tests {
.name("test-producer-2".to_string())
.spawn(move || {
debug!("thread 2 starts with {} txs", txs_2.len());
let tx_costs = qos_service_2.compute_transaction_costs(txs_2.iter(), false);
let tx_costs = qos_service_2.compute_transaction_costs(txs_2.iter());
assert_eq!(txs_count, tx_costs.len());
debug!(
"thread 2 done, generated {} count, see service count as {}",
Expand Down
5 changes: 1 addition & 4 deletions ledger-tool/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -800,10 +800,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String>
.for_each(|transaction| {
num_programs += transaction.message().instructions().len();

let tx_cost = cost_model.calculate_cost(
&transaction,
true, // demote_program_write_locks
);
let tx_cost = cost_model.calculate_cost(&transaction);
let result = cost_tracker.try_add(&transaction, &tx_cost);
if result.is_err() {
println!(
Expand Down
29 changes: 8 additions & 21 deletions program-runtime/src/invoke_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ use {
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
compute_budget::ComputeBudget,
feature_set::{
demote_program_write_locks, do_support_realloc, neon_evm_compute_budget,
reject_empty_instruction_without_program, remove_native_loader, requestable_heap_size,
tx_wide_compute_cap, FeatureSet,
do_support_realloc, neon_evm_compute_budget, reject_empty_instruction_without_program,
remove_native_loader, requestable_heap_size, tx_wide_compute_cap, FeatureSet,
},
hash::Hash,
instruction::{AccountMeta, CompiledInstruction, Instruction, InstructionError},
Expand Down Expand Up @@ -282,9 +281,6 @@ impl<'a> InvokeContext<'a> {
}

// Create the KeyedAccounts that will be passed to the program
let demote_program_write_locks = self
.feature_set
.is_active(&demote_program_write_locks::id());
let keyed_accounts = program_indices
.iter()
.map(|account_index| {
Expand All @@ -304,7 +300,7 @@ impl<'a> InvokeContext<'a> {
};
(
message.is_signer(index_in_instruction),
message.is_writable(index_in_instruction, demote_program_write_locks),
message.is_writable(index_in_instruction),
&self.accounts[account_index].0,
&self.accounts[account_index].1 as &RefCell<AccountSharedData>,
)
Expand Down Expand Up @@ -336,9 +332,6 @@ impl<'a> InvokeContext<'a> {
program_indices: &[usize],
) -> Result<(), InstructionError> {
let program_id = instruction.program_id(&message.account_keys);
let demote_program_write_locks = self
.feature_set
.is_active(&demote_program_write_locks::id());
let do_support_realloc = self.feature_set.is_active(&do_support_realloc::id());

// Verify all executable accounts have zero outstanding refs
Expand All @@ -364,7 +357,7 @@ impl<'a> InvokeContext<'a> {
pre_account
.verify(
program_id,
message.is_writable(account_index, demote_program_write_locks),
message.is_writable(account_index),
&self.rent,
&account,
&mut self.timings,
Expand Down Expand Up @@ -667,11 +660,8 @@ impl<'a> InvokeContext<'a> {
if is_lowest_invocation_level {
self.verify(message, instruction, program_indices)
} else {
let demote_program_write_locks = self
.feature_set
.is_active(&demote_program_write_locks::id());
let write_privileges: Vec<bool> = (0..message.account_keys.len())
.map(|i| message.is_writable(i, demote_program_write_locks))
.map(|i| message.is_writable(i))
.collect();
self.verify_and_update(instruction, account_indices, &write_privileges)
}
Expand Down Expand Up @@ -1143,7 +1133,7 @@ mod tests {
None,
);
let write_privileges: Vec<bool> = (0..message.account_keys.len())
.map(|i| message.is_writable(i, /*demote_program_write_locks=*/ true))
.map(|i| message.is_writable(i))
.collect();

// modify account owned by the program
Expand Down Expand Up @@ -1265,14 +1255,11 @@ mod tests {
.unwrap();

// not owned account modified by the caller (before the invoke)
let demote_program_write_locks = invoke_context
.feature_set
.is_active(&demote_program_write_locks::id());
let caller_write_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>>();
accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1;
assert_eq!(
Expand Down Expand Up @@ -1326,7 +1313,7 @@ mod tests {
.account_keys
.iter()
.enumerate()
.map(|(i, _)| message.is_writable(i, demote_program_write_locks))
.map(|(i, _)| message.is_writable(i))
.collect::<Vec<bool>>();
assert_eq!(
invoke_context.process_instruction(
Expand Down
8 changes: 2 additions & 6 deletions program-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use {
compute_budget::ComputeBudget,
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 @@ -244,15 +243,12 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
let message = Message::new(&[instruction.clone()], None);
let program_id_index = message.instructions[0].program_id_index as usize;
let program_id = message.account_keys[program_id_index];
let demote_program_write_locks = invoke_context
.feature_set
.is_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(&log_collector, &program_id, invoke_context.invoke_depth());
Expand Down Expand Up @@ -280,7 +276,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
account.set_executable(account_info.executable);
account.set_rent_epoch(account_info.rent_epoch);
}
let account_info = if message.is_writable(i, demote_program_write_locks) {
let account_info = if message.is_writable(i) {
Some(account_info)
} else {
None
Expand Down
14 changes: 5 additions & 9 deletions programs/bpf_loader/src/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,10 @@ use {
entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, SUCCESS},
epoch_schedule::EpochSchedule,
feature_set::{
blake3_syscall_enabled, demote_program_write_locks, disable_fees_sysvar,
do_support_realloc, fixed_memcpy_nonoverlapping_check,
libsecp256k1_0_5_upgrade_enabled, prevent_calling_precompiles_as_programs,
return_data_syscall_enabled, secp256k1_recover_syscall_enabled,
sol_log_data_syscall_enabled,
blake3_syscall_enabled, disable_fees_sysvar, do_support_realloc,
fixed_memcpy_nonoverlapping_check, libsecp256k1_0_5_upgrade_enabled,
prevent_calling_precompiles_as_programs, return_data_syscall_enabled,
secp256k1_recover_syscall_enabled, sol_log_data_syscall_enabled,
},
hash::{Hasher, HASH_BYTES},
instruction::{AccountMeta, Instruction, InstructionError},
Expand Down Expand Up @@ -2198,9 +2197,6 @@ fn get_translated_accounts<'a, T, F>(
where
F: Fn(&T, &InvokeContext) -> Result<CallerAccount<'a>, EbpfError<BpfError>>,
{
let demote_program_write_locks = invoke_context
.feature_set
.is_active(&demote_program_write_locks::id());
let keyed_accounts = invoke_context
.get_instruction_keyed_accounts()
.map_err(SyscallError::InstructionError)?;
Expand Down Expand Up @@ -2229,7 +2225,7 @@ where
account.set_executable(caller_account.executable);
account.set_rent_epoch(caller_account.rent_epoch);
}
let caller_account = if message.is_writable(i, demote_program_write_locks) {
let caller_account = if message.is_writable(i) {
if let Some(orig_data_len_index) = keyed_accounts
.iter()
.position(|keyed_account| keyed_account.unsigned_key() == account_key)
Expand Down
3 changes: 1 addition & 2 deletions rpc/src/transaction_status_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ impl TransactionStatusService {
transaction.message(),
lamports_per_signature,
);
let tx_account_locks =
transaction.get_account_locks(bank.demote_program_write_locks());
let tx_account_locks = transaction.get_account_locks();

let inner_instructions = inner_instructions.map(|inner_instructions| {
inner_instructions
Expand Down
Loading