Skip to content

Commit

Permalink
use checked arithmetic (#31259)
Browse files Browse the repository at this point in the history 
* use checked arithmetic
* add feature gate; updated tests
* allow collapsible-else-if

Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: mvines <[email protected]>
  • Loading branch information
@tao-stones @t-nelson @mvines
3 people authored Apr 21, 2023
1 parent 94350f0 commit b4331ae
Show file tree
Hide file tree
Showing 2 changed files with 217 additions and 3 deletions.
215 changes: 212 additions & 3 deletions runtime/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,10 +594,24 @@ impl Accounts {
}
};

if payer_account.lamports() < fee + min_balance {
error_counters.insufficient_funds += 1;
return Err(TransactionError::InsufficientFundsForFee);
// allow collapsible-else-if to make removing the feature gate safer once activated
#[allow(clippy::collapsible_else_if)]
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)
Expand Down Expand Up @@ -4319,4 +4333,199 @@ mod tests {
(Err(TransactionError::InsufficientFundsForFee), None),
);
}

struct ValidateFeePayerTestParameter {
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: ValidateFeePayerTestParameter,
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(
0,
EpochSchedule::default(),
500_000.0,
Rent {
lamports_per_byte_year: 1,
..Rent::default()
},
);
let min_balance = rent_collector.rent.minimum_balance(NonceState::size());
let fee = 5_000;

// If payer account has sufficient balance, expect successful fee deduction,
// regardless feature gate status, or if payer is nonce account.
{
for feature_checked_arithmmetic_enable in [true, false] {
for (is_nonce, min_balance) in [(true, min_balance), (false, 0)] {
validate_fee_payer_account(
ValidateFeePayerTestParameter {
is_nonce,
payer_init_balance: min_balance + fee,
fee,
expected_result: Ok(()),
payer_post_balance: min_balance,
feature_checked_arithmmetic_enable,
},
&rent_collector,
);
}
}
}

// If payer account has no balance, expected AccountNotFound Error
// regardless feature gate status, or if payer is nonce account.
{
for feature_checked_arithmmetic_enable in [true, false] {
for is_nonce in [true, false] {
validate_fee_payer_account(
ValidateFeePayerTestParameter {
is_nonce,
payer_init_balance: 0,
fee,
expected_result: Err(TransactionError::AccountNotFound),
payer_post_balance: 0,
feature_checked_arithmmetic_enable,
},
&rent_collector,
);
}
}
}

// If payer account has insufficent balance, expect InsufficientFundsForFee error
// regardless feature gate status, or if payer is nonce account.
{
for feature_checked_arithmmetic_enable in [true, false] {
for (is_nonce, min_balance) in [(true, min_balance), (false, 0)] {
validate_fee_payer_account(
ValidateFeePayerTestParameter {
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 balance of u64::MAX, so does fee; since it does not require
// min_balance, expect successful fee deduction, regardless of feature gate status
{
for feature_checked_arithmmetic_enable in [true, false] {
validate_fee_payer_account(
ValidateFeePayerTestParameter {
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 if feature gate is
// enabled
validate_fee_payer_account(
ValidateFeePayerTestParameter {
is_nonce: true,
payer_init_balance: u64::MAX,
fee: u64::MAX,
expected_result: Err(TransactionError::InsufficientFundsForFee),
payer_post_balance: u64::MAX,
feature_checked_arithmmetic_enable: true,
},
&rent_collector,
);
}

#[test]
#[should_panic]
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()
},
);

// same test setup as `test_validate_nonce_fee_payer_with_checked_arithmetic`:
// nonce payer account has balance of u64::MAX, so does fee; and nonce account
// requires additional min_balance, if feature gate is not enabled, in `debug`
// mode, `u64::MAX + min_balance` would panic on "attempt to add with overflow";
// in `release` mode, the addition will wrap, so the expected result would be
// `Ok(())` with post payer balance `0`, therefore fails test with a panic.
validate_fee_payer_account(
ValidateFeePayerTestParameter {
is_nonce: true,
payer_init_balance: u64::MAX,
fee: u64::MAX,
expected_result: Err(TransactionError::InsufficientFundsForFee),
payer_post_balance: u64::MAX,
feature_checked_arithmmetic_enable: false,
},
&rent_collector,
);
}
}
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pubkey, &'static str> = [
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit b4331ae

Please sign in to comment.