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.

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

Use reserved account keys list to restrict tx write locks #541

Merged
merged 3 commits into from
Apr 13, 2024
Merged

Use reserved account keys list to restrict tx write locks #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bad8a10
Track reserved account keys in bank
jstarry Apr 12, 2024
51e49c6
Plumb through reserved account keys set
jstarry Apr 2, 2024
fd03ab9
Plumb through tests
jstarry Apr 2, 2024
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
6 changes: 5 additions & 1 deletion banks-server/src/banks_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ fn simulate_transaction(
MessageHash::Compute,
Some(false), // is_simple_vote_tx
bank,
bank.get_reserved_account_keys(),
) {
Err(err) => {
return BanksTransactionResultWithSimulation {
Expand Down Expand Up @@ -332,6 +333,7 @@ impl Banks for BanksServer {
MessageHash::Compute,
Some(false), // is_simple_vote_tx
bank.as_ref(),
bank.get_reserved_account_keys(),
) {
Ok(tx) => tx,
Err(err) => return Some(Err(err)),
Expand Down Expand Up @@ -417,7 +419,9 @@ impl Banks for BanksServer {
commitment: CommitmentLevel,
) -> Option<u64> {
let bank = self.bank(commitment);
let sanitized_message = SanitizedMessage::try_from_legacy_message(message).ok()?;
let sanitized_message =
SanitizedMessage::try_from_legacy_message(message, bank.get_reserved_account_keys())
.ok()?;
bank.get_fee_for_message(&sanitized_message)
}
}
Expand Down
2 changes: 2 additions & 0 deletions core/src/banking_stage/consumer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ mod tests {
nonce_account::verify_nonce_account,
poh_config::PohConfig,
pubkey::Pubkey,
reserved_account_keys::ReservedAccountKeys,
signature::Keypair,
signer::Signer,
system_instruction, system_program, system_transaction,
Expand Down Expand Up @@ -2026,6 +2027,7 @@ mod tests {
MessageHash::Compute,
Some(false),
bank.as_ref(),
&ReservedAccountKeys::empty_key_set(),
)
.unwrap();

Expand Down
5 changes: 4 additions & 1 deletion core/src/banking_stage/immutable_deserialized_packet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use {
feature_set,
hash::Hash,
message::Message,
pubkey::Pubkey,
sanitize::SanitizeError,
saturating_add_assign,
short_vec::decode_shortu16_len,
Expand All @@ -15,7 +16,7 @@ use {
VersionedTransaction,
},
},
std::{cmp::Ordering, mem::size_of, sync::Arc},
std::{cmp::Ordering, collections::HashSet, mem::size_of, sync::Arc},
thiserror::Error,
};

Expand Down Expand Up @@ -123,6 +124,7 @@ impl ImmutableDeserializedPacket {
feature_set: &Arc<feature_set::FeatureSet>,
votes_only: bool,
address_loader: impl AddressLoader,
reserved_account_keys: &HashSet<Pubkey>,
) -> Option<SanitizedTransaction> {
if votes_only && !self.is_simple_vote() {
return None;
Expand All @@ -132,6 +134,7 @@ impl ImmutableDeserializedPacket {
*self.message_hash(),
self.is_simple_vote(),
address_loader,
reserved_account_keys,
)
.ok()?;
tx.verify_precompiles(feature_set).ok()?;
Expand Down
1 change: 1 addition & 0 deletions core/src/banking_stage/latest_unprocessed_votes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ impl LatestUnprocessedVotes {
&bank.feature_set,
bank.vote_only_bank(),
bank.as_ref(),
bank.get_reserved_account_keys(),
)
{
if forward_packet_batches_by_accounts.try_add_packet(
Expand Down
1 change: 1 addition & 0 deletions core/src/banking_stage/read_write_account_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ mod tests {
MessageHash::Compute,
Some(false),
bank,
bank.get_reserved_account_keys(),
)
.unwrap()
}
Expand Down
7 changes: 6 additions & 1 deletion core/src/banking_stage/transaction_scheduler/scheduler_controller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,12 @@ impl SchedulerController {
let (transactions, fee_budget_limits_vec): (Vec<_>, Vec<_>) = chunk
.iter()
.filter_map(|packet| {
packet.build_sanitized_transaction(feature_set, vote_only, bank.as_ref())
packet.build_sanitized_transaction(
feature_set,
vote_only,
bank.as_ref(),
bank.get_reserved_account_keys(),
)
})
.inspect(|_| saturating_add_assign!(post_sanitization_count, 1))
.filter(|tx| {
Expand Down
7 changes: 7 additions & 0 deletions core/src/banking_stage/unprocessed_packet_batches.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ mod tests {
solana_sdk::{
compute_budget::ComputeBudgetInstruction,
message::Message,
reserved_account_keys::ReservedAccountKeys,
signature::{Keypair, Signer},
system_instruction, system_transaction,
transaction::{SimpleAddressLoader, Transaction},
Expand Down Expand Up @@ -490,6 +491,7 @@ mod tests {
&Arc::new(FeatureSet::default()),
votes_only,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
});
assert_eq!(2, txs.count());
Expand All @@ -500,6 +502,7 @@ mod tests {
&Arc::new(FeatureSet::default()),
votes_only,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
});
assert_eq!(0, txs.count());
Expand All @@ -519,6 +522,7 @@ mod tests {
&Arc::new(FeatureSet::default()),
votes_only,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
});
assert_eq!(3, txs.count());
Expand All @@ -529,6 +533,7 @@ mod tests {
&Arc::new(FeatureSet::default()),
votes_only,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
});
assert_eq!(2, txs.count());
Expand All @@ -548,6 +553,7 @@ mod tests {
&Arc::new(FeatureSet::default()),
votes_only,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
});
assert_eq!(3, txs.count());
Expand All @@ -558,6 +564,7 @@ mod tests {
&Arc::new(FeatureSet::default()),
votes_only,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
});
assert_eq!(3, txs.count());
Expand Down
17 changes: 13 additions & 4 deletions core/src/banking_stage/unprocessed_transaction_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,13 @@ fn consume_scan_should_process_packet(
}

// Try to sanitize the packet
let (maybe_sanitized_transaction, sanitization_time_us) = measure_us!(
packet.build_sanitized_transaction(&bank.feature_set, bank.vote_only_bank(), bank)
);
let (maybe_sanitized_transaction, sanitization_time_us) = measure_us!(packet
.build_sanitized_transaction(
&bank.feature_set,
bank.vote_only_bank(),
bank,
bank.get_reserved_account_keys(),
));

payload
.slot_metrics_tracker
Expand Down Expand Up @@ -770,7 +774,12 @@ impl ThreadLocalUnprocessedPackets {
.enumerate()
.filter_map(|(packet_index, deserialized_packet)| {
deserialized_packet
.build_sanitized_transaction(&bank.feature_set, bank.vote_only_bank(), bank)
.build_sanitized_transaction(
&bank.feature_set,
bank.vote_only_bank(),
bank,
bank.get_reserved_account_keys(),
)
Comment on lines +777 to +782
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidenote: not a thing to be dealt with in this PR, but it seems really clunky that this method takes 4 different bank-related things, including the Bank itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, hurts my eyes every time too.

.map(|transaction| (transaction, packet_index))
})
.unzip();
Expand Down
2 changes: 2 additions & 0 deletions cost-model/src/cost_tracker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ mod tests {
crate::transaction_cost::*,
solana_sdk::{
hash::Hash,
reserved_account_keys::ReservedAccountKeys,
signature::{Keypair, Signer},
system_transaction,
transaction::{
Expand Down Expand Up @@ -401,6 +402,7 @@ mod tests {
MessageHash::Compute,
Some(true),
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
.unwrap();

Expand Down
3 changes: 3 additions & 0 deletions cost-model/src/transaction_cost.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ mod tests {
feature_set::FeatureSet,
hash::Hash,
message::SimpleAddressLoader,
reserved_account_keys::ReservedAccountKeys,
signer::keypair::Keypair,
transaction::{MessageHash, SanitizedTransaction, VersionedTransaction},
},
Expand Down Expand Up @@ -231,6 +232,7 @@ mod tests {
MessageHash::Compute,
Some(true),
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
.unwrap();

Expand All @@ -240,6 +242,7 @@ mod tests {
MessageHash::Compute,
Some(false),
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
.unwrap();

Expand Down
3 changes: 3 additions & 0 deletions entry/benches/entry_sigverify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use {
solana_perf::test_tx::test_tx,
solana_sdk::{
hash::Hash,
reserved_account_keys::ReservedAccountKeys,
transaction::{
Result, SanitizedTransaction, SimpleAddressLoader, TransactionVerificationMode,
VersionedTransaction,
Expand Down Expand Up @@ -41,6 +42,7 @@ fn bench_gpusigverify(bencher: &mut Bencher) {
message_hash,
None,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
}?;

Expand Down Expand Up @@ -84,6 +86,7 @@ fn bench_cpusigverify(bencher: &mut Bencher) {
message_hash,
None,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
}?;

Expand Down
2 changes: 2 additions & 0 deletions entry/src/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,7 @@ mod tests {
solana_sdk::{
hash::{hash, Hash},
pubkey::Pubkey,
reserved_account_keys::ReservedAccountKeys,
signature::{Keypair, Signer},
system_transaction,
transaction::{
Expand Down Expand Up @@ -1084,6 +1085,7 @@ mod tests {
message_hash,
None,
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
)
}?;

Expand Down
7 changes: 6 additions & 1 deletion ledger-tool/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ use {
native_token::{lamports_to_sol, sol_to_lamports, Sol},
pubkey::Pubkey,
rent::Rent,
reserved_account_keys::ReservedAccountKeys,
shred_version::compute_shred_version,
stake::{self, state::StakeStateV2},
system_program,
Expand Down Expand Up @@ -461,6 +462,9 @@ fn compute_slot_cost(
let mut program_ids = HashMap::new();
let mut cost_tracker = CostTracker::default();

let feature_set = FeatureSet::all_enabled();
let reserved_account_keys = ReservedAccountKeys::new_all_activated();

for entry in entries {
num_transactions += entry.transactions.len();
entry
Expand All @@ -472,6 +476,7 @@ fn compute_slot_cost(
MessageHash::Compute,
None,
SimpleAddressLoader::Disabled,
&reserved_account_keys.active,
)
.map_err(|err| {
warn!("Failed to compute cost of transaction: {:?}", err);
Expand All @@ -481,7 +486,7 @@ fn compute_slot_cost(
.for_each(|transaction| {
num_programs += transaction.message().instructions().len();

let tx_cost = CostModel::calculate_cost(&transaction, &FeatureSet::all_enabled());
let tx_cost = CostModel::calculate_cost(&transaction, &feature_set);
let result = cost_tracker.try_add(&tx_cost);
if result.is_err() {
println!(
Expand Down
19 changes: 16 additions & 3 deletions programs/sbf/tests/programs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ use {
message::Message,
pubkey::Pubkey,
rent::Rent,
reserved_account_keys::ReservedAccountKeys,
signature::{Keypair, Signer},
system_program,
transaction::{SanitizedTransaction, Transaction, TransactionError},
Expand Down Expand Up @@ -201,7 +202,11 @@ fn execute_transactions(
}
.expect("lamports_per_signature must be available");
let fee = bank.get_fee_for_message_with_lamports_per_signature(
&SanitizedMessage::try_from_legacy_message(tx.message().clone()).unwrap(),
&SanitizedMessage::try_from_legacy_message(
tx.message().clone(),
&ReservedAccountKeys::empty_key_set(),
)
.unwrap(),
lamports_per_signature,
);

Expand Down Expand Up @@ -3706,7 +3711,11 @@ fn test_program_fees() {
Some(&mint_keypair.pubkey()),
);

let sanitized_message = SanitizedMessage::try_from_legacy_message(message.clone()).unwrap();
let sanitized_message = SanitizedMessage::try_from_legacy_message(
message.clone(),
&ReservedAccountKeys::empty_key_set(),
)
.unwrap();
let expected_normal_fee = fee_structure.calculate_fee(
&sanitized_message,
congestion_multiplier,
Expand All @@ -3730,7 +3739,11 @@ fn test_program_fees() {
],
Some(&mint_keypair.pubkey()),
);
let sanitized_message = SanitizedMessage::try_from_legacy_message(message.clone()).unwrap();
let sanitized_message = SanitizedMessage::try_from_legacy_message(
message.clone(),
&ReservedAccountKeys::empty_key_set(),
)
.unwrap();
let expected_prioritized_fee = fee_structure.calculate_fee(
&sanitized_message,
congestion_multiplier,
Expand Down
Loading
Loading