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

generic verify_precompiles #3055

Merged
merged 4 commits into from
Oct 4, 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion banks-server/src/banks_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use {
bank::{Bank, TransactionSimulationResult},
bank_forks::BankForks,
commitment::BlockCommitmentCache,
verify_precompiles::verify_precompiles,
},
solana_sdk::{
account::Account,
Expand Down Expand Up @@ -167,7 +168,7 @@ fn verify_transaction(
let move_precompile_verification_to_svm =
feature_set.is_active(&move_precompile_verification_to_svm::id());
if !move_precompile_verification_to_svm {
transaction.verify_precompiles(feature_set)?;
verify_precompiles(transaction, feature_set)?;
}

Ok(())
Expand Down
5 changes: 3 additions & 2 deletions core/src/banking_stage/consumer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use {
solana_runtime::{
bank::{Bank, LoadAndExecuteTransactionsOutput},
transaction_batch::TransactionBatch,
verify_precompiles::verify_precompiles,
},
solana_runtime_transaction::instructions_processor::process_compute_budget_instructions,
solana_sdk::{
Expand Down Expand Up @@ -401,7 +402,7 @@ impl Consumer {
.map(|(tx, result)| match result {
Ok(_) => {
if !move_precompile_verification_to_svm {
tx.verify_precompiles(&bank.feature_set)
verify_precompiles(tx, &bank.feature_set)
} else {
Ok(())
}
Expand Down Expand Up @@ -452,7 +453,7 @@ impl Consumer {
} else {
// Verify pre-compiles.
if !move_precompile_verification_to_svm {
tx.verify_precompiles(&bank.feature_set)?;
verify_precompiles(tx, &bank.feature_set)?;
}

// Any transaction executed between sanitization time and now may have closed the lookup table(s).
Expand Down
3 changes: 2 additions & 1 deletion rpc/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ use {
prioritization_fee_cache::PrioritizationFeeCache,
snapshot_config::SnapshotConfig,
snapshot_utils,
verify_precompiles::verify_precompiles,
},
solana_sdk::{
account::{AccountSharedData, ReadableAccount},
Expand Down Expand Up @@ -2260,7 +2261,7 @@ fn verify_transaction(
let move_precompile_verification_to_svm =
feature_set.is_active(&feature_set::move_precompile_verification_to_svm::id());
if !move_precompile_verification_to_svm {
if let Err(e) = transaction.verify_precompiles(feature_set) {
if let Err(e) = verify_precompiles(transaction, feature_set) {
Copy link
Author

Choose a reason for hiding this comment

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

implementation change, no affect on the RPC interface.

return Err(RpcCustomError::TransactionPrecompileVerificationFailure(e).into());
}
}
Expand Down
1 change: 1 addition & 0 deletions runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ assert_matches = { workspace = true }
ed25519-dalek = { workspace = true }
libsecp256k1 = { workspace = true }
memoffset = { workspace = true }
rand0-7 = { package = "rand", version = "0.7" }
Copy link
Author

@apfitzge apfitzge Oct 2, 2024

Choose a reason for hiding this comment

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

need this to create the precompile instructions for testing.

logic was mainly copied from the individual precompile tests - if there's a better way without this old version would be happy to change & remove this

rand_chacha = { workspace = true }
solana-accounts-db = { workspace = true, features = ["dev-context-only-utils"] }
solana-logger = { workspace = true }
Expand Down
3 changes: 2 additions & 1 deletion runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ use {
stakes::{InvalidCacheEntryReason, Stakes, StakesCache, StakesEnum},
status_cache::{SlotDelta, StatusCache},
transaction_batch::{OwnedOrBorrowed, TransactionBatch},
verify_precompiles::verify_precompiles,
},
byteorder::{ByteOrder, LittleEndian},
dashmap::{DashMap, DashSet},
Expand Down Expand Up @@ -5664,7 +5665,7 @@ impl Bank {
verification_mode == TransactionVerificationMode::HashAndVerifyPrecompiles
|| verification_mode == TransactionVerificationMode::FullVerification
} {
sanitized_tx.verify_precompiles(&self.feature_set)?;
verify_precompiles(&sanitized_tx, &self.feature_set)?;
}

Ok(sanitized_tx)
Expand Down
1 change: 1 addition & 0 deletions runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub mod stakes;
pub mod static_ids;
pub mod status_cache;
pub mod transaction_batch;
pub mod verify_precompiles;
pub mod vote_sender_types;

#[macro_use]
Expand Down
165 changes: 165 additions & 0 deletions runtime/src/verify_precompiles.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
use {
solana_feature_set::FeatureSet,
solana_sdk::{
precompiles::get_precompiles,
transaction::{Result, TransactionError},
},
solana_svm_transaction::svm_message::SVMMessage,
};

pub fn verify_precompiles(message: &impl SVMMessage, feature_set: &FeatureSet) -> Result<()> {
let mut all_instruction_data = None; // lazily collect this on first pre-compile

let precompiles = get_precompiles();
for (program_id, instruction) in message.program_instructions_iter() {
for precompile in precompiles {
if precompile.check_id(program_id, |id| feature_set.is_active(id)) {
let all_instruction_data: &Vec<&[u8]> = all_instruction_data
.get_or_insert_with(|| message.instructions_iter().map(|ix| ix.data).collect());
precompile
.verify(instruction.data, all_instruction_data, feature_set)
.map_err(|_| TransactionError::InvalidAccountIndex)?;
Copy link
Author

Choose a reason for hiding this comment

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

kept error type here consistent with what tx.verify_precompiles returns...but this does not seem like the correct variant to me.

Copy link

Choose a reason for hiding this comment

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

yeah I think we should fix that up, no idea why that was chosen

Copy link
Author

Choose a reason for hiding this comment

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

Seems like the existing TransactionError::SignatureFailure is reasonable.

Do you feel that one is fine or should we add a new PrecompileFailure variant?

Copy link
Author

Choose a reason for hiding this comment

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

TransactionError::InstructionError also seems reasonable to me

Copy link

Choose a reason for hiding this comment

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

InstructionError is probably best because invalid data offsets is not technically a signature error

break;
}
}
}

Ok(())
}

#[cfg(test)]
mod tests {
use {
super::*,
rand0_7::{thread_rng, Rng},
solana_sdk::{
ed25519_instruction::new_ed25519_instruction,
hash::Hash,
pubkey::Pubkey,
secp256k1_instruction::new_secp256k1_instruction,
signature::Keypair,
signer::Signer,
system_instruction, system_transaction,
transaction::{SanitizedTransaction, Transaction},
},
};

#[test]
fn test_verify_precompiles_simple_transaction() {
let tx = SanitizedTransaction::from_transaction_for_tests(system_transaction::transfer(
&Keypair::new(),
&Pubkey::new_unique(),
1,
Hash::default(),
));
assert!(verify_precompiles(&tx, &FeatureSet::all_enabled()).is_ok());
}

#[test]
fn test_verify_precompiles_secp256k1() {
let secp_privkey = libsecp256k1::SecretKey::random(&mut thread_rng());
let message_arr = b"hello";
let mut secp_instruction = new_secp256k1_instruction(&secp_privkey, message_arr);
let mint_keypair = Keypair::new();
let feature_set = FeatureSet::all_enabled();

let tx =
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer(
&[secp_instruction.clone()],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
Hash::default(),
));

assert!(verify_precompiles(&tx, &feature_set).is_ok());

let index = thread_rng().gen_range(0, secp_instruction.data.len());
secp_instruction.data[index] = secp_instruction.data[index].wrapping_add(12);
let tx =
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer(
&[secp_instruction],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
Hash::default(),
));

assert!(verify_precompiles(&tx, &feature_set).is_err());
}

#[test]
fn test_verify_precompiles_ed25519() {
let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng());
let message_arr = b"hello";
let mut instruction = new_ed25519_instruction(&privkey, message_arr);
let mint_keypair = Keypair::new();
let feature_set = FeatureSet::all_enabled();

let tx =
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer(
&[instruction.clone()],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
Hash::default(),
));

assert!(verify_precompiles(&tx, &feature_set).is_ok());

let index = loop {
let index = thread_rng().gen_range(0, instruction.data.len());
// byte 1 is not used, so this would not cause the verify to fail
if index != 1 {
break index;
}
};

instruction.data[index] = instruction.data[index].wrapping_add(12);
let tx =
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer(
&[instruction],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
Hash::default(),
));
assert!(verify_precompiles(&tx, &feature_set).is_err());
}

#[test]
fn test_verify_precompiles_mixed() {
let message_arr = b"hello";
let secp_privkey = libsecp256k1::SecretKey::random(&mut thread_rng());
let mut secp_instruction = new_secp256k1_instruction(&secp_privkey, message_arr);
let ed25519_privkey = ed25519_dalek::Keypair::generate(&mut thread_rng());
let ed25519_instruction = new_ed25519_instruction(&ed25519_privkey, message_arr);

let mint_keypair = Keypair::new();
let feature_set = FeatureSet::all_enabled();

let tx =
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer(
&[
secp_instruction.clone(),
ed25519_instruction.clone(),
system_instruction::transfer(&mint_keypair.pubkey(), &Pubkey::new_unique(), 1),
],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
Hash::default(),
));
assert!(verify_precompiles(&tx, &feature_set).is_ok());

let index = thread_rng().gen_range(0, secp_instruction.data.len());
secp_instruction.data[index] = secp_instruction.data[index].wrapping_add(12);
let tx =
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer(
&[
secp_instruction,
ed25519_instruction,
system_instruction::transfer(&mint_keypair.pubkey(), &Pubkey::new_unique(), 1),
],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
Hash::default(),
));
assert!(verify_precompiles(&tx, &feature_set).is_err());
}
}
Loading