Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use checked arithmetic #31259

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 212 additions & 3 deletions runtime/src/accounts.rs
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 help removing gate safer.
tao-stones marked this conversation as resolved.
Show resolved Hide resolved
#[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
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