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

v1.17: Discard packets statically known to fail (backport of #370) #374

Merged
merged 2 commits into from
Mar 21, 2024
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
52 changes: 51 additions & 1 deletion core/src/banking_stage/immutable_deserialized_packet.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use {
solana_cost_model::block_cost_limits::BUILT_IN_INSTRUCTION_COSTS,
solana_perf::packet::Packet,
solana_runtime::transaction_priority_details::{
GetTransactionPriorityDetails, TransactionPriorityDetails,
Expand All @@ -8,6 +9,7 @@ use {
hash::Hash,
message::Message,
sanitize::SanitizeError,
saturating_add_assign,
short_vec::decode_shortu16_len,
signature::Signature,
transaction::{
Expand Down Expand Up @@ -96,6 +98,22 @@ impl ImmutableDeserializedPacket {
self.priority_details.compute_unit_limit
}

/// Returns true if the transaction's compute unit limit is at least as
/// large as the sum of the static builtins' costs.
/// This is a simple sanity check so the leader can discard transactions
/// which are statically known to exceed the compute budget, and will
/// result in no useful state-change.
pub fn compute_unit_limit_above_static_builtins(&self) -> bool {
let mut static_builtin_cost_sum: u64 = 0;
for (program_id, _) in self.transaction.get_message().program_instructions_iter() {
if let Some(ix_cost) = BUILT_IN_INSTRUCTION_COSTS.get(program_id) {
saturating_add_assign!(static_builtin_cost_sum, *ix_cost);
}
}

self.compute_unit_limit() >= static_builtin_cost_sum
}

// This function deserializes packets into transactions, computes the blake3 hash of transaction
// messages, and verifies secp256k1 instructions.
pub fn build_sanitized_transaction(
Expand Down Expand Up @@ -148,7 +166,10 @@ fn packet_message(packet: &Packet) -> Result<&[u8], DeserializedPacketError> {
mod tests {
use {
super::*,
solana_sdk::{signature::Keypair, system_transaction},
solana_sdk::{
compute_budget, instruction::Instruction, pubkey::Pubkey, signature::Keypair,
signer::Signer, system_instruction, system_transaction, transaction::Transaction,
},
};

#[test]
Expand All @@ -164,4 +185,33 @@ mod tests {

assert!(deserialized_packet.is_ok());
}

#[test]
fn compute_unit_limit_above_static_builtins() {
// Cases:
// 1. compute_unit_limit under static builtins
// 2. compute_unit_limit equal to static builtins
// 3. compute_unit_limit above static builtins
for (cu_limit, expectation) in [(250, false), (300, true), (350, true)] {
let keypair = Keypair::new();
let bpf_program_id = Pubkey::new_unique();
let ixs = vec![
system_instruction::transfer(&keypair.pubkey(), &Pubkey::new_unique(), 1),
compute_budget::ComputeBudgetInstruction::set_compute_unit_limit(cu_limit),
Instruction::new_with_bytes(bpf_program_id, &[], vec![]), // non-builtin - not counted in filter
];
let tx = Transaction::new_signed_with_payer(
&ixs,
Some(&keypair.pubkey()),
&[&keypair],
Hash::new_unique(),
);
let packet = Packet::from_data(None, tx).unwrap();
let deserialized_packet = ImmutableDeserializedPacket::new(packet).unwrap();
assert_eq!(
deserialized_packet.compute_unit_limit_above_static_builtins(),
expectation
);
}
}
}
13 changes: 11 additions & 2 deletions core/src/banking_stage/packet_deserializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ impl PacketDeserializer {
&self,
recv_timeout: Duration,
capacity: usize,
packet_filter: impl Fn(&ImmutableDeserializedPacket) -> bool,
) -> Result<ReceivePacketResults, RecvTimeoutError> {
let (packet_count, packet_batches) = self.receive_until(recv_timeout, capacity)?;

Expand All @@ -62,6 +63,7 @@ impl PacketDeserializer {
packet_count,
&packet_batches,
round_compute_unit_price_enabled,
&packet_filter,
))
}

Expand All @@ -71,6 +73,7 @@ impl PacketDeserializer {
packet_count: usize,
banking_batches: &[BankingPacketBatch],
round_compute_unit_price_enabled: bool,
packet_filter: &impl Fn(&ImmutableDeserializedPacket) -> bool,
) -> ReceivePacketResults {
let mut passed_sigverify_count: usize = 0;
let mut failed_sigverify_count: usize = 0;
Expand All @@ -88,6 +91,7 @@ impl PacketDeserializer {
packet_batch,
&packet_indexes,
round_compute_unit_price_enabled,
packet_filter,
));
}

Expand Down Expand Up @@ -158,13 +162,16 @@ impl PacketDeserializer {
packet_batch: &'a PacketBatch,
packet_indexes: &'a [usize],
round_compute_unit_price_enabled: bool,
packet_filter: &'a (impl Fn(&ImmutableDeserializedPacket) -> bool + 'a),
) -> impl Iterator<Item = ImmutableDeserializedPacket> + 'a {
packet_indexes.iter().filter_map(move |packet_index| {
let mut packet_clone = packet_batch[*packet_index].clone();
packet_clone
.meta_mut()
.set_round_compute_unit_price(round_compute_unit_price_enabled);
ImmutableDeserializedPacket::new(packet_clone).ok()
ImmutableDeserializedPacket::new(packet_clone)
.ok()
.filter(packet_filter)
})
}
}
Expand All @@ -186,7 +193,7 @@ mod tests {

#[test]
fn test_deserialize_and_collect_packets_empty() {
let results = PacketDeserializer::deserialize_and_collect_packets(0, &[], false);
let results = PacketDeserializer::deserialize_and_collect_packets(0, &[], false, &|_| true);
assert_eq!(results.deserialized_packets.len(), 0);
assert!(results.new_tracer_stats_option.is_none());
assert_eq!(results.passed_sigverify_count, 0);
Expand All @@ -204,6 +211,7 @@ mod tests {
packet_count,
&[BankingPacketBatch::new((packet_batches, None))],
false,
&|_| true,
);
assert_eq!(results.deserialized_packets.len(), 2);
assert!(results.new_tracer_stats_option.is_none());
Expand All @@ -223,6 +231,7 @@ mod tests {
packet_count,
&[BankingPacketBatch::new((packet_batches, None))],
false,
&|_| true,
);
assert_eq!(results.deserialized_packets.len(), 1);
assert!(results.new_tracer_stats_option.is_none());
Expand Down
1 change: 1 addition & 0 deletions core/src/banking_stage/packet_receiver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ impl PacketReceiver {
.receive_packets(
recv_timeout,
unprocessed_transaction_storage.max_receive_size(),
|packet| packet.compute_unit_limit_above_static_builtins(),
)
// Consumes results if Ok, otherwise we keep the Err
.map(|receive_packet_results| {
Expand Down
Loading