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

Validate account locks on buffering #34229

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Validate account locks on buffering #34229

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
26 changes: 25 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 @@ -18,7 +18,9 @@ use {
crossbeam_channel::RecvTimeoutError,
solana_measure::measure_us,
solana_runtime::bank_forks::BankForks,
solana_sdk::{saturating_add_assign, timing::AtomicInterval},
solana_sdk::{
saturating_add_assign, timing::AtomicInterval, transaction::SanitizedTransaction,
},
std::{
sync::{Arc, RwLock},
time::Duration,
Expand Down Expand Up @@ -199,6 +201,7 @@ impl SchedulerController {
// Sanitize packets, generate IDs, and insert into the container.
let bank = self.bank_forks.read().unwrap().working_bank();
let last_slot_in_epoch = bank.epoch_schedule().get_last_slot_in_epoch(bank.epoch());
let transaction_account_lock_limit = bank.get_transaction_account_lock_limit();
let feature_set = &bank.feature_set;
let vote_only = bank.vote_only_bank();
for packet in packets {
Expand All @@ -209,6 +212,18 @@ impl SchedulerController {
continue;
};

// Check transaction does not have too many or duplicate locks.
// If it does, transaction is not valid and should be dropped here.
if SanitizedTransaction::validate_account_locks(
transaction.message(),
transaction_account_lock_limit,
)
.is_err()
{
saturating_add_assign!(self.count_metrics.num_dropped_on_validate_locks, 1);
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps move this all the way up to ImmutableDeserializedPacket::new() or somewhere during packet receiving to drop bad packets before inserting into storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done before we insert to storage.
We can't do it in ImmutableDeserializedPacket::new because we haven't resolved look up tables at that point.
This is immediately after we resolve look up tables, which I think is the earliest we can do this.

Potentially, it could/should be done in SanitizedTransaction::try_new but that's changing the definition of a SanitizedTransaction. Not sure we want to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I got confused by thread-local-MI and central-scheduler code. So this is somewhat parallel to unprocessed_transactino_storage::consume_scan_should_process_packet() where immmutable_deserialized_packet is sanitized then accounts locks checked, which should be OK then.

Leave SanitizedTransaction to solana-transaction-types crate :D, there maybe can add a type for "sanitized and account locks validated" type


let transaction_id = self.transaction_id_generator.next();
let transaction_ttl = SanitizedTransactionTTL {
transaction,
Expand Down Expand Up @@ -247,6 +262,8 @@ struct SchedulerCountMetrics {
num_dropped_on_receive: usize,
/// Number of transactions that were dropped due to sanitization failure.
num_dropped_on_sanitization: usize,
/// Number of transactions that were dropped due to failed lock validation.
num_dropped_on_validate_locks: usize,
/// Number of transactions that were dropped due to clearing.
num_dropped_on_clear: usize,
/// Number of transactions that were dropped due to exceeded capacity.
Expand Down Expand Up @@ -278,6 +295,11 @@ impl SchedulerCountMetrics {
self.num_dropped_on_sanitization,
i64
),
(
"num_dropped_on_validate_locks",
self.num_dropped_on_validate_locks,
i64
),
("num_dropped_on_clear", self.num_dropped_on_clear, i64),
("num_dropped_on_capacity", self.num_dropped_on_capacity, i64)
);
Expand All @@ -291,6 +313,7 @@ impl SchedulerCountMetrics {
|| self.num_retryable != 0
|| self.num_dropped_on_receive != 0
|| self.num_dropped_on_sanitization != 0
|| self.num_dropped_on_validate_locks != 0
|| self.num_dropped_on_clear != 0
|| self.num_dropped_on_capacity != 0
}
Expand All @@ -303,6 +326,7 @@ impl SchedulerCountMetrics {
self.num_retryable = 0;
self.num_dropped_on_receive = 0;
self.num_dropped_on_sanitization = 0;
self.num_dropped_on_validate_locks = 0;
self.num_dropped_on_clear = 0;
self.num_dropped_on_capacity = 0;
}
Expand Down