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

separate functions for different signature types #4354

Merged
merged 4 commits into from
Jan 16, 2025
Merged

separate functions for different signature types #4354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a69f77
separate functions for different signature types
apfitzge Jan 3, 2025
7362ecd
calculate_fee_details_internal
apfitzge Jan 15, 2025
52cfd85
tests
apfitzge Jan 15, 2025
10e3213
calculate_signature_fee
apfitzge Jan 15, 2025
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
4 changes: 2 additions & 2 deletions cost-model/src/transaction_cost.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@ pub struct WritableKeysTransaction(pub Vec<Pubkey>);

#[cfg(feature = "dev-context-only-utils")]
impl solana_svm_transaction::svm_message::SVMMessage for WritableKeysTransaction {
fn num_total_signatures(&self) -> u64 {
unimplemented!("WritableKeysTransaction::num_total_signatures")
fn num_transaction_signatures(&self) -> u64 {
unimplemented!("WritableKeysTransaction::num_transaction_signatures")
}

fn num_write_locks(&self) -> u64 {
Expand Down
86 changes: 82 additions & 4 deletions fee/src/lib.rs
View file
Open in desktop
Copy link
Author

Choose a reason for hiding this comment

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

@jstarry thinking to make the fee functions above take something where SignatureCounts: From<&T>.
This gives the functions a bit more flexibility, since we should be able to use this function for types that do not implement SVMMessage. Ideally we'd use this same function for estimating priority before we resolve ALTs (which SVMMessage requires from us); and could just implement the correct From here.

If fee model changed and we needed additional information from the type, this also gives a bit more flexibility than just taking SignatureCounts as the argument directly, as we can just add additional bounds:

pub fn calculate_fee_details<'a, T>(message:T,...) -> FeeDetails 
where
	SignatureCounts: From<&'a T>,
    OtherInformation: From<&'a T>,
{}

I think the alternative is basically just us doing this a few times for different types we want to calculate fees for, which seems worse to me than using From traits. let me know what you think of the approach.

Copy link

Choose a reason for hiding this comment

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

Maybe in a follow up? I'm happy with the current PR. I'm just not sure how useful it is to have priority before we resolve ALTs in the scheduler..

Copy link
Author

Choose a reason for hiding this comment

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

It's useful because if below the current minimum priority, I won't resolve at all.
Less useful in scheduler, more useful in forwarding stage (wip) where the buffer is significantly smaller

Copy link
Author

Choose a reason for hiding this comment

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

also just to expand on this, the medium-term plan is to move prioritization into sigverify stage. but we don't want SV reaching into accounts-db at all, obviously. so we'd want a way to calculate fees just from an unresolved view (essentially to what i mentioned for forwarding stage now).

Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,91 @@ pub fn calculate_fee_details(
if zero_fees_for_test {
return FeeDetails::default();
}
let signature_fee = message
.num_total_signatures()
.saturating_mul(lamports_per_signature);

FeeDetails::new(
signature_fee,
calculate_signature_fee(SignatureCounts::from(message), lamports_per_signature),
prioritization_fee,
remove_rounding_in_fee_calculation,
)
}

/// Calculate fees from signatures.
fn calculate_signature_fee(
SignatureCounts {
num_transaction_signatures,
num_ed25519_signatures,
num_secp256k1_signatures,
}: SignatureCounts,
lamports_per_signature: u64,
) -> u64 {
let signature_count = num_transaction_signatures
.saturating_add(num_ed25519_signatures)
.saturating_add(num_secp256k1_signatures);

signature_count.saturating_mul(lamports_per_signature)
}

struct SignatureCounts {
Copy link
Author

Choose a reason for hiding this comment

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

see follow-up PR in #4472 which adds the secpr1 signature count behind a feature-gate.

pub num_transaction_signatures: u64,
pub num_ed25519_signatures: u64,
pub num_secp256k1_signatures: u64,
}

impl<Tx: SVMMessage> From<&Tx> for SignatureCounts {
fn from(message: &Tx) -> Self {
Self {
num_transaction_signatures: message.num_transaction_signatures(),
num_ed25519_signatures: message.num_ed25519_signatures(),
num_secp256k1_signatures: message.num_secp256k1_signatures(),
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_calculate_signature_fee() {
const LAMPORTS_PER_SIGNATURE: u64 = 5_000;

// Impossible case - 0 signatures.
assert_eq!(
calculate_signature_fee(
SignatureCounts {
num_transaction_signatures: 0,
num_ed25519_signatures: 0,
num_secp256k1_signatures: 0,
},
LAMPORTS_PER_SIGNATURE,
),
0
);

// Simple signature
assert_eq!(
calculate_signature_fee(
SignatureCounts {
num_transaction_signatures: 1,
num_ed25519_signatures: 0,
num_secp256k1_signatures: 0,
},
LAMPORTS_PER_SIGNATURE,
),
LAMPORTS_PER_SIGNATURE
);

// Pre-compile signatures.
assert_eq!(
calculate_signature_fee(
SignatureCounts {
num_transaction_signatures: 1,
num_ed25519_signatures: 2,
num_secp256k1_signatures: 3,
},
LAMPORTS_PER_SIGNATURE,
),
6 * LAMPORTS_PER_SIGNATURE
);
}
}
21 changes: 19 additions & 2 deletions runtime-transaction/src/runtime_transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,26 @@ impl<T> Deref for RuntimeTransaction<T> {
}

impl<T: SVMMessage> SVMMessage for RuntimeTransaction<T> {
fn num_transaction_signatures(&self) -> u64 {
self.transaction.num_transaction_signatures()
}
// override to access from the cached meta instead of re-calculating
fn num_ed25519_signatures(&self) -> u64 {
self.meta
.signature_details
.num_ed25519_instruction_signatures()
}
// override to access from the cached meta instead of re-calculating
fn num_total_signatures(&self) -> u64 {
self.meta.signature_details.total_signatures()
fn num_secp256k1_signatures(&self) -> u64 {
self.meta
.signature_details
.num_secp256k1_instruction_signatures()
}
// override to access form the cached meta instead of re-calculating
fn num_secp256r1_signatures(&self) -> u64 {
self.meta
.signature_details
.num_secp256r1_instruction_signatures()
}

fn num_write_locks(&self) -> u64 {
Expand Down
30 changes: 25 additions & 5 deletions svm-transaction/src/svm_message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use {
solana_hash::Hash,
solana_message::AccountKeys,
solana_pubkey::Pubkey,
solana_sdk_ids::system_program,
solana_sdk_ids::{ed25519_program, secp256k1_program, secp256r1_program, system_program},
};

mod sanitized_message;
Expand All @@ -21,10 +21,20 @@ const NONCED_TX_MARKER_IX_INDEX: u8 = 0;

// - Debug to support legacy logging
pub trait SVMMessage: Debug {
/// Returns the total number of signatures in the message.
/// This includes required transaction signatures as well as any
/// pre-compile signatures that are attached in instructions.
fn num_total_signatures(&self) -> u64;
/// Return the number of transaction-level signatures in the message.
fn num_transaction_signatures(&self) -> u64;
/// Return the number of ed25519 precompile signatures in the message.
fn num_ed25519_signatures(&self) -> u64 {
default_precompile_signature_count(&ed25519_program::ID, self.program_instructions_iter())
}
/// Return the number of secp256k1 precompile signatures in the message.
fn num_secp256k1_signatures(&self) -> u64 {
default_precompile_signature_count(&secp256k1_program::ID, self.program_instructions_iter())
}
/// Return the number of secp256r1 precompile signatures in the message.
fn num_secp256r1_signatures(&self) -> u64 {
default_precompile_signature_count(&secp256r1_program::ID, self.program_instructions_iter())
}

/// Returns the number of requested write-locks in this message.
/// This does not consider if write-locks are demoted.
Expand Down Expand Up @@ -125,3 +135,13 @@ pub trait SVMMessage: Debug {
/// Get message address table lookups used in the message
fn message_address_table_lookups(&self) -> impl Iterator<Item = SVMMessageAddressTableLookup>;
}

fn default_precompile_signature_count<'a>(
precompile: &Pubkey,
instructions: impl Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)>,
) -> u64 {
instructions
.filter(|(program_id, _)| *program_id == precompile)
.map(|(_, ix)| u64::from(ix.data.first().copied().unwrap_or(0)))
.sum()
}
4 changes: 2 additions & 2 deletions svm-transaction/src/svm_message/sanitized_message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use {

// Implement for the "reference" `SanitizedMessage` type.
impl SVMMessage for SanitizedMessage {
fn num_total_signatures(&self) -> u64 {
SanitizedMessage::num_total_signatures(self)
fn num_transaction_signatures(&self) -> u64 {
u64::from(self.header().num_required_signatures)
}

fn num_write_locks(&self) -> u64 {
Expand Down
4 changes: 2 additions & 2 deletions svm-transaction/src/svm_message/sanitized_transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use {
};

impl SVMMessage for SanitizedTransaction {
fn num_total_signatures(&self) -> u64 {
SVMMessage::num_total_signatures(SanitizedTransaction::message(self))
fn num_transaction_signatures(&self) -> u64 {
SVMMessage::num_transaction_signatures(SanitizedTransaction::message(self))
}

fn num_write_locks(&self) -> u64 {
Expand Down
42 changes: 4 additions & 38 deletions transaction-view/src/resolved_transaction_view.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@ use {
ops::Deref,
},
solana_hash::Hash,
solana_message::{v0::LoadedAddresses, AccountKeys, TransactionSignatureDetails},
solana_message::{v0::LoadedAddresses, AccountKeys},
solana_pubkey::Pubkey,
solana_sdk_ids::{
bpf_loader_upgradeable, ed25519_program, secp256k1_program, secp256r1_program,
},
solana_sdk_ids::bpf_loader_upgradeable,
solana_signature::Signature,
solana_svm_transaction::{
instruction::SVMInstruction, message_address_table_lookup::SVMMessageAddressTableLookup,
Expand Down Expand Up @@ -159,43 +157,11 @@ impl<D: TransactionData> ResolvedTransactionView<D> {
pub fn loaded_addresses(&self) -> Option<&LoadedAddresses> {
self.resolved_addresses.as_ref()
}

fn signature_details(&self) -> TransactionSignatureDetails {
// counting the number of pre-processor operations separately
let mut num_secp256k1_instruction_signatures: u64 = 0;
let mut num_ed25519_instruction_signatures: u64 = 0;
let mut num_secp256r1_instruction_signatures: u64 = 0;
for (program_id, instruction) in self.program_instructions_iter() {
if secp256k1_program::check_id(program_id) {
if let Some(num_verifies) = instruction.data.first() {
num_secp256k1_instruction_signatures =
num_secp256k1_instruction_signatures.wrapping_add(u64::from(*num_verifies));
}
} else if ed25519_program::check_id(program_id) {
if let Some(num_verifies) = instruction.data.first() {
num_ed25519_instruction_signatures =
num_ed25519_instruction_signatures.wrapping_add(u64::from(*num_verifies));
}
} else if secp256r1_program::check_id(program_id) {
if let Some(num_verifies) = instruction.data.first() {
num_secp256r1_instruction_signatures =
num_secp256r1_instruction_signatures.wrapping_add(u64::from(*num_verifies));
}
}
}

TransactionSignatureDetails::new(
u64::from(self.view.num_required_signatures()),
num_secp256k1_instruction_signatures,
num_ed25519_instruction_signatures,
num_secp256r1_instruction_signatures,
)
}
}

impl<D: TransactionData> SVMMessage for ResolvedTransactionView<D> {
fn num_total_signatures(&self) -> u64 {
self.signature_details().total_signatures()
fn num_transaction_signatures(&self) -> u64 {
u64::from(self.view.num_required_signatures())
}

fn num_write_locks(&self) -> u64 {
Expand Down
Loading