From 674d2698a150251689b8eab787a45b50f4b906df Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 25 Feb 2022 15:42:53 -0700 Subject: [PATCH 1/4] Add failing test --- runtime/src/bank.rs | 139 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 1a3912c28ce186..4f090be4172a05 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -16476,6 +16476,145 @@ pub(crate) mod tests { assert!(result.is_ok()); } + #[test] + fn test_invalid_rent_state_changes_fee_payer() { + let GenesisConfigInfo { + mut genesis_config, + mint_keypair, + .. + } = create_genesis_config_with_leader(sol_to_lamports(100.), &Pubkey::new_unique(), 42); + genesis_config.rent = Rent::default(); + genesis_config.fee_rate_governor = FeeRateGovernor::new( + solana_sdk::fee_calculator::DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE, + solana_sdk::fee_calculator::DEFAULT_TARGET_SIGNATURES_PER_SLOT, + ); + let rent_exempt_minimum = genesis_config.rent.minimum_balance(0); + + // Create legacy rent-paying System account + let rent_paying_fee_payer = Keypair::new(); + genesis_config.accounts.insert( + rent_paying_fee_payer.pubkey(), + Account::new(rent_exempt_minimum - 1, 0, &system_program::id()), + ); + // Create RentExempt recipient account + let recipient = Pubkey::new_unique(); + genesis_config.accounts.insert( + recipient, + Account::new(rent_exempt_minimum, 0, &system_program::id()), + ); + + // Activate features, including require_rent_exempt_accounts + activate_all_features(&mut genesis_config); + + let bank = Bank::new_for_tests(&genesis_config); + let recent_blockhash = bank.last_blockhash(); + + let check_account_is_rent_exempt = |pubkey: &Pubkey| -> bool { + let account = bank.get_account(pubkey).unwrap(); + Rent::default().is_exempt(account.lamports(), account.data().len()) + }; + + // Create just-rent-exempt fee-payer + let rent_exempt_fee_payer = Keypair::new(); + bank.transfer( + rent_exempt_minimum, + &mint_keypair, + &rent_exempt_fee_payer.pubkey(), + ) + .unwrap(); + + // RentPaying fee-payer can remain RentPaying + let tx = Transaction::new( + &[&rent_paying_fee_payer, &mint_keypair], + Message::new( + &[system_instruction::transfer( + &mint_keypair.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_paying_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert!(result.is_ok()); + assert!(!check_account_is_rent_exempt( + &rent_paying_fee_payer.pubkey() + )); + + // RentPaying fee-payer can remain RentPaying on failed executed tx + let sender = Keypair::new(); + let fee_payer_balance = bank.get_balance(&rent_paying_fee_payer.pubkey()); + let tx = Transaction::new( + &[&rent_paying_fee_payer, &sender], + Message::new( + &[system_instruction::transfer( + &sender.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_paying_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_ne!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert_ne!( + fee_payer_balance, + bank.get_balance(&rent_paying_fee_payer.pubkey()) + ); + assert!(!check_account_is_rent_exempt( + &rent_paying_fee_payer.pubkey() + )); + + // RentExempt fee-payer cannot become RentPaying via rent + let tx = Transaction::new( + &[&rent_exempt_fee_payer, &mint_keypair], + Message::new( + &[system_instruction::transfer( + &mint_keypair.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + + // RentExempt fee-payer cannot become RentPaying via failed executed tx + let tx = Transaction::new( + &[&rent_exempt_fee_payer, &sender], + Message::new( + &[system_instruction::transfer( + &sender.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + } + #[test] fn test_rent_state_list_len() { let GenesisConfigInfo { From 278632046341a4a834e5da472bf698dffb69039f Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 25 Feb 2022 17:45:54 -0700 Subject: [PATCH 2/4] Check fee-payer rent-state change on load --- runtime/src/account_rent_state.rs | 43 ++++++++++++++++++++++--------- runtime/src/accounts.rs | 17 ++++++++++++ 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/runtime/src/account_rent_state.rs b/runtime/src/account_rent_state.rs index 86808ed6dc0cf6..0fed99a8c9e5de 100644 --- a/runtime/src/account_rent_state.rs +++ b/runtime/src/account_rent_state.rs @@ -3,6 +3,7 @@ use { log::*, solana_sdk::{ account::{AccountSharedData, ReadableAccount}, + pubkey::Pubkey, rent::Rent, transaction::{Result, TransactionError}, transaction_context::TransactionContext, @@ -10,7 +11,7 @@ use { }; #[derive(Debug, PartialEq, IntoEnumIterator)] -pub(crate) enum RentState { +pub enum RentState { Uninitialized, // account.lamports == 0 RentPaying, // 0 < account.lamports < rent-exempt-minimum RentExempt, // account.lamports >= rent-exempt-minimum @@ -55,17 +56,35 @@ pub(crate) fn check_rent_state( index: usize, ) -> Result<()> { if let Some((pre_rent_state, post_rent_state)) = pre_rent_state.zip(post_rent_state) { - submit_rent_state_metrics(pre_rent_state, post_rent_state); - if !post_rent_state.transition_allowed_from(pre_rent_state) { - debug!( - "Account {:?} not rent exempt, state {:?}", - transaction_context.get_key_of_account_at_index(index), - transaction_context - .get_account_at_index(index) - .map(|account| account.borrow()), - ); - return Err(TransactionError::InvalidRentPayingAccount); - } + let expect_msg = "account must exist at TransactionContext index if rent-states are Some"; + check_rent_state_with_account( + pre_rent_state, + post_rent_state, + transaction_context + .get_key_of_account_at_index(index) + .expect(expect_msg), + &transaction_context + .get_account_at_index(index) + .expect(expect_msg) + .borrow(), + )?; + } + Ok(()) +} + +pub(crate) fn check_rent_state_with_account( + pre_rent_state: &RentState, + post_rent_state: &RentState, + address: &Pubkey, + account_state: &AccountSharedData, +) -> Result<()> { + submit_rent_state_metrics(pre_rent_state, post_rent_state); + if !post_rent_state.transition_allowed_from(pre_rent_state) { + debug!( + "Account {:?} not rent exempt, state {:?}", + address, account_state, + ); + return Err(TransactionError::InvalidRentPayingAccount); } Ok(()) } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 6dc827d688a2d9..dd977bb104388e 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -1,5 +1,6 @@ use { crate::{ + account_rent_state::{check_rent_state_with_account, RentState}, accounts_db::{ AccountShrinkThreshold, AccountsAddRootTiming, AccountsDb, AccountsDbConfig, BankHashInfo, ErrorCounters, LoadHint, LoadedAccount, ScanStorageResult, @@ -342,6 +343,7 @@ impl Accounts { if payer_index != 0 { warn!("Payer index should be 0! {:?}", tx); } + let payer_address = accounts[payer_index].0; let payer_account = &mut accounts[payer_index].1; if payer_account.lamports() == 0 { error_counters.account_not_found += 1; @@ -363,10 +365,25 @@ impl Accounts { error_counters.insufficient_funds += 1; return Err(TransactionError::InsufficientFundsForFee); } + let payer_pre_rent_state = + RentState::from_account(payer_account, &rent_collector.rent); payer_account .checked_sub_lamports(fee) .map_err(|_| TransactionError::InsufficientFundsForFee)?; + let rent_state_result = check_rent_state_with_account( + &payer_pre_rent_state, + &RentState::from_account(payer_account, &rent_collector.rent), + &payer_address, + payer_account, + ); + // Feature gate only wraps the actual error return so that the metrics and debug + // logging generated by `check_rent_state_with_account()` can be examined before + // feature activation + if feature_set.is_active(&feature_set::require_rent_exempt_accounts::id()) { + rent_state_result?; + } + let program_indices = message .instructions() .iter() From 07ce3defa195b82623427c6cf5318e868fdf7cfa Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 25 Feb 2022 17:59:30 -0700 Subject: [PATCH 3/4] Add more test cases --- runtime/src/bank.rs | 110 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 4f090be4172a05..506f03ab6dc414 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -16523,6 +16523,19 @@ pub(crate) mod tests { ) .unwrap(); + // Dummy message to determine fee amount + let dummy_message = SanitizedMessage::try_from(Message::new_with_blockhash( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + sol_to_lamports(1.), + )], + Some(&rent_exempt_fee_payer.pubkey()), + &recent_blockhash, + )) + .unwrap(); + let fee = bank.get_fee_for_message(&dummy_message).unwrap(); + // RentPaying fee-payer can remain RentPaying let tx = Transaction::new( &[&rent_paying_fee_payer, &mint_keypair], @@ -16570,6 +16583,23 @@ pub(crate) mod tests { &rent_paying_fee_payer.pubkey() )); + // RentPaying fee-payer can be emptied with fee and transaction + let tx = Transaction::new( + &[&rent_paying_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_paying_fee_payer.pubkey(), + &recipient, + bank.get_balance(&rent_paying_fee_payer.pubkey()) - fee, + )], + Some(&rent_paying_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert!(result.is_ok()); + assert_eq!(0, bank.get_balance(&rent_paying_fee_payer.pubkey())); + // RentExempt fee-payer cannot become RentPaying via rent let tx = Transaction::new( &[&rent_exempt_fee_payer, &mint_keypair], @@ -16613,6 +16643,86 @@ pub(crate) mod tests { assert!(check_account_is_rent_exempt( &rent_exempt_fee_payer.pubkey() )); + + // For good measure, show that a RentExempt fee-payer that is also debited by a transaction + // cannot become RentPaying by that debit, but can still be charged for the fee + bank.transfer(fee, &mint_keypair, &rent_exempt_fee_payer.pubkey()) + .unwrap(); + let fee_payer_balance = bank.get_balance(&rent_exempt_fee_payer.pubkey()); + assert_eq!(fee_payer_balance, rent_exempt_minimum + fee); + let tx = Transaction::new( + &[&rent_exempt_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + fee, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert_eq!( + fee_payer_balance - fee, + bank.get_balance(&rent_exempt_fee_payer.pubkey()) + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + + // Also show that a RentExempt fee-payer can be completely emptied via fee and transaction + bank.transfer(fee + 1, &mint_keypair, &rent_exempt_fee_payer.pubkey()) + .unwrap(); + assert!(bank.get_balance(&rent_exempt_fee_payer.pubkey()) > rent_exempt_minimum + fee); + let tx = Transaction::new( + &[&rent_exempt_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + bank.get_balance(&rent_exempt_fee_payer.pubkey()) - fee, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert!(result.is_ok()); + assert_eq!(0, bank.get_balance(&rent_exempt_fee_payer.pubkey())); + + // ... but not if the fee alone would make it RentPaying + bank.transfer( + rent_exempt_minimum + 1, + &mint_keypair, + &rent_exempt_fee_payer.pubkey(), + ) + .unwrap(); + assert!(bank.get_balance(&rent_exempt_fee_payer.pubkey()) < rent_exempt_minimum + fee); + let tx = Transaction::new( + &[&rent_exempt_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + bank.get_balance(&rent_exempt_fee_payer.pubkey()) - fee, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); } #[test] From b06bde0e218b5dc727207ba2b523c92d4dc6f076 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 25 Feb 2022 20:58:24 -0700 Subject: [PATCH 4/4] Review comments --- runtime/src/account_rent_state.rs | 4 ++-- runtime/src/accounts.rs | 9 +++++---- runtime/src/bank.rs | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/runtime/src/account_rent_state.rs b/runtime/src/account_rent_state.rs index 0fed99a8c9e5de..ec70a0541f74af 100644 --- a/runtime/src/account_rent_state.rs +++ b/runtime/src/account_rent_state.rs @@ -11,7 +11,7 @@ use { }; #[derive(Debug, PartialEq, IntoEnumIterator)] -pub enum RentState { +pub(crate) enum RentState { Uninitialized, // account.lamports == 0 RentPaying, // 0 < account.lamports < rent-exempt-minimum RentExempt, // account.lamports >= rent-exempt-minimum @@ -81,7 +81,7 @@ pub(crate) fn check_rent_state_with_account( submit_rent_state_metrics(pre_rent_state, post_rent_state); if !post_rent_state.transition_allowed_from(pre_rent_state) { debug!( - "Account {:?} not rent exempt, state {:?}", + "Account {} not rent exempt, state {:?}", address, account_state, ); return Err(TransactionError::InvalidRentPayingAccount); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index dd977bb104388e..f933b838b442c1 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -343,8 +343,7 @@ impl Accounts { if payer_index != 0 { warn!("Payer index should be 0! {:?}", tx); } - let payer_address = accounts[payer_index].0; - let payer_account = &mut accounts[payer_index].1; + let (ref payer_address, ref mut payer_account) = accounts[payer_index]; if payer_account.lamports() == 0 { error_counters.account_not_found += 1; return Err(TransactionError::AccountNotFound); @@ -371,10 +370,12 @@ impl Accounts { .checked_sub_lamports(fee) .map_err(|_| TransactionError::InsufficientFundsForFee)?; + let payer_post_rent_state = + RentState::from_account(payer_account, &rent_collector.rent); let rent_state_result = check_rent_state_with_account( &payer_pre_rent_state, - &RentState::from_account(payer_account, &rent_collector.rent), - &payer_address, + &payer_post_rent_state, + payer_address, payer_account, ); // Feature gate only wraps the actual error return so that the metrics and debug diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 506f03ab6dc414..3bfa88a5735273 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -16600,7 +16600,7 @@ pub(crate) mod tests { assert!(result.is_ok()); assert_eq!(0, bank.get_balance(&rent_paying_fee_payer.pubkey())); - // RentExempt fee-payer cannot become RentPaying via rent + // RentExempt fee-payer cannot become RentPaying from transaction fee let tx = Transaction::new( &[&rent_exempt_fee_payer, &mint_keypair], Message::new(