From c8f6a0817b81d4a85b8ad95f14b473efddbb1381 Mon Sep 17 00:00:00 2001 From: Sean Young Date: Mon, 30 Aug 2021 08:58:45 +0100 Subject: [PATCH] verify_precompiles needs FeatureSet Rather than pass in individual features, pass in the entire feature set so that we can add the ed25519 program feature in a later commit. (cherry picked from commit 0f62771f424bd19dc23a5db09bd0e4a65a364ac5) Conflicts: banks-server/src/banks_server.rs core/src/banking_stage.rs programs/secp256k1/src/lib.rs rpc/src/rpc.rs runtime/src/bank.rs sdk/src/transaction.rs sdk/src/transaction/sanitized.rs --- banks-server/src/banks_server.rs | 10 +++----- core/src/banking_stage.rs | 25 ++++++++++-------- ledger/src/blockstore_processor.rs | 6 +---- ledger/src/entry.rs | 41 ++++++++++++++++++++---------- programs/secp256k1/src/lib.rs | 14 ++++++++-- rpc/src/rpc.rs | 14 ++++------ runtime/src/bank.rs | 5 ---- sdk/src/transaction.rs | 6 +++-- 8 files changed, 67 insertions(+), 54 deletions(-) diff --git a/banks-server/src/banks_server.rs b/banks-server/src/banks_server.rs index 62a8570894..5e62a222f4 100644 --- a/banks-server/src/banks_server.rs +++ b/banks-server/src/banks_server.rs @@ -12,6 +12,7 @@ use solana_sdk::{ account::Account, clock::Slot, commitment_config::CommitmentLevel, + feature_set::FeatureSet, fee_calculator::FeeCalculator, hash::Hash, pubkey::Pubkey, @@ -142,11 +143,11 @@ impl BanksServer { fn verify_transaction( transaction: &Transaction, - libsecp256k1_0_5_upgrade_enabled: bool, + feature_set: &Arc, ) -> transaction::Result<()> { if let Err(err) = transaction.verify() { Err(err) - } else if let Err(err) = transaction.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) { + } else if let Err(err) = transaction.verify_precompiles(feature_set) { Err(err) } else { Ok(()) @@ -236,10 +237,7 @@ impl Banks for BanksServer { transaction: Transaction, commitment: CommitmentLevel, ) -> Option> { - if let Err(err) = verify_transaction( - &transaction, - self.bank(commitment).libsecp256k1_0_5_upgrade_enabled(), - ) { + if let Err(err) = verify_transaction(&transaction, &self.bank(commitment).feature_set) { return Some(Err(err)); } diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 987ffa875f..ca84cc9142 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -35,6 +35,7 @@ use solana_sdk::{ Slot, DEFAULT_TICKS_PER_SLOT, MAX_PROCESSING_AGE, MAX_TRANSACTION_FORWARDING_DELAY, MAX_TRANSACTION_FORWARDING_DELAY_GPU, }, + feature_set, message::Message, pubkey::Pubkey, short_vec::decode_shortu16_len, @@ -1129,7 +1130,7 @@ impl BankingStage { fn transactions_from_packets( msgs: &Packets, transaction_indexes: &[usize], - libsecp256k1_0_5_upgrade_enabled: bool, + feature_set: &Arc, votes_only: bool, cost_tracker: &Arc>, banking_stage_stats: &BankingStageStats, @@ -1147,8 +1148,7 @@ impl BankingStage { } let tx: Transaction = limited_deserialize(&p.data[0..p.meta.size]).ok()?; - tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) - .ok()?; + tx.verify_precompiles(feature_set).ok()?; Some((tx, *tx_index)) }) @@ -1270,7 +1270,7 @@ impl BankingStage { Self::transactions_from_packets( msgs, &packet_indexes, - bank.libsecp256k1_0_5_upgrade_enabled(), + &bank.feature_set, bank.vote_only_bank(), cost_tracker, banking_stage_stats, @@ -1380,7 +1380,7 @@ impl BankingStage { Self::transactions_from_packets( msgs, &transaction_indexes, - bank.libsecp256k1_0_5_upgrade_enabled(), + &bank.feature_set, bank.vote_only_bank(), cost_tracker, banking_stage_stats, @@ -3204,6 +3204,7 @@ mod tests { &keypair, None, ); + let features = Arc::new(feature_set::FeatureSet::default()); // packets with no votes { @@ -3212,10 +3213,11 @@ mod tests { make_test_packets(vec![transfer_tx.clone(), transfer_tx.clone()], vote_indexes); let mut votes_only = false; + let (txs, tx_packet_index, _) = BankingStage::transactions_from_packets( &packets, &packet_indexes, - false, + &features, votes_only, &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( CostModel::default(), @@ -3231,7 +3233,7 @@ mod tests { let (txs, tx_packet_index, _) = BankingStage::transactions_from_packets( &packets, &packet_indexes, - false, + &features, votes_only, &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( CostModel::default(), @@ -3256,7 +3258,7 @@ mod tests { let (txs, tx_packet_index, _) = BankingStage::transactions_from_packets( &packets, &packet_indexes, - false, + &features, votes_only, &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( CostModel::default(), @@ -3272,7 +3274,7 @@ mod tests { let (txs, tx_packet_index, _) = BankingStage::transactions_from_packets( &packets, &packet_indexes, - false, + &features, votes_only, &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( CostModel::default(), @@ -3292,12 +3294,13 @@ mod tests { vec![vote_tx.clone(), vote_tx.clone(), vote_tx], vote_indexes, ); + let features = Arc::new(feature_set::FeatureSet::default()); let mut votes_only = false; let (txs, tx_packet_index, _) = BankingStage::transactions_from_packets( &packets, &packet_indexes, - false, + &features, votes_only, &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( CostModel::default(), @@ -3313,7 +3316,7 @@ mod tests { let (txs, tx_packet_index, _) = BankingStage::transactions_from_packets( &packets, &packet_indexes, - false, + &features, votes_only, &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( CostModel::default(), diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 11971e5de1..a50ba8e1aa 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -872,11 +872,7 @@ pub fn confirm_slot( }; let check_start = Instant::now(); - let check_result = entries.verify_and_hash_transactions( - skip_verification, - bank.libsecp256k1_0_5_upgrade_enabled(), - bank.verify_tx_signatures_len_enabled(), - ); + let check_result = entries.verify_and_hash_transactions(skip_verification, &bank.feature_set); if check_result.is_none() { warn!("Ledger proof of history failed at slot: {}", slot); return Err(BlockError::InvalidEntryHash.into()); diff --git a/ledger/src/entry.rs b/ledger/src/entry.rs index f6dfb77e7c..6e48aa7954 100644 --- a/ledger/src/entry.rs +++ b/ledger/src/entry.rs @@ -22,6 +22,7 @@ use solana_sdk::hash::Hash; use solana_sdk::packet::PACKET_DATA_SIZE; use solana_sdk::timing; use solana_sdk::transaction::Transaction; +use solana_sdk::{feature_set, feature_set::FeatureSet}; use std::borrow::Cow; use std::cell::RefCell; use std::ffi::OsStr; @@ -359,8 +360,7 @@ pub trait EntrySlice { fn verify_and_hash_transactions( &self, skip_verification: bool, - libsecp256k1_0_5_upgrade_enabled: bool, - verify_tx_signatures_len: bool, + feature_set: &Arc, ) -> Option>>; } @@ -515,8 +515,7 @@ impl EntrySlice for [Entry] { fn verify_and_hash_transactions<'a>( &'a self, skip_verification: bool, - libsecp256k1_0_5_upgrade_enabled: bool, - verify_tx_signatures_len: bool, + feature_set: &Arc, ) -> Option>> { let verify_and_hash = |tx: &'a Transaction| -> Option> { let message_hash = if !skip_verification { @@ -524,9 +523,10 @@ impl EntrySlice for [Entry] { if size > PACKET_DATA_SIZE as u64 { return None; } - tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) - .ok()?; - if verify_tx_signatures_len && !tx.verify_signatures_len() { + tx.verify_precompiles(feature_set).ok()?; + if feature_set.is_active(&feature_set::verify_tx_signatures_len::id()) + && !tx.verify_signatures_len() + { return None; } tx.verify_and_hash_message().ok()? @@ -924,22 +924,34 @@ mod tests { { let tx = make_transaction(TestCase::RemoveSignature); let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; + let features = Arc::new(feature_set::FeatureSet::default()); assert!(entries[..] - .verify_and_hash_transactions(false, false, false) + .verify_and_hash_transactions(false, &features) .is_some()); + let mut features = feature_set::FeatureSet::default(); + features + .active + .insert(feature_set::verify_tx_signatures_len::id(), 0); + let features = Arc::new(features); assert!(entries[..] - .verify_and_hash_transactions(false, false, true) + .verify_and_hash_transactions(false, &features) .is_none()); } // Too many signatures. { let tx = make_transaction(TestCase::AddSignature); let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; + let features = Arc::new(feature_set::FeatureSet::default()); assert!(entries[..] - .verify_and_hash_transactions(false, false, false) + .verify_and_hash_transactions(false, &features) .is_some()); + let mut features = feature_set::FeatureSet::default(); + features + .active + .insert(feature_set::verify_tx_signatures_len::id(), 0); + let features = Arc::new(features); assert!(entries[..] - .verify_and_hash_transactions(false, false, true) + .verify_and_hash_transactions(false, &features) .is_none()); } } @@ -950,6 +962,7 @@ mod tests { let recent_blockhash = hash_new_rand(&mut rng); let keypair = Keypair::new(); let pubkey = keypair.pubkey(); + let features = Arc::new(feature_set::FeatureSet::default()); let make_transaction = |size| { let ixs: Vec<_> = std::iter::repeat_with(|| { system_instruction::transfer(&pubkey, &Pubkey::new_unique(), 1) @@ -965,7 +978,7 @@ mod tests { let entries = vec![next_entry(&recent_blockhash, 1, vec![tx.clone()])]; assert!(bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64); assert!(entries[..] - .verify_and_hash_transactions(false, false, false) + .verify_and_hash_transactions(false, &features) .is_some()); } // Big transaction. @@ -974,7 +987,7 @@ mod tests { let entries = vec![next_entry(&recent_blockhash, 1, vec![tx.clone()])]; assert!(bincode::serialized_size(&tx).unwrap() > PACKET_DATA_SIZE as u64); assert!(entries[..] - .verify_and_hash_transactions(false, false, false) + .verify_and_hash_transactions(false, &features) .is_none()); } // Assert that verify fails as soon as serialized @@ -985,7 +998,7 @@ mod tests { assert_eq!( bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64, entries[..] - .verify_and_hash_transactions(false, false, false) + .verify_and_hash_transactions(false, &features) .is_some(), ); } diff --git a/programs/secp256k1/src/lib.rs b/programs/secp256k1/src/lib.rs index 2f1aee3e1e..de879e0924 100644 --- a/programs/secp256k1/src/lib.rs +++ b/programs/secp256k1/src/lib.rs @@ -15,6 +15,7 @@ pub fn process_instruction( pub mod test { use rand::{thread_rng, Rng}; use solana_sdk::{ + feature_set, hash::Hash, secp256k1_instruction::{ new_secp256k1_instruction, SecpSignatureOffsets, SIGNATURE_OFFSETS_SERIALIZED_SIZE, @@ -22,6 +23,7 @@ pub mod test { signature::{Keypair, Signer}, transaction::Transaction, }; + use std::sync::Arc; #[test] fn test_secp256k1() { @@ -36,6 +38,14 @@ pub mod test { let message_arr = b"hello"; let mut secp_instruction = new_secp256k1_instruction(&secp_privkey, message_arr); let mint_keypair = Keypair::new(); + let mut feature_set = feature_set::FeatureSet::all_enabled(); + feature_set + .active + .remove(&feature_set::libsecp256k1_0_5_upgrade_enabled::id()); + feature_set + .inactive + .insert(feature_set::libsecp256k1_0_5_upgrade_enabled::id()); + let feature_set = Arc::new(feature_set); let tx = Transaction::new_signed_with_payer( &[secp_instruction.clone()], @@ -44,7 +54,7 @@ pub mod test { Hash::default(), ); - assert!(tx.verify_precompiles(false).is_ok()); + assert!(tx.verify_precompiles(&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); @@ -54,6 +64,6 @@ pub mod test { &[&mint_keypair], Hash::default(), ); - assert!(tx.verify_precompiles(false).is_err()); + assert!(tx.verify_precompiles(&feature_set).is_err()); } } diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 3b2685e264..a6975f4cde 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -58,6 +58,7 @@ use { epoch_info::EpochInfo, epoch_schedule::EpochSchedule, exit::Exit, + feature_set, hash::Hash, pubkey::Pubkey, sanitize::Sanitize, @@ -1937,13 +1938,13 @@ fn optimize_filters(filters: &mut Vec) { fn verify_transaction( transaction: &Transaction, - libsecp256k1_0_5_upgrade_enabled: bool, + feature_set: &Arc, ) -> Result<()> { if transaction.verify().is_err() { return Err(RpcCustomError::TransactionSignatureVerificationFailure.into()); } - if let Err(e) = transaction.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) { + if let Err(e) = transaction.verify_precompiles(feature_set) { return Err(RpcCustomError::TransactionPrecompileVerificationFailure(e).into()); } @@ -3097,10 +3098,7 @@ pub mod rpc_full { } if !config.skip_preflight { - if let Err(e) = verify_transaction( - &transaction, - preflight_bank.libsecp256k1_0_5_upgrade_enabled(), - ) { + if let Err(e) = verify_transaction(&transaction, &preflight_bank.feature_set) { return Err(e); } @@ -3172,9 +3170,7 @@ pub mod rpc_full { )); } - if let Err(e) = - verify_transaction(&transaction, bank.libsecp256k1_0_5_upgrade_enabled()) - { + if let Err(e) = verify_transaction(&transaction, &bank.feature_set) { return Err(e); } } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 5e4e02ada5..d5d95d892b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -5609,11 +5609,6 @@ impl Bank { .is_active(&feature_set::verify_tx_signatures_len::id()) } - pub fn libsecp256k1_0_5_upgrade_enabled(&self) -> bool { - self.feature_set - .is_active(&feature_set::libsecp256k1_0_5_upgrade_enabled::id()) - } - pub fn merge_nonce_error_into_system_error(&self) -> bool { self.feature_set .is_active(&feature_set::merge_nonce_error_into_system_error::id()) diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index f72c417ad5..f70f388236 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -5,6 +5,7 @@ use crate::sanitize::{Sanitize, SanitizeError}; use crate::secp256k1_instruction::verify_eth_addresses; use crate::{ + feature_set, hash::Hash, instruction::{CompiledInstruction, Instruction, InstructionError}, message::Message, @@ -18,6 +19,7 @@ use crate::{ system_program, }; use std::result; +use std::sync::Arc; use thiserror::Error; /// Reasons a transaction might be rejected. @@ -409,7 +411,7 @@ impl Transaction { .collect() } - pub fn verify_precompiles(&self, libsecp256k1_0_5_upgrade_enabled: bool) -> Result<()> { + pub fn verify_precompiles(&self, feature_set: &Arc) -> Result<()> { for instruction in &self.message().instructions { // The Transaction may not be sanitized at this point if instruction.program_id_index as usize >= self.message().account_keys.len() { @@ -427,7 +429,7 @@ impl Transaction { let e = verify_eth_addresses( data, &instruction_datas, - libsecp256k1_0_5_upgrade_enabled, + feature_set.is_active(&feature_set::libsecp256k1_0_5_upgrade_enabled::id()), ); e.map_err(|_| TransactionError::InvalidAccountIndex)?; }