Skip to content
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ impl TransactionViewReceiveAndBuffer {
transaction_account_lock_limit: usize,
) -> Result<TransactionViewState, PacketHandlingError> {
// Parsing and basic sanitization checks
let Ok(view) = SanitizedTransactionView::try_new_sanitized(bytes) else {
let Ok(view) = SanitizedTransactionView::try_new_sanitized(bytes, working_bank.feature_set.is_active(&agave_feature_set::static_instruction_limit::id())) else {
return Err(PacketHandlingError::Sanitization);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ mod tests {

let reserved_addresses = HashSet::default();
let packet_parser = |data, priority, cost| {
let view = SanitizedTransactionView::try_new_sanitized(data).unwrap();
let view = SanitizedTransactionView::try_new_sanitized(data, true).unwrap();
let view = RuntimeTransaction::<SanitizedTransactionView<_>>::try_from(
view,
MessageHash::Compute,
Expand Down
27 changes: 15 additions & 12 deletions core/src/forwarding_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,19 +324,22 @@ impl<VoteClient: ForwardingClient, NonVoteClient: ForwardingClient>

// Perform basic sanitization checks and calculate priority.
// If any steps fail, drop the packet.
let Some(priority) = SanitizedTransactionView::try_new_sanitized(packet_data)
let Some(priority) = SanitizedTransactionView::try_new_sanitized(
packet_data,
bank.feature_set
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's move this out of the loop, otherwise it looks it up everytime

.is_active(&agave_feature_set::static_instruction_limit::id()),
)
.map_err(|_| ())
.and_then(|transaction| {
RuntimeTransaction::<SanitizedTransactionView<_>>::try_from(
transaction,
MessageHash::Compute,
Some(packet.meta().is_simple_vote_tx()),
)
.map_err(|_| ())
.and_then(|transaction| {
RuntimeTransaction::<SanitizedTransactionView<_>>::try_from(
transaction,
MessageHash::Compute,
Some(packet.meta().is_simple_vote_tx()),
)
.map_err(|_| ())
})
.ok()
.and_then(|transaction| calculate_priority(&transaction, bank))
else {
})
.ok()
.and_then(|transaction| calculate_priority(&transaction, bank)) else {
self.metrics.votes_dropped_on_receive += vote_count;
self.metrics.non_votes_dropped_on_receive += non_vote_count;
continue;
Expand Down
8 changes: 8 additions & 0 deletions feature-set/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,10 @@ pub mod enforce_fixed_fec_set {
solana_pubkey::declare_id!("fixfecLZYMfkGzwq6NJA11Yw6KYztzXiK9QcL3K78in");
}

pub mod static_instruction_limit {
solana_pubkey::declare_id!("64ixypL1HPu8WtJhNSMb9mSgfFaJvsANuRkTbHyuLfnx");
}

pub static FEATURE_NAMES: LazyLock<AHashMap<Pubkey, &'static str>> = LazyLock::new(|| {
[
(secp256k1_program_enabled::id(), "secp256k1 program"),
Expand Down Expand Up @@ -2035,6 +2039,10 @@ pub static FEATURE_NAMES: LazyLock<AHashMap<Pubkey, &'static str>> = LazyLock::n
enforce_fixed_fec_set::id(),
"SIMD-0317: Enforce 32 data + 32 coding shreds",
),
(
static_instruction_limit::id(),
"SIMD-0160: static instruction limit",
),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down
120 changes: 87 additions & 33 deletions perf/src/sigverify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ struct PacketOffsets {
pub msg_start: u32,
pub pubkey_start: u32,
pub pubkey_len: u32,
pub instruction_len: u32,
pub instruction_start: u32,
}

impl PacketOffsets {
Expand All @@ -54,13 +56,17 @@ impl PacketOffsets {
msg_start: u32,
pubkey_start: u32,
pubkey_len: u32,
instruction_len: u32,
instruction_start: u32,
) -> Self {
Self {
sig_len,
sig_start,
msg_start,
pubkey_start,
pubkey_len,
instruction_len,
instruction_start,
}
}
}
Expand Down Expand Up @@ -261,7 +267,7 @@ fn do_get_packet_offsets(
.checked_add(pubkey_len_size)
.ok_or(PacketError::InvalidPubkeyLen)?;

let _ = pubkey_len
let blockhash_offset = pubkey_len
.checked_mul(size_of::<Pubkey>())
.and_then(|v| v.checked_add(pubkey_start))
.filter(|v| *v <= packet.meta().size)
Expand All @@ -271,6 +277,37 @@ fn do_get_packet_offsets(
return Err(PacketError::InvalidPubkeyLen);
}

// read instruction length
let instructions_len_offset = blockhash_offset
.checked_add(size_of::<Hash>())
.ok_or(PacketError::InvalidLen)?;

// Packet should have at least 1 more byte for instructions.len
let _ = instructions_len_offset
.checked_add(1usize)
.filter(|v| *v <= packet.meta().size)
.ok_or(PacketError::InvalidLen)?;

let (instruction_len, instruction_len_size) = packet
.data(instructions_len_offset..)
.and_then(|bytes| decode_shortu16_len(bytes).ok())
.ok_or(PacketError::InvalidLen)?;

// SIMD-0160: skip if has more than 64 top instructions
if instruction_len > 64 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is unconditional. i'm like 99% sure this fn is only run for BP, where that would be fine, but we should verify that's the case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

BP == "block producer/packer"? sigverify only feed to leaders, it does not impact consensus, if that is what you mean. I'm thinking changes to sigverify can be o nits own PR without feature gate.

return Err(PacketError::InvalidProgramLen);
}

let instruction_start = instructions_len_offset
.checked_add(instruction_len_size)
.ok_or(PacketError::InvalidLen)?;

// Packet should have at least 1 more byte for one instructions_program_id
let _ = instruction_start
.checked_add(1usize)
.filter(|v| *v <= packet.meta().size)
.ok_or(PacketError::InvalidLen)?;

let sig_start = current_offset
.checked_add(sig_size)
.ok_or(PacketError::InvalidLen)?;
Expand All @@ -280,13 +317,18 @@ fn do_get_packet_offsets(
let pubkey_start = current_offset
.checked_add(pubkey_start)
.ok_or(PacketError::InvalidLen)?;
let instruction_start = current_offset
.checked_add(instruction_start)
.ok_or(PacketError::InvalidLen)?;

Ok(PacketOffsets::new(
u32::try_from(sig_len_untrusted)?,
u32::try_from(sig_start)?,
u32::try_from(msg_start)?,
u32::try_from(pubkey_start)?,
u32::try_from(pubkey_len)?,
u32::try_from(instruction_len)?,
u32::try_from(instruction_start)?,
))
}

Expand All @@ -303,7 +345,7 @@ fn get_packet_offsets(
}
}
// force sigverify to fail by returning zeros
PacketOffsets::new(0, 0, 0, 0, 0)
PacketOffsets::new(0, 0, 0, 0, 0, 0, 0)
}

fn check_for_simple_vote_transaction(
Expand All @@ -330,35 +372,13 @@ fn check_for_simple_vote_transaction(
.checked_sub(current_offset)
.ok_or(PacketError::InvalidLen)?;

let instructions_len_offset = (packet_offsets.pubkey_len as usize)
.checked_mul(size_of::<Pubkey>())
.and_then(|v| v.checked_add(pubkey_start))
.and_then(|v| v.checked_add(size_of::<Hash>()))
.ok_or(PacketError::InvalidLen)?;

// Packet should have at least 1 more byte for instructions.len
let _ = instructions_len_offset
.checked_add(1usize)
.filter(|v| *v <= packet.meta().size)
.ok_or(PacketError::InvalidLen)?;

let (instruction_len, instruction_len_size) = packet
.data(instructions_len_offset..)
.and_then(|bytes| decode_shortu16_len(bytes).ok())
.ok_or(PacketError::InvalidLen)?;
// skip if has more than 1 instruction
if instruction_len != 1 {
if packet_offsets.instruction_len != 1 {
return Err(PacketError::InvalidProgramLen);
}

let instruction_start = instructions_len_offset
.checked_add(instruction_len_size)
.ok_or(PacketError::InvalidLen)?;

// Packet should have at least 1 more byte for one instructions_program_id
let _ = instruction_start
.checked_add(1usize)
.filter(|v| *v <= packet.meta().size)
let instruction_start = (packet_offsets.instruction_start as usize)
.checked_sub(current_offset)
.ok_or(PacketError::InvalidLen)?;

let instruction_program_id_index: usize = usize::from(
Expand Down Expand Up @@ -681,7 +701,9 @@ mod tests {
PACKETS_PER_BATCH,
},
sigverify::{self, PacketOffsets},
test_tx::{new_test_vote_tx, test_multisig_tx, test_tx},
test_tx::{
new_test_tx_with_number_of_ixs, new_test_vote_tx, test_multisig_tx, test_tx,
},
},
bincode::{deserialize, serialize},
bytes::{BufMut, Bytes, BytesMut},
Expand All @@ -691,11 +713,12 @@ mod tests {
solana_message::{compiled_instruction::CompiledInstruction, Message, MessageHeader},
solana_signature::Signature,
solana_signer::Signer,
solana_transaction::Transaction,
solana_transaction::{versioned::VersionedTransaction, Transaction},
std::{
iter::repeat_with,
sync::atomic::{AtomicU64, Ordering},
},
test_case::test_case,
};

const SIG_OFFSET: usize = 1;
Expand Down Expand Up @@ -996,6 +1019,7 @@ mod tests {
let offsets = sigverify::do_get_packet_offsets(packet.as_ref(), 0).unwrap();
let expected_offsets = {
legacy_offsets.pubkey_start += 1;
legacy_offsets.instruction_start += 1;
legacy_offsets
};

Expand Down Expand Up @@ -1033,30 +1057,32 @@ mod tests {
packet_offsets.msg_start - packet_offsets.sig_start,
packet_offsets.pubkey_start - packet_offsets.msg_start,
packet_offsets.pubkey_len,
packet_offsets.instruction_len,
packet_offsets.instruction_start - packet_offsets.pubkey_start,
)
}

#[test]
fn test_get_packet_offsets() {
assert_eq!(
get_packet_offsets_from_tx(test_tx(), 0),
PacketOffsets::new(1, 1, 64, 4, 2)
PacketOffsets::new(1, 1, 64, 4, 2, 1, 97)
);
assert_eq!(
get_packet_offsets_from_tx(test_tx(), 100),
PacketOffsets::new(1, 1, 64, 4, 2)
PacketOffsets::new(1, 1, 64, 4, 2, 1, 97)
);

// Ensure we're not indexing packet by the `current_offset` parameter.
assert_eq!(
get_packet_offsets_from_tx(test_tx(), 1_000_000),
PacketOffsets::new(1, 1, 64, 4, 2)
PacketOffsets::new(1, 1, 64, 4, 2, 1, 97)
);

// Ensure we're returning sig_len, not sig_size.
assert_eq!(
get_packet_offsets_from_tx(test_multisig_tx(), 0),
PacketOffsets::new(2, 1, 128, 4, 4)
PacketOffsets::new(2, 1, 128, 4, 4, 1, 161)
);
}

Expand Down Expand Up @@ -1799,4 +1825,32 @@ mod tests {
]
)
}

#[test_case(false, false; "ok_ixs_legacy")]
#[test_case(true, false; "too_many_ixs_legacy")]
#[test_case(false, true; "ok_ixs_versioned")]
#[test_case(true, true; "too_many_ixs_versioned")]
fn test_number_of_instructions(too_many_ixs: bool, is_versioned_tx: bool) {
let mut number_of_ixs = 64;
if too_many_ixs {
number_of_ixs += 1;
}

let packet = if is_versioned_tx {
let tx: VersionedTransaction = new_test_tx_with_number_of_ixs(number_of_ixs);
BytesPacket::from_data(None, tx.clone()).unwrap()
} else {
let tx: Transaction = new_test_tx_with_number_of_ixs(number_of_ixs);
BytesPacket::from_data(None, tx.clone()).unwrap()
};

if too_many_ixs {
assert_eq!(
do_get_packet_offsets(packet.as_ref(), 0),
Err(PacketError::InvalidProgramLen),
);
} else {
assert!(do_get_packet_offsets(packet.as_ref(), 0).is_ok());
}
}
}
51 changes: 49 additions & 2 deletions perf/src/test_tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ use {
solana_clock::Slot,
solana_hash::Hash,
solana_keypair::Keypair,
solana_message::compiled_instruction::CompiledInstruction,
solana_message::{
compiled_instruction::CompiledInstruction, v0::Message as MessageV0, AccountMeta,
Instruction, Message, VersionedMessage,
},
solana_pubkey::Pubkey,
solana_sdk_ids::{stake, system_program},
solana_signer::Signer,
solana_system_interface::instruction::SystemInstruction,
solana_transaction::Transaction,
solana_transaction::{versioned::VersionedTransaction, Transaction},
solana_vote::vote_transaction,
solana_vote_program::vote_state::TowerSync,
};
Expand Down Expand Up @@ -69,3 +73,46 @@ where
switch_proof_hash,
)
}

pub fn new_test_tx_with_number_of_ixs<T>(number_of_ixs: usize) -> T
where
T: FromTestTx,
{
let program_id = Pubkey::new_unique();
let account = Pubkey::new_unique();

let mut instructions = Vec::with_capacity(number_of_ixs);
for i in 0..number_of_ixs {
instructions.push(Instruction {
program_id,
accounts: vec![AccountMeta::new(account, false)],
data: vec![i as u8],
});
}

let payer = Keypair::new();
let blockhash = Hash::new_unique();

T::from_test_tx(&payer, blockhash, instructions)
}

/// Trait that unifies building legacy vs v0 transactions
pub trait FromTestTx: Sized {
fn from_test_tx(payer: &Keypair, blockhash: Hash, ixs: Vec<Instruction>) -> Self;
}

impl FromTestTx for Transaction {
fn from_test_tx(payer: &Keypair, blockhash: Hash, ixs: Vec<Instruction>) -> Self {
let msg = Message::new(&ixs, Some(&payer.pubkey()));
Transaction::new(&[payer], msg, blockhash)
}
}

impl FromTestTx for VersionedTransaction {
fn from_test_tx(payer: &Keypair, _blockhash: Hash, ixs: Vec<Instruction>) -> Self {
let msg_v0 =
MessageV0::try_compile(&payer.pubkey(), &ixs, &[], Hash::new_unique()).unwrap();
let versioned = VersionedMessage::V0(msg_v0);
VersionedTransaction::try_new(versioned, &[payer]).unwrap()
}
}
Loading