Skip to content
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: 2 additions & 0 deletions banks-server/src/banks_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ fn simulate_transaction(
bank.get_reserved_account_keys(),
bank.feature_set
.is_active(&agave_feature_set::static_instruction_limit::id()),
bank.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::id()),
) {
Err(err) => {
return BanksTransactionResultWithSimulation {
Expand Down
19 changes: 16 additions & 3 deletions core/src/banking_stage/consume_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -769,13 +769,17 @@ pub(crate) mod external {
let enable_static_instruction_limit = bank
.feature_set
.is_active(&agave_feature_set::static_instruction_limit::ID);
let enable_instruction_accounts_limit = bank
.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::ID);
let mut parsing_results = Vec::with_capacity(MAX_TRANSACTIONS_PER_MESSAGE);
let mut parsed_transactions = Vec::with_capacity(MAX_TRANSACTIONS_PER_MESSAGE);
for (tx_ptr, _) in batch.iter() {
// Parsing and basic sanitization checks
match SanitizedTransactionView::try_new_sanitized(
tx_ptr,
enable_static_instruction_limit,
enable_instruction_accounts_limit,
) {
Ok(view) => {
parsing_results.push(Ok(()));
Expand Down Expand Up @@ -959,6 +963,9 @@ pub(crate) mod external {
let enable_static_instruction_limit = bank
.feature_set
.is_active(&agave_feature_set::static_instruction_limit::ID);
let enable_instruction_accounts_limit = bank
.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::ID);
let transaction_account_lock_limit = bank.get_transaction_account_lock_limit();

let mut translation_results = Vec::with_capacity(MAX_TRANSACTIONS_PER_MESSAGE);
Expand All @@ -970,6 +977,7 @@ pub(crate) mod external {
bank,
enable_static_instruction_limit,
transaction_account_lock_limit,
enable_instruction_accounts_limit,
) {
Ok((tx, max_age)) => {
transactions.push(tx);
Expand All @@ -988,12 +996,14 @@ pub(crate) mod external {
bank: &Bank,
enable_static_instruction_limit: bool,
transaction_account_lock_limit: usize,
enable_instruction_accounts_limit: bool,
) -> Result<(Tx, MaxAge), PacketHandlingError> {
translate_to_runtime_view(
transaction_ptr,
bank,
enable_static_instruction_limit,
transaction_account_lock_limit,
enable_instruction_accounts_limit,
)
.map(|(view, deactivation_slot)| {
(
Expand Down Expand Up @@ -1177,6 +1187,7 @@ pub(crate) mod external {
&bank,
true,
bank.get_transaction_account_lock_limit(),
true,
)
.ok()
.unwrap()
Expand Down Expand Up @@ -1364,8 +1375,8 @@ pub(crate) mod external {

let parsing_results = [Ok(()), Err(TransactionViewError::ParseError), Ok(())];
let parsed_transactions = [
SanitizedTransactionView::try_new_sanitized(&tx1[..], true).unwrap(),
SanitizedTransactionView::try_new_sanitized(&tx2[..], true).unwrap(),
SanitizedTransactionView::try_new_sanitized(&tx1[..], true, true).unwrap(),
SanitizedTransactionView::try_new_sanitized(&tx2[..], true, true).unwrap(),
];
bank.store_account(
&parsed_transactions[1].static_account_keys()[0],
Expand Down Expand Up @@ -1415,7 +1426,7 @@ pub(crate) mod external {
) -> RuntimeTransaction<ResolvedTransactionView<&'_ [u8]>> {
RuntimeTransaction::<ResolvedTransactionView<_>>::try_new(
RuntimeTransaction::<SanitizedTransactionView<_>>::try_new(
SanitizedTransactionView::try_new_sanitized(tx, true).unwrap(),
SanitizedTransactionView::try_new_sanitized(tx, true, true).unwrap(),
solana_transaction::sanitized::MessageHash::Compute,
Some(false),
)
Expand Down Expand Up @@ -2565,6 +2576,8 @@ mod tests {
&HashSet::default(),
bank.feature_set
.is_active(&agave_feature_set::static_instruction_limit::id()),
bank.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::id()),
)
.unwrap()
};
Expand Down
2 changes: 2 additions & 0 deletions core/src/banking_stage/consumer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1443,6 +1443,8 @@ mod tests {
&ReservedAccountKeys::empty_key_set(),
bank.feature_set
.is_active(&agave_feature_set::static_instruction_limit::id()),
bank.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::id()),
)
.unwrap();
let batch_transactions_inner = [&sanitized_tx]
Expand Down
1 change: 1 addition & 0 deletions core/src/banking_stage/latest_validator_vote_packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ impl LatestValidatorVote {
let vote = SanitizedTransactionView::try_new_sanitized(
std::sync::Arc::new(packet.data(..).unwrap().to_vec()),
false,
true,
)
.unwrap();

Expand Down
15 changes: 12 additions & 3 deletions core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ impl TransactionViewReceiveAndBuffer {
let enable_static_instruction_limit = root_bank
.feature_set
.is_active(&agave_feature_set::static_instruction_limit::ID);
let enable_instruction_accounts_limit = root_bank
.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::ID);
let transaction_account_lock_limit = working_bank.get_transaction_account_lock_limit();

// Create temporary batches of transactions to be age-checked.
Expand Down Expand Up @@ -347,6 +350,7 @@ impl TransactionViewReceiveAndBuffer {
working_bank,
enable_static_instruction_limit,
transaction_account_lock_limit,
enable_instruction_accounts_limit,
) {
Ok(state) => Ok(state),
Err(
Expand Down Expand Up @@ -407,12 +411,14 @@ impl TransactionViewReceiveAndBuffer {
working_bank: &Bank,
enable_static_instruction_limit: bool,
transaction_account_lock_limit: usize,
enable_instruction_accounts_limit: bool,
) -> Result<TransactionViewState, PacketHandlingError> {
let (view, deactivation_slot) = translate_to_runtime_view(
bytes,
root_bank,
enable_static_instruction_limit,
transaction_account_lock_limit,
enable_instruction_accounts_limit,
)?;

let Ok(compute_budget_limits) = view
Expand All @@ -438,11 +444,14 @@ pub(crate) fn translate_to_runtime_view<D: TransactionData>(
bank: &Bank,
enable_static_instruction_limit: bool,
transaction_account_lock_limit: usize,
enable_instruction_accounts_limit: bool,
) -> Result<(RuntimeTransaction<ResolvedTransactionView<D>>, u64), PacketHandlingError> {
// Parsing and basic sanitization checks
let Ok(view) =
SanitizedTransactionView::try_new_sanitized(data, enable_static_instruction_limit)
else {
let Ok(view) = SanitizedTransactionView::try_new_sanitized(
data,
enable_static_instruction_limit,
enable_instruction_accounts_limit,
) else {
return Err(PacketHandlingError::Sanitization);
};

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

let reserved_addresses = HashSet::default();
let packet_parser = |data, priority, cost| {
let view = SanitizedTransactionView::try_new_sanitized(data, true).unwrap();
let view = SanitizedTransactionView::try_new_sanitized(data, true, true).unwrap();
let view = RuntimeTransaction::<SanitizedTransactionView<_>>::try_new(
view,
MessageHash::Compute,
Expand Down
3 changes: 3 additions & 0 deletions core/src/banking_stage/vote_packet_receiver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ impl VotePacketReceiver {
// NB: It's safe to always pass false in here as simple vote
// transactions are guaranteed to be a single instruction.
false,
// Vote instructions are created in the validator code, and they are not
// referencing more than 255 accounts, so it is safe to set this to true.
true,
Comment on lines +113 to +115
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Even though true is the correct choice here, I think the comment is misleading.

Sure, Vote instructions are typically created by validator software, but there's nothing that says they have to be. The simple vote transaction filtering doesn't know where the transaction originated and it also doesn't enforce a specific number of accounts. For the latter reason, though, true is the right setting here.

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.

it also doesn't enforce a specific number of accounts.

I didn't follow. If it does not enforce a specific number of accounts, then, theoretically, it could receive instructions with more than 255 accounts, right?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's correct. If someone crafted a "simple vote" transaction on their own, outside validator software, they could include > 255 accounts and it would still be filtered as a simple vote transaction. This is why I said using true is correct.

Your comment says that the transactions are created in the validator code, but that's not guaranteed.

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.

What about changing the comment to "Vote instructions do not need more than 255 accounts, and the vote transaction filtering does not enforce that, so enforcing the constraint here is right" ?

) {
Ok(pkt) => Some(pkt),
Err(err) => {
Expand Down
2 changes: 1 addition & 1 deletion core/src/banking_stage/vote_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ pub(crate) mod tests {
}

fn to_sanitized_view(packet: BytesPacket) -> SanitizedTransactionView<SharedBytes> {
SanitizedTransactionView::try_new_sanitized(Arc::new(packet.buffer().to_vec()), false)
SanitizedTransactionView::try_new_sanitized(Arc::new(packet.buffer().to_vec()), false, true)
.unwrap()
}

Expand Down
9 changes: 6 additions & 3 deletions core/src/banking_stage/vote_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -587,9 +587,12 @@ mod tests {
};

fn to_runtime_transaction_view(packet: BytesPacket) -> RuntimeTransactionView {
let tx =
SanitizedTransactionView::try_new_sanitized(Arc::new(packet.buffer().to_vec()), false)
.unwrap();
let tx = SanitizedTransactionView::try_new_sanitized(
Arc::new(packet.buffer().to_vec()),
false,
true,
)
.unwrap();
let tx = RuntimeTransaction::<SanitizedTransactionView<_>>::try_new(
tx,
MessageHash::Compute,
Expand Down
4 changes: 4 additions & 0 deletions core/src/forwarding_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ impl<VoteClient: ForwardingClient, NonVoteClient: ForwardingClient>
let enable_static_instruction_limit = bank
.feature_set
.is_active(&agave_feature_set::static_instruction_limit::id());
let enable_instruction_accounts_limit = bank
.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::id());
for batch in packet_batches.iter() {
for packet in batch
.iter()
Expand All @@ -317,6 +320,7 @@ impl<VoteClient: ForwardingClient, NonVoteClient: ForwardingClient>
let Some(priority) = SanitizedTransactionView::try_new_sanitized(
packet_data,
enable_static_instruction_limit,
enable_instruction_accounts_limit,
)
.map_err(|_| ())
.and_then(|transaction| {
Expand Down
2 changes: 2 additions & 0 deletions cost-model/src/transaction_cost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@ mod tests {
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
true,
true,
)
.unwrap();

Expand Down Expand Up @@ -430,6 +431,7 @@ mod tests {
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
true,
true,
)
.unwrap();

Expand Down
1 change: 1 addition & 0 deletions entry/benches/entry_sigverify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ fn bench_cpusigverify(bencher: &mut Bencher) {
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
true,
true,
)
}?;

Expand Down
1 change: 1 addition & 0 deletions entry/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ mod tests {
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
true,
true,
)
}?;

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 @@ -1287,6 +1287,10 @@ pub mod stop_use_static_simple_vote_tx_cost {
solana_pubkey::declare_id!("NSVt1s8oP1A9NjEc6UNcj2voeCcfzHaq4jZTiUL2Mf5");
}

pub mod limit_instruction_accounts {
solana_pubkey::declare_id!("DqbnFPASg7tHmZ6qfpdrt2M6MWoSeiicWPXxPhxqFCQ");
}

pub static FEATURE_NAMES: LazyLock<AHashMap<Pubkey, &'static str>> = LazyLock::new(|| {
[
(secp256k1_program_enabled::id(), "secp256k1 program"),
Expand Down Expand Up @@ -2298,6 +2302,10 @@ pub static FEATURE_NAMES: LazyLock<AHashMap<Pubkey, &'static str>> = LazyLock::n
stop_use_static_simple_vote_tx_cost::id(),
"stop use static SimpleVote transaction cost, issue #10227",
),
(
limit_instruction_accounts::id(),
"SIMD-406: Maximum instruction accounts",
),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down
1 change: 1 addition & 0 deletions ledger-tool/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ fn compute_slot_cost(
SimpleAddressLoader::Disabled,
&reserved_account_keys.active,
feature_set.is_active(&agave_feature_set::static_instruction_limit::id()),
feature_set.is_active(&agave_feature_set::limit_instruction_accounts::id()),
)
.map_err(|err| {
warn!("Failed to compute cost of transaction: {err:?}");
Expand Down
9 changes: 9 additions & 0 deletions rpc/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3837,6 +3837,9 @@ pub mod rpc_full {
preflight_bank
.feature_set
.is_active(&agave_feature_set::static_instruction_limit::id()),
preflight_bank
.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::id()),
)?;
let blockhash = *transaction.message().recent_blockhash();
let message_hash = *transaction.message_hash();
Expand Down Expand Up @@ -3999,6 +4002,8 @@ pub mod rpc_full {
bank.get_reserved_account_keys(),
bank.feature_set
.is_active(&agave_feature_set::static_instruction_limit::id()),
bank.feature_set
.is_active(&agave_feature_set::limit_instruction_accounts::id()),
)?;

let verification_error = if sig_verify {
Expand Down Expand Up @@ -4411,6 +4416,7 @@ fn sanitize_transaction(
address_loader: impl AddressLoader,
reserved_account_keys: &HashSet<Pubkey>,
enable_static_instruction_limit: bool,
enable_instruction_accounts_limit: bool,
) -> Result<RuntimeTransaction<SanitizedTransaction>> {
RuntimeTransaction::try_create(
transaction,
Expand All @@ -4419,6 +4425,7 @@ fn sanitize_transaction(
address_loader,
reserved_account_keys,
enable_static_instruction_limit,
enable_instruction_accounts_limit,
)
.map_err(|err| Error::invalid_params(format!("invalid transaction: {err}")))
}
Expand Down Expand Up @@ -9211,6 +9218,7 @@ pub mod tests {
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
true,
true,
)
.unwrap_err(),
expect58
Expand All @@ -9237,6 +9245,7 @@ pub mod tests {
SimpleAddressLoader::Disabled,
&ReservedAccountKeys::empty_key_set(),
true,
true,
)
.unwrap_err(),
Error::invalid_params(
Expand Down
Loading
Loading