From 7896e8288dbff27c09e1e9be564e763417d0b701 Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Fri, 29 Mar 2019 16:11:21 -0600 Subject: [PATCH] Replace Transaction::fee with a FeeCalculator --- core/src/chacha.rs | 2 +- runtime/src/accounts.rs | 35 +++++++++++++++++----- runtime/src/bank.rs | 62 ++++++++++++++++++++++----------------- sdk/src/fee_calculator.rs | 48 ++++++++++++++++++++++++++++++ sdk/src/instruction.rs | 4 +-- sdk/src/lib.rs | 1 + sdk/src/message.rs | 6 ---- sdk/src/transaction.rs | 31 ++++++++------------ 8 files changed, 127 insertions(+), 62 deletions(-) create mode 100644 sdk/src/fee_calculator.rs diff --git a/core/src/chacha.rs b/core/src/chacha.rs index 30876c6f09..27b2da717e 100644 --- a/core/src/chacha.rs +++ b/core/src/chacha.rs @@ -164,7 +164,7 @@ mod tests { use bs58; // golden needs to be updated if blob stuff changes.... let golden = Hash::new( - &bs58::decode("GnNzHbBRnn1WbrAXudmyxcqhaFiXQdzYWZpi6ToMM4pW") + &bs58::decode("P5ZZcpRopdqU4ZVZz1Kmck5zykiNSxc9T3iPZzF3rMc") .into_vec() .unwrap(), ); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 0a5d0a15d8..817e20dabe 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -7,6 +7,7 @@ use log::*; use rand::{thread_rng, Rng}; use solana_metrics::counter::Counter; use solana_sdk::account::Account; +use solana_sdk::fee_calculator::FeeCalculator; use solana_sdk::hash::{hash, Hash}; use solana_sdk::native_loader; use solana_sdk::pubkey::Pubkey; @@ -611,11 +612,12 @@ impl AccountsDB { &self, fork: Fork, tx: &Transaction, + fee: u64, error_counters: &mut ErrorCounters, ) -> Result> { // Copy all the accounts let message = tx.message(); - if tx.signatures.is_empty() && message.fee != 0 { + if tx.signatures.is_empty() && fee != 0 { Err(TransactionError::MissingSignatureForFee) } else { // Check for unique account keys @@ -633,11 +635,11 @@ impl AccountsDB { if called_accounts.is_empty() || called_accounts[0].lamports == 0 { error_counters.account_not_found += 1; Err(TransactionError::AccountNotFound) - } else if called_accounts[0].lamports < message.fee { + } else if called_accounts[0].lamports < fee { error_counters.insufficient_funds += 1; Err(TransactionError::InsufficientFundsForFee) } else { - called_accounts[0].lamports -= message.fee; + called_accounts[0].lamports -= fee; Ok(called_accounts) } } @@ -711,13 +713,15 @@ impl AccountsDB { fork: Fork, txs: &[Transaction], lock_results: Vec>, + fee_calculator: &FeeCalculator, error_counters: &mut ErrorCounters, ) -> Vec> { txs.iter() .zip(lock_results.into_iter()) .map(|etx| match etx { (tx, Ok(())) => { - let accounts = self.load_tx_accounts(fork, tx, error_counters)?; + let fee = fee_calculator.calculate_fee(tx.message()); + let accounts = self.load_tx_accounts(fork, tx, fee, error_counters)?; let loaders = self.load_loaders(fork, tx, error_counters)?; Ok((accounts, loaders)) } @@ -943,10 +947,11 @@ impl Accounts { fork: Fork, txs: &[Transaction], results: Vec>, + fee_calculator: &FeeCalculator, error_counters: &mut ErrorCounters, ) -> Vec> { self.accounts_db - .load_accounts(fork, txs, results, error_counters) + .load_accounts(fork, txs, results, fee_calculator, error_counters) } /// Store the accounts into the DB @@ -1006,9 +1011,10 @@ mod tests { }); } - fn load_accounts( + fn load_accounts_with_fee( tx: Transaction, ka: &Vec<(Pubkey, Account)>, + fee_calculator: &FeeCalculator, error_counters: &mut ErrorCounters, ) -> Vec> { let accounts = Accounts::new(0, None); @@ -1016,10 +1022,19 @@ mod tests { accounts.store_slow(0, &ka.0, &ka.1); } - let res = accounts.load_accounts(0, &[tx], vec![Ok(())], error_counters); + let res = accounts.load_accounts(0, &[tx], vec![Ok(())], &fee_calculator, error_counters); res } + fn load_accounts( + tx: Transaction, + ka: &Vec<(Pubkey, Account)>, + error_counters: &mut ErrorCounters, + ) -> Vec> { + let fee_calculator = FeeCalculator::default(); + load_accounts_with_fee(tx, ka, &fee_calculator, error_counters) + } + #[test] fn test_load_accounts_no_key() { let accounts: Vec<(Pubkey, Account)> = Vec::new(); @@ -1119,7 +1134,11 @@ mod tests { instructions, ); - let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + let fee_calculator = FeeCalculator::new(10); + assert_eq!(fee_calculator.calculate_fee(tx.message()), 10); + + let loaded_accounts = + load_accounts_with_fee(tx, &accounts, &fee_calculator, &mut error_counters); assert_eq!(error_counters.insufficient_funds, 1); assert_eq!(loaded_accounts.len(), 1); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index b5116ea56b..763756d2e4 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -12,6 +12,7 @@ use hashbrown::HashMap; use log::*; use solana_metrics::counter::Counter; use solana_sdk::account::Account; +use solana_sdk::fee_calculator::FeeCalculator; use solana_sdk::genesis_block::GenesisBlock; use solana_sdk::hash::{extend_and_hash, Hash}; use solana_sdk::native_loader; @@ -143,6 +144,9 @@ pub struct Bank { /// The pubkey to send transactions fees to. collector_id: Pubkey, + /// An object to calculate transaction fees. + pub fee_calculator: FeeCalculator, + /// initialized from genesis epoch_schedule: EpochSchedule, @@ -465,8 +469,13 @@ impl Bank { results: Vec>, error_counters: &mut ErrorCounters, ) -> Vec> { - self.accounts - .load_accounts(self.accounts_id, txs, results, error_counters) + self.accounts.load_accounts( + self.accounts_id, + txs, + results, + &self.fee_calculator, + error_counters, + ) } fn check_refs( &self, @@ -659,16 +668,17 @@ impl Bank { .iter() .zip(executed.iter()) .map(|(tx, res)| { + let fee = self.fee_calculator.calculate_fee(tx.message()); let message = tx.message(); match *res { Err(TransactionError::InstructionError(_, _)) => { // Charge the transaction fee even in case of InstructionError - self.withdraw(&message.account_keys[0], message.fee)?; - fees += message.fee; + self.withdraw(&message.account_keys[0], fee)?; + fees += fee; Ok(()) } Ok(()) => { - fees += message.fee; + fees += fee; Ok(()) } _ => res.clone(), @@ -1059,7 +1069,9 @@ mod tests { #[test] fn test_detect_failed_duplicate_transactions() { let (genesis_block, mint_keypair) = GenesisBlock::new(2); - let bank = Bank::new(&genesis_block); + let mut bank = Bank::new(&genesis_block); + bank.fee_calculator.lamports_per_signature = 1; + let dest = Keypair::new(); // source with 0 program context @@ -1068,7 +1080,7 @@ mod tests { &dest.pubkey(), 2, genesis_block.hash(), - 1, + 0, ); let signature = tx.signatures[0]; assert!(!bank.has_signature(&signature)); @@ -1176,19 +1188,22 @@ mod tests { fn test_bank_tx_fee() { let leader = Keypair::new().pubkey(); let (genesis_block, mint_keypair) = GenesisBlock::new_with_leader(100, &leader, 3); - let bank = Bank::new(&genesis_block); + let mut bank = Bank::new(&genesis_block); + bank.fee_calculator.lamports_per_signature = 3; + let key1 = Keypair::new(); let key2 = Keypair::new(); let tx = - SystemTransaction::new_move(&mint_keypair, &key1.pubkey(), 2, genesis_block.hash(), 3); + SystemTransaction::new_move(&mint_keypair, &key1.pubkey(), 2, genesis_block.hash(), 0); let initial_balance = bank.get_balance(&leader); assert_eq!(bank.process_transaction(&tx), Ok(())); assert_eq!(bank.get_balance(&leader), initial_balance + 3); assert_eq!(bank.get_balance(&key1.pubkey()), 2); assert_eq!(bank.get_balance(&mint_keypair.pubkey()), 100 - 5 - 3); - let tx = SystemTransaction::new_move(&key1, &key2.pubkey(), 1, genesis_block.hash(), 1); + bank.fee_calculator.lamports_per_signature = 1; + let tx = SystemTransaction::new_move(&key1, &key2.pubkey(), 1, genesis_block.hash(), 0); assert_eq!(bank.process_transaction(&tx), Ok(())); assert_eq!(bank.get_balance(&leader), initial_balance + 4); assert_eq!(bank.get_balance(&key1.pubkey()), 0); @@ -1200,13 +1215,13 @@ mod tests { fn test_filter_program_errors_and_collect_fee() { let leader = Keypair::new().pubkey(); let (genesis_block, mint_keypair) = GenesisBlock::new_with_leader(100, &leader, 3); - let bank = Bank::new(&genesis_block); + let mut bank = Bank::new(&genesis_block); let key = Keypair::new(); let tx1 = - SystemTransaction::new_move(&mint_keypair, &key.pubkey(), 2, genesis_block.hash(), 3); + SystemTransaction::new_move(&mint_keypair, &key.pubkey(), 2, genesis_block.hash(), 0); let tx2 = - SystemTransaction::new_move(&mint_keypair, &key.pubkey(), 5, genesis_block.hash(), 1); + SystemTransaction::new_move(&mint_keypair, &key.pubkey(), 5, genesis_block.hash(), 0); let results = vec![ Ok(()), @@ -1216,9 +1231,10 @@ mod tests { )), ]; + bank.fee_calculator.lamports_per_signature = 2; let initial_balance = bank.get_balance(&leader); let results = bank.filter_program_errors_and_collect_fee(&vec![tx1, tx2], &results); - assert_eq!(bank.get_balance(&leader), initial_balance + 3 + 1); + assert_eq!(bank.get_balance(&leader), initial_balance + 2 + 2); assert_eq!(results[0], Ok(())); assert_eq!(results[1], Ok(())); } @@ -1586,29 +1602,21 @@ mod tests { fn test_zero_signatures() { solana_logger::setup(); let (genesis_block, mint_keypair) = GenesisBlock::new(500); - let bank = Arc::new(Bank::new(&genesis_block)); + let mut bank = Bank::new(&genesis_block); + bank.fee_calculator.lamports_per_signature = 2; let key = Keypair::new(); let mut move_instruction = - SystemInstruction::new_move(&mint_keypair.pubkey(), &key.pubkey(), 1); + SystemInstruction::new_move(&mint_keypair.pubkey(), &key.pubkey(), 0); move_instruction.accounts[0].is_signer = false; - let mut tx = Transaction::new_signed_instructions( + let tx = Transaction::new_signed_instructions( &Vec::<&Keypair>::new(), vec![move_instruction], bank.last_blockhash(), - 2, + 0, ); - assert_eq!( - bank.process_transaction(&tx), - Err(TransactionError::MissingSignatureForFee) - ); - - // Set the fee to 0, this should give an InstructionError - // but since no signature we cannot look up the error. - tx.message.fee = 0; - assert_eq!(bank.process_transaction(&tx), Ok(())); assert_eq!(bank.get_balance(&key.pubkey()), 0); } diff --git a/sdk/src/fee_calculator.rs b/sdk/src/fee_calculator.rs new file mode 100644 index 0000000000..56b2b709ee --- /dev/null +++ b/sdk/src/fee_calculator.rs @@ -0,0 +1,48 @@ +use crate::message::Message; + +#[derive(Default)] +pub struct FeeCalculator { + pub lamports_per_signature: u64, +} + +impl FeeCalculator { + pub fn new(lamports_per_signature: u64) -> Self { + Self { + lamports_per_signature, + } + } + + pub fn calculate_fee(&self, message: &Message) -> u64 { + self.lamports_per_signature * u64::from(message.num_required_signatures) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::pubkey::Pubkey; + use crate::system_instruction::SystemInstruction; + + #[test] + fn test_fee_calculator_calculate_fee() { + // Default: no fee. + let message = Message::new(vec![]); + assert_eq!(FeeCalculator::default().calculate_fee(&message), 0); + + // No signature, no fee. + assert_eq!(FeeCalculator::new(1).calculate_fee(&message), 0); + + // One signature, a fee. + let pubkey0 = Pubkey::new(&[0; 32]); + let pubkey1 = Pubkey::new(&[1; 32]); + let ix0 = SystemInstruction::new_move(&pubkey0, &pubkey1, 1); + let message = Message::new(vec![ix0]); + assert_eq!(FeeCalculator::new(2).calculate_fee(&message), 2); + + // Two signatures, double the fee. + let ix0 = SystemInstruction::new_move(&pubkey0, &pubkey1, 1); + let ix1 = SystemInstruction::new_move(&pubkey1, &pubkey0, 1); + let message = Message::new(vec![ix0, ix1]); + assert_eq!(FeeCalculator::new(2).calculate_fee(&message), 4); + } +} diff --git a/sdk/src/instruction.rs b/sdk/src/instruction.rs index 2867c67598..c6740082fe 100644 --- a/sdk/src/instruction.rs +++ b/sdk/src/instruction.rs @@ -67,7 +67,7 @@ impl InstructionError { } } -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Clone)] pub struct Instruction { /// Pubkey of the instruction processor that executes this instruction pub program_ids_index: Pubkey, @@ -93,7 +93,7 @@ impl Instruction { } /// Account metadata used to define Instructions -#[derive(Debug, PartialEq, Serialize, Deserialize)] +#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] pub struct AccountMeta { /// An account's public key pub pubkey: Pubkey, diff --git a/sdk/src/lib.rs b/sdk/src/lib.rs index 7f02144562..e6f0d7eb7e 100644 --- a/sdk/src/lib.rs +++ b/sdk/src/lib.rs @@ -1,5 +1,6 @@ pub mod account; pub mod bpf_loader; +pub mod fee_calculator; pub mod genesis_block; pub mod hash; pub mod instruction; diff --git a/sdk/src/message.rs b/sdk/src/message.rs index ddb4d9410e..dfaad1197e 100644 --- a/sdk/src/message.rs +++ b/sdk/src/message.rs @@ -81,9 +81,6 @@ pub struct Message { /// The id of a recent ledger entry. pub recent_blockhash: Hash, - /// The number of lamports paid for processing and storing of this transaction. - pub fee: u64, - /// All the program id keys used to execute this transaction's instructions #[serde(with = "short_vec")] pub program_ids: Vec, @@ -99,7 +96,6 @@ impl Message { num_required_signatures: u8, account_keys: Vec, recent_blockhash: Hash, - fee: u64, program_ids: Vec, instructions: Vec, ) -> Self { @@ -107,7 +103,6 @@ impl Message { num_required_signatures, account_keys, recent_blockhash, - fee, program_ids, instructions, } @@ -123,7 +118,6 @@ impl Message { num_required_signatures, signed_keys, Hash::default(), - 0, program_ids, instructions, ) diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index 63a0493672..1ccb4aaaea 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -49,7 +49,7 @@ pub enum TransactionError { /// An atomic transaction #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct Transaction { - /// A set of digital signatures of `account_keys`, `program_ids`, `recent_blockhash`, `fee` and `instructions`, signed by the first + /// A set of digital signatures of `account_keys`, `program_ids`, `recent_blockhash`, and `instructions`, signed by the first /// signatures.len() keys of account_keys #[serde(with = "short_vec")] pub signatures: Vec, @@ -85,10 +85,9 @@ impl Transaction { from_keypairs: &[&T], instructions: Vec, recent_blockhash: Hash, - fee: u64, + _fee: u64, ) -> Transaction { - let mut message = Message::new(instructions); - message.fee = fee; + let message = Message::new(instructions); Self::new(from_keypairs, message, recent_blockhash) } @@ -104,7 +103,7 @@ impl Transaction { from_keypairs: &[&T], keys: &[Pubkey], recent_blockhash: Hash, - fee: u64, + _fee: u64, program_ids: Vec, instructions: Vec, ) -> Self { @@ -117,7 +116,6 @@ impl Transaction { from_keypairs.len() as u8, account_keys, Hash::default(), - fee, program_ids, instructions, ); @@ -308,8 +306,7 @@ mod tests { AccountMeta::new(to, false), ]; let instruction = Instruction::new(program_id, &(1u8, 2u8, 3u8), account_metas); - let mut message = Message::new(vec![instruction]); - message.fee = 99; + let message = Message::new(vec![instruction]); Transaction::new(&[&keypair], message, Hash::default()) } @@ -352,19 +349,17 @@ mod tests { let len_size = 1; let num_required_sigs_size = 1; let blockhash_size = size_of::(); - let fee_size = size_of::(); let expected_transaction_size = len_size + (tx.signatures.len() * size_of::()) + num_required_sigs_size + len_size + (tx.message.account_keys.len() * size_of::()) + blockhash_size - + fee_size + len_size + (tx.message.program_ids.len() * size_of::()) + len_size + expected_instruction_size; - assert_eq!(expected_transaction_size, 222); + assert_eq!(expected_transaction_size, 214); assert_eq!( serialized_size(&tx).unwrap() as usize, @@ -380,16 +375,16 @@ mod tests { assert_eq!( serialize(&create_sample_transaction()).unwrap(), vec![ - 1, 102, 197, 120, 176, 192, 35, 190, 233, 5, 135, 30, 114, 209, 105, 219, 212, 203, - 242, 72, 150, 205, 68, 30, 188, 119, 31, 212, 40, 43, 88, 45, 239, 83, 45, 208, - 103, 108, 239, 82, 185, 160, 161, 111, 112, 51, 254, 61, 254, 131, 206, 221, 117, - 124, 50, 1, 6, 38, 218, 9, 95, 28, 46, 49, 5, 1, 2, 36, 100, 158, 252, 33, 161, 97, + 1, 0, 30, 236, 164, 222, 77, 89, 244, 36, 92, 35, 192, 25, 100, 18, 61, 155, 111, + 89, 189, 154, 90, 255, 217, 203, 105, 50, 243, 208, 179, 89, 146, 122, 222, 91, 34, + 106, 93, 82, 147, 213, 223, 184, 32, 204, 61, 227, 227, 41, 211, 67, 5, 156, 236, + 251, 178, 235, 234, 174, 123, 15, 26, 145, 3, 1, 2, 36, 100, 158, 252, 33, 161, 97, 185, 62, 89, 99, 195, 250, 249, 187, 189, 171, 118, 241, 90, 248, 14, 68, 219, 231, 62, 157, 5, 142, 27, 210, 117, 1, 1, 1, 4, 5, 6, 7, 8, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 8, 7, 6, 5, 4, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 99, 0, 0, 0, 0, 0, 0, 0, - 1, 2, 2, 2, 4, 5, 6, 7, 8, 9, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 9, 8, 7, 6, - 5, 4, 2, 2, 2, 1, 0, 2, 0, 1, 3, 1, 2, 3 + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 2, 2, 4, 5, 6, 7, 8, + 9, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 9, 8, 7, 6, 5, 4, 2, 2, 2, 1, 0, 2, 0, + 1, 3, 1, 2, 3 ] ); }