From 7e62a48aa86e1add57ee127cc92e76b5c1a95b0d Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Thu, 20 Apr 2023 00:37:02 +0000 Subject: [PATCH] add feature gate; updated tests --- runtime/src/accounts.rs | 273 ++++++++++++++++++++++------------------ sdk/src/feature_set.rs | 5 + 2 files changed, 158 insertions(+), 120 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 1ac4aeca261089..6373a23a66ca71 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -594,14 +594,20 @@ impl Accounts { } }; - payer_account - .lamports() - .checked_sub(min_balance) - .and_then(|v| v.checked_sub(fee)) - .ok_or_else(|| { - error_counters.insufficient_funds += 1; - TransactionError::InsufficientFundsForFee - })?; + if feature_set.is_active(&feature_set::checked_arithmetic_in_fee_validation::id()) { + payer_account + .lamports() + .checked_sub(min_balance) + .and_then(|v| v.checked_sub(fee)) + .ok_or_else(|| { + error_counters.insufficient_funds += 1; + TransactionError::InsufficientFundsForFee + })?; + } else if payer_account.lamports() < fee + min_balance { + 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) @@ -4324,6 +4330,45 @@ mod tests { ); } + struct TestParameter { + is_nonce: bool, + payer_init_balance: u64, + fee: u64, + expected_result: Result<()>, + payer_post_balance: u64, + feature_checked_arithmmetic_enable: bool, + } + + fn validate_fee_payer_account(test_parameter: TestParameter, rent_collector: &RentCollector) { + let payer_account_keys = Keypair::new(); + let mut account = if test_parameter.is_nonce { + AccountSharedData::new_data( + test_parameter.payer_init_balance, + &NonceVersions::new(NonceState::Initialized(nonce::state::Data::default())), + &system_program::id(), + ) + .unwrap() + } else { + AccountSharedData::new(test_parameter.payer_init_balance, 0, &system_program::id()) + }; + let mut feature_set = FeatureSet::default(); + if test_parameter.feature_checked_arithmmetic_enable { + feature_set.activate(&feature_set::checked_arithmetic_in_fee_validation::id(), 0); + }; + let result = Accounts::validate_fee_payer( + &payer_account_keys.pubkey(), + &mut account, + 0, + &mut TransactionErrorMetrics::default(), + rent_collector, + &feature_set, + test_parameter.fee, + ); + + assert_eq!(result, test_parameter.expected_result); + assert_eq!(account.lamports(), test_parameter.payer_post_balance); + } + #[test] fn test_validate_fee_payer() { let rent_collector = RentCollector::new( @@ -4338,126 +4383,100 @@ mod tests { let min_balance = rent_collector.rent.minimum_balance(NonceState::size()); let fee = 5_000; - struct TestParameter { - is_nonce: bool, - payer_init_balance: u64, - fee: u64, - expected_result: Result<()>, - payer_post_balance: u64, - } - - fn validate_fee_payer_account( - test_parameter: TestParameter, - rent_collector: &RentCollector, - ) { - let payer_account_keys = Keypair::new(); - let mut account = if test_parameter.is_nonce { - AccountSharedData::new_data( - test_parameter.payer_init_balance, - &NonceVersions::new(NonceState::Initialized(nonce::state::Data::default())), - &system_program::id(), - ) - .unwrap() - } else { - AccountSharedData::new(test_parameter.payer_init_balance, 0, &system_program::id()) - }; - let result = Accounts::validate_fee_payer( - &payer_account_keys.pubkey(), - &mut account, - 0, - &mut TransactionErrorMetrics::default(), - rent_collector, - &FeatureSet::default(), - test_parameter.fee, - ); - - assert_eq!(result, test_parameter.expected_result); - assert_eq!(account.lamports(), test_parameter.payer_post_balance); - } - - // nonce payer account has sufficient balance, expect successful fee deduction + // If payer account has sufficient balance, expect successful fee deduction, + // regardless feature gate status, or if payer is nonce account. { - validate_fee_payer_account( - TestParameter { - is_nonce: true, - payer_init_balance: min_balance + fee, - fee, - expected_result: Ok(()), - payer_post_balance: min_balance, - }, - &rent_collector, - ); - } - // normal payer account has sufficient balance, expect successful fee deduction - { - validate_fee_payer_account( - TestParameter { - is_nonce: false, - payer_init_balance: fee, - fee, - expected_result: Ok(()), - payer_post_balance: 0, - }, - &rent_collector, - ); + for feature_checked_arithmmetic_enable in [true, false] { + for (is_nonce, min_balance) in [(true, min_balance), (false, 0)] { + validate_fee_payer_account( + TestParameter { + is_nonce, + payer_init_balance: min_balance + fee, + fee, + expected_result: Ok(()), + payer_post_balance: min_balance, + feature_checked_arithmmetic_enable, + }, + &rent_collector, + ); + } + } } - // nonce payer account has no balance, expected AccountNotFound Error + // If payer account has no balance, expected AccountNotFound Error + // regardless feature gate status, or if payer is nonce account. { - validate_fee_payer_account( - TestParameter { - is_nonce: true, - payer_init_balance: 0, - fee, - expected_result: Err(TransactionError::AccountNotFound), - payer_post_balance: 0, - }, - &rent_collector, - ); - } - // normal payer account has no balance, expected AccountNotFound Error - { - validate_fee_payer_account( - TestParameter { - is_nonce: false, - payer_init_balance: 0, - fee, - expected_result: Err(TransactionError::AccountNotFound), - payer_post_balance: 0, - }, - &rent_collector, - ); + for feature_checked_arithmmetic_enable in [true, false] { + for is_nonce in [true, false] { + validate_fee_payer_account( + TestParameter { + is_nonce, + payer_init_balance: 0, + fee, + expected_result: Err(TransactionError::AccountNotFound), + payer_post_balance: 0, + feature_checked_arithmmetic_enable, + }, + &rent_collector, + ); + } + } } - // nonce payer account has insufficent balance, expect InsufficientFundsForFee error + // If payer account has insufficent balance, expect InsufficientFundsForFee error + // regardless feature gate status, or if payer is nonce account. { - validate_fee_payer_account( - TestParameter { - is_nonce: true, - payer_init_balance: min_balance + fee - 1, - fee, - expected_result: Err(TransactionError::InsufficientFundsForFee), - payer_post_balance: min_balance + fee - 1, - }, - &rent_collector, - ); + for feature_checked_arithmmetic_enable in [true, false] { + for (is_nonce, min_balance) in [(true, min_balance), (false, 0)] { + validate_fee_payer_account( + TestParameter { + is_nonce, + payer_init_balance: min_balance + fee - 1, + fee, + expected_result: Err(TransactionError::InsufficientFundsForFee), + payer_post_balance: min_balance + fee - 1, + feature_checked_arithmmetic_enable, + }, + &rent_collector, + ); + } + } } - // normal payer account has insufficent balance, expect InsufficientFundsForFee error + + // normal payer account has balance of u64::MAX, so does fee; since it does not require + // min_balance, expect successful fee deduction, regardless of feature gate status { - validate_fee_payer_account( - TestParameter { - is_nonce: false, - payer_init_balance: fee - 1, - fee, - expected_result: Err(TransactionError::InsufficientFundsForFee), - payer_post_balance: fee - 1, - }, - &rent_collector, - ); + for feature_checked_arithmmetic_enable in [true, false] { + validate_fee_payer_account( + TestParameter { + is_nonce: false, + payer_init_balance: u64::MAX, + fee: u64::MAX, + expected_result: Ok(()), + payer_post_balance: 0, + feature_checked_arithmmetic_enable, + }, + &rent_collector, + ); + } } + } + + #[test] + fn test_validate_nonce_fee_payer_with_checked_arithmetic() { + let rent_collector = RentCollector::new( + 0, + EpochSchedule::default(), + 500_000.0, + Rent { + lamports_per_byte_year: 1, + ..Rent::default() + }, + ); // nonce payer account has balance of u64::MAX, so does fee; due to nonce account - // requires additional min_balance, expect InsufficientFundsForFee error + // requires additional min_balance, expect InsufficientFundsForFee error if feature gate is + // enabled { validate_fee_payer_account( TestParameter { @@ -4466,20 +4485,34 @@ mod tests { fee: u64::MAX, expected_result: Err(TransactionError::InsufficientFundsForFee), payer_post_balance: u64::MAX, + feature_checked_arithmmetic_enable: true, }, &rent_collector, ); } - // normal payer account has balance of u64::MAX, so does fee; without requiring - // min_balance, expect successful fee deduction + } + + #[test] + #[should_panic(expected = "attempt to add with overflow")] + fn test_validate_nonce_fee_payer_without_checked_arithmetic() { + let rent_collector = RentCollector::new( + 0, + EpochSchedule::default(), + 500_000.0, + Rent { + lamports_per_byte_year: 1, + ..Rent::default() + }, + ); { validate_fee_payer_account( TestParameter { - is_nonce: false, + is_nonce: true, payer_init_balance: u64::MAX, fee: u64::MAX, expected_result: Ok(()), - payer_post_balance: 0, + payer_post_balance: u64::MAX, + feature_checked_arithmmetic_enable: false, }, &rent_collector, ); diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index c8f9646d773d35..25dfacec6e8e5f 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -658,6 +658,10 @@ pub mod vote_state_add_vote_latency { solana_sdk::declare_id!("7axKe5BTYBDD87ftzWbk5DfzWMGyRvqmWTduuo22Yaqy"); } +pub mod checked_arithmetic_in_fee_validation { + solana_sdk::declare_id!("5Pecy6ie6XGm22pc9d4P9W5c31BugcFBuy6hsP2zkETv"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -817,6 +821,7 @@ lazy_static! { (stop_truncating_strings_in_syscalls::id(), "Stop truncating strings in syscalls #31029"), (clean_up_delegation_errors::id(), "Return InsufficientDelegation instead of InsufficientFunds or InsufficientStake where applicable #31206"), (vote_state_add_vote_latency::id(), "replace Lockout with LandedVote (including vote latency) in vote state #31264"), + (checked_arithmetic_in_fee_validation::id(), "checked arithmetic in fee validation #31273"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()