Skip to content

Commit

Permalink
Perform fee padding in a larger bit width of integers (#70)
Browse files Browse the repository at this point in the history
* Modify fee calculation to use higher int size
* Add tests for various overflow cases
* Simplify behavior and add another overflow check
  • Loading branch information
Shaptic committed Feb 21, 2024
1 parent cbafd84 commit a3047e5
Showing 1 changed file with 81 additions and 9 deletions.
90 changes: 81 additions & 9 deletions cmd/crates/soroban-rpc/src/txn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,15 +313,25 @@ pub fn assemble(
}
}

// update the fees of the actual transaction to meet the minimum resource fees.
let classic_transaction_fees = crate::DEFAULT_TRANSACTION_FEES;
// Pad the fees up by 15% for a bit of wiggle room.
tx.fee = (tx.fee.max(
classic_transaction_fees
+ u32::try_from(simulation.min_resource_fee)
.map_err(|_| Error::LargeFee(simulation.min_resource_fee))?,
) * 115)
/ 100;
// Update transaction fees to meet the minimum resource fees.
let classic_tx_fee: u64 = crate::DEFAULT_TRANSACTION_FEES.into();

// Choose larger of existing fee or inclusion + resource fee.
let base_tx_fee: u64 = tx
.fee
.max(
u32::try_from(classic_tx_fee + simulation.min_resource_fee)
.map_err(|_| Error::LargeFee(simulation.min_resource_fee + classic_tx_fee))?,
)
.into(); // invariant: base_tx_fee <= u32::MAX

// Pad the total fee by up to 15% for a bit of wiggle room.
//
// We know for a fact because of the .min() call that we will not exceed
// U32_MAX, but we can't get clippy to shut the fuck up for this single
// line, so we have a redundant error check anyway.
tx.fee = u32::try_from((base_tx_fee * 115 / 100).min(u64::from(u32::MAX)))
.map_err(|_| Error::LargeFee(base_tx_fee))?;

tx.operations = vec![op].try_into()?;
tx.ext = TransactionExt::V1(transaction_data);
Expand Down Expand Up @@ -721,4 +731,66 @@ mod tests {
r => panic!("expected UnexpectedSimulateTransactionResultSize error, got: {r:#?}"),
}
}

#[test]
fn test_assemble_transaction_overflow_behavior() {
//
// Test three separate cases:
//
// 1. Given a near-max (U32_MAX) resource fee, ensure the "wiggle room"
// doesn't cause an overflow due to correct math order.
// (Specifically, do U32_MAX - 15% - 100 - 1, so the final fee is
// U32_MAX-1.)
//
// 2. Given a large resource fee that WILL exceed on its own with the
// inclusion fee, ensure the overflow is caught with an error rather
// than silently ignored.
//
// 3. Given a max resource fee that exceeds U32_MAX *only with* wiggle
// room, ensure the overflow is ignored and we cap on the max.
//
let txn = single_contract_fn_transaction();
let mut response = simulation_response();

// sanity check so these can be adjusted if the above helper changes
assert_eq!(txn.fee, 100, "modified txn.fee: update the math below");

// 1: wiggle room math overflows but result fits
let mut resource_fee = (u64::from(u32::MAX) * 85 / 100) - 100 - 1;
assert_eq!(resource_fee, 3_650_722_099);
response.min_resource_fee = resource_fee;

match assemble(&txn, &response) {
Ok(asstxn) => {
let expected = (resource_fee + 100) * 115 / 100;
assert_eq!(asstxn.fee, u32::try_from(expected).unwrap());
}
r => panic!("expected success, got: {r:#?}"),
}

// 2: combo overflows, should throw
resource_fee = u64::from(u32::MAX);
assert_eq!(resource_fee, 4_294_967_295);
response.min_resource_fee = resource_fee;

match assemble(&txn, &response) {
Err(Error::LargeFee(fee)) => {
let expected = resource_fee + 100;
assert_eq!(expected, fee, "expected {expected} != {fee} actual");
}
r => panic!("expected LargeFee error, got: {r:#?}"),
}

// 2: combo works but wiggle room overflows, should cap
resource_fee = u64::from(u32::MAX) * 90 / 100;
assert_eq!(resource_fee, 3_865_470_565);
response.min_resource_fee = resource_fee;

match assemble(&txn, &response) {
Ok(asstxn) => {
assert_eq!(asstxn.fee, u32::MAX);
}
r => panic!("expected success, got: {r:#?}"),
}
}
}

0 comments on commit a3047e5

Please sign in to comment.