EIP1559 strict balance validation

frame/evm/src/runner/stack.rs

@@ -65,27 +65,42 @@ where
 		) -> (ExitReason, R),
 	{
 		let (base_fee, weight) = T::FeeCalculator::min_gas_price();
-		let max_fee_per_gas = match (max_fee_per_gas, is_transactional) {
-			(Some(max_fee_per_gas), _) => max_fee_per_gas,
-			// Gas price check is skipped for non-transactional calls that don't
-			// define a `max_fee_per_gas` input.
-			(None, false) => Default::default(),
-			// Unreachable, previously validated. Handle gracefully.
-			_ => {
-				return Err(RunnerError {
-					error: Error::<T>::GasPriceTooLow,
-					weight,
-				})
-			}
-		};
+		let (total_fee_per_gas, actual_priority_fee_per_gas) =
+			match (max_fee_per_gas, max_priority_fee_per_gas, is_transactional) {
+				// With no tip, we pay exactly the base_fee
+				// TODO: should this fn explicitly check that base_fee <= max_fee_per_gas?
+				(Some(_), None, _) => (base_fee, U256::zero()),
+				// With tip, we include as much of the tip on top of base_fee that we can, never
+				// exceeding max_fee_per_gas
+				(Some(max_fee_per_gas), Some(max_priority_fee_per_gas), _) => {
+					let actual_priority_fee_per_gas = max_fee_per_gas
+						.saturating_sub(base_fee)
+						.min(max_priority_fee_per_gas);
+					(
+						base_fee.saturating_add(actual_priority_fee_per_gas),
+						actual_priority_fee_per_gas,
+					)
+				}
+				// Gas price check is skipped for non-transactional calls that don't
+				// define a `max_fee_per_gas` input.
+				(None, _, false) => (Default::default(), U256::zero()),
+				// Unreachable, previously validated. Handle gracefully.
+				_ => {
+					return Err(RunnerError {
+						error: Error::<T>::GasPriceTooLow,
+						weight,
+					})
+				}
+			};
 
 		// After eip-1559 we make sure the account can pay both the evm execution and priority fees.
-		let total_fee = max_fee_per_gas
-			.checked_mul(U256::from(gas_limit))
-			.ok_or(RunnerError {
-				error: Error::<T>::FeeOverflow,
-				weight,
-			})?;
+		let total_fee =
+			total_fee_per_gas
+				.checked_mul(U256::from(gas_limit))
+				.ok_or(RunnerError {
+					error: Error::<T>::FeeOverflow,
+					weight,
+				})?;
 
 		// Deduct fee from the `source` account. Returns `None` if `total_fee` is Zero.
 		let fee = T::OnChargeTransaction::withdraw_fee(&source, total_fee)
@@ -105,18 +120,12 @@ where
 
 		// Post execution.
 		let used_gas = U256::from(executor.used_gas());
-		let actual_fee = if let Some(max_priority_fee) = max_priority_fee_per_gas {
-			let actual_priority_fee = max_fee_per_gas
-				.saturating_sub(base_fee)
-				.min(max_priority_fee)
-				.saturating_mul(used_gas);
-			executor
-				.fee(base_fee)
-				.checked_add(actual_priority_fee)
-				.unwrap_or_else(U256::max_value)
-		} else {
-			executor.fee(base_fee)
-		};
+		// TODO: after simplifying the logic here, we no longer care about the specified priority
+		// fee except for calculating the total fee paid.
+		// When paying out the priority fee, we let correct_and_deposit_fee() come up with an
+		// arbitrary value that is used as the tip to pay.
+		// Assuming this is ok, actual_priority_fee_per_gas does not need to be returned above.
+		let actual_fee = executor.fee(total_fee_per_gas);
 		log::debug!(
 			target: "evm",
 			"Execution {:?} [source: {:?}, value: {}, gas_limit: {}, actual_fee: {}, is_transactional: {}]",

primitives/evm/src/validation.rs

@@ -116,23 +116,18 @@ impl<'config, E: From<InvalidEvmTransactionError>> CheckEvmTransaction<'config,
 
 	pub fn with_balance_for(&self, who: &Account) -> Result<&Self, E> {
 		// Get fee data from either a legacy or typed transaction input.
-		let (_, effective_gas_price) = self.transaction_fee_input()?;
+		let (max_fee_per_gas, _) = self.transaction_fee_input()?;
 
 		// Account has enough funds to pay for the transaction.
 		// Check is skipped on non-transactional calls that don't provide
 		// a gas price input.
 		//
-		// Fee for EIP-1559 transaction **with** tip is calculated using
-		// the effective gas price.
-		//
-		// Fee for EIP-1559 transaction **without** tip is calculated using
-		// the base fee.
+		// Validation for EIP-1559 is done using the max_fee_per_gas, which is
+		// the most a txn could possibly pay.
 		//
 		// Fee for Legacy or EIP-2930 transaction is calculated using
 		// the provided `gas_price`.
-		let fee = effective_gas_price
-			.unwrap_or_default()
-			.saturating_mul(self.transaction.gas_limit);
+		let fee = max_fee_per_gas.saturating_mul(self.transaction.gas_limit);
 		if self.config.is_transactional || fee > U256::zero() {
 			let total_payment = self.transaction.value.saturating_add(fee);
 			if who.balance < total_payment {
@@ -142,6 +137,9 @@ impl<'config, E: From<InvalidEvmTransactionError>> CheckEvmTransaction<'config,
 		Ok(self)
 	}
 
+	// Returns the max_fee_per_gas (or gas_price for legacy txns) as well as an optional
+	// effective_gas_price for EIP-1559 transactions. effective_gas_price represents
+	// the total (fee + tip) that would be paid given the current base_fee.
 	fn transaction_fee_input(&self) -> Result<(U256, Option<U256>), E> {
 		match (
 			self.transaction.gas_price,
@@ -659,29 +657,31 @@ mod tests {
 	}
 
 	#[test]
-	// Account balance is matched against the base fee without tip.
-	fn validate_balance_using_base_fee() {
+	// Account balance is matched against max_fee_per_gas (without txn tip)
+	fn validate_balance_regardless_of_base_fee() {
 		let who = Account {
+			// sufficient for base_fee, but not for max_fee_per_gas
 			balance: U256::from(21_000_000_000_001u128),
 			nonce: U256::zero(),
 		};
 		let with_tip = false;
 		let test = transaction_max_fee_high(with_tip);
 		let res = test.with_balance_for(&who);
-		assert!(res.is_ok());
+		assert!(res.is_err());
 	}
 
 	#[test]
-	// Account balance is matched against the effective gas price with tip.
-	fn validate_balance_using_effective_gas_price() {
+	// Account balance is matched against max_fee_per_gas (with txn tip)
+	fn validate_balance_regardless_of_effective_gas_price() {
 		let who = Account {
+			// sufficient for (base_fee + tip), but not for max_fee_per_gas
 			balance: U256::from(42_000_000_000_001u128),
 			nonce: U256::zero(),
 		};
 		let with_tip = true;
 		let test = transaction_max_fee_high(with_tip);
 		let res = test.with_balance_for(&who);
-		assert!(res.is_ok());
+		assert!(res.is_err());
 	}
 
 	#[test]