max_fee_per_gas and max_priority_fee_per_gas fixes (polkadot-evm#618

)

* `max_fee_per_gas` and `max_priority_fee_per_gas` fixes

* fmt

* Handle `block_count` 0

client/rpc/src/eth/fee.rs

@@ -90,7 +90,7 @@ where
 			};
 			// Highest and lowest block number within the requested range.
 			let highest = UniqueSaturatedInto::<u64>::unique_saturated_into(number);
-			let lowest = highest.saturating_sub(block_count);
+			let lowest = highest.saturating_sub(block_count.saturating_sub(1));
 			// Tip of the chain.
 			let best_number =
 				UniqueSaturatedInto::<u64>::unique_saturated_into(self.client.info().best_number);
@@ -130,7 +130,10 @@ where
 								block_rewards.push(reward);
 							}
 							// Push block rewards.
-							rewards.push(block_rewards);
+							if !block_rewards.is_empty() {
+								// Push block rewards.
+								rewards.push(block_rewards);
+							}
 						}
 					}
 				}

client/rpc/src/eth/transaction.rs

@@ -411,7 +411,8 @@ where
 							.base_fee(&id)
 							.unwrap_or_default()
 							.checked_add(t.max_priority_fee_per_gas)
-							.unwrap_or(U256::max_value()),
+							.unwrap_or(U256::max_value())
+							.min(t.max_fee_per_gas),
 					};
 
 					return Ok(Some(Receipt {

frame/ethereum/src/lib.rs

@@ -525,32 +525,36 @@ impl<T: Config> Pallet<T> {
 		let base_fee = T::FeeCalculator::min_gas_price();
 		let mut priority = 0;
 
-		let gas_price = if let Some(gas_price) = transaction_data.gas_price {
-			// Legacy and EIP-2930 transactions.
+		let max_fee_per_gas = match (
+			transaction_data.gas_price,
+			transaction_data.max_fee_per_gas,
+			transaction_data.max_priority_fee_per_gas,
+		) {
+			// Legacy or EIP-2930 transaction.
 			// Handle priority here. On legacy transaction everything in gas_price except
 			// the current base_fee is considered a tip to the miner and thus the priority.
-			priority = gas_price.saturating_sub(base_fee).unique_saturated_into();
-			gas_price
-		} else if let Some(max_fee_per_gas) = transaction_data.max_fee_per_gas {
-			// EIP-1559 transactions.
-			max_fee_per_gas
-		} else {
-			return Err(InvalidTransaction::Payment.into());
+			(Some(gas_price), None, None) => {
+				priority = gas_price.saturating_sub(base_fee).unique_saturated_into();
+				gas_price
+			}
+			// EIP-1559 transaction without tip.
+			(None, Some(max_fee_per_gas), None) => max_fee_per_gas,
+			// EIP-1559 transaction with tip.
+			(None, Some(max_fee_per_gas), Some(max_priority_fee_per_gas)) => {
+				priority = max_fee_per_gas
+					.saturating_sub(base_fee)
+					.min(max_priority_fee_per_gas)
+					.unique_saturated_into();
+				max_fee_per_gas
+			}
+			_ => return Err(InvalidTransaction::Payment.into()),
 		};
 
-		if gas_price < base_fee {
+		if max_fee_per_gas < base_fee {
 			return Err(InvalidTransaction::Payment.into());
 		}
 
-		let mut fee = gas_price.saturating_mul(gas_limit);
-		if let Some(max_priority_fee_per_gas) = transaction_data.max_priority_fee_per_gas {
-			// EIP-1559 transaction priority is determined by `max_priority_fee_per_gas`.
-			// If the transaction do not include this optional parameter, priority is now considered zero.
-			priority = max_priority_fee_per_gas.unique_saturated_into();
-			// Add the priority tip to the payable fee.
-			fee = fee.saturating_add(max_priority_fee_per_gas.saturating_mul(gas_limit));
-		}
-
+		let fee = max_fee_per_gas.saturating_mul(gas_limit);
 		let account_data = pallet_evm::Pallet::<T>::account_basic(&origin);
 		let total_payment = transaction_data.value.saturating_add(fee);
 		if account_data.balance < total_payment {
@@ -734,29 +738,17 @@ impl<T: Config> Pallet<T> {
 			match transaction {
 				// max_fee_per_gas and max_priority_fee_per_gas in legacy and 2930 transactions is
 				// the provided gas_price.
-				Transaction::Legacy(t) => {
-					let base_fee = T::FeeCalculator::min_gas_price();
-					let priority_fee = t
-						.gas_price
-						.checked_sub(base_fee)
-						.ok_or_else(|| DispatchError::Other("Gas price too low"))?;
-					(
-						t.input.clone(),
-						t.value,
-						t.gas_limit,
-						Some(base_fee),
-						Some(priority_fee),
-						Some(t.nonce),
-						t.action,
-						Vec::new(),
-					)
-				}
+				Transaction::Legacy(t) => (
+					t.input.clone(),
+					t.value,
+					t.gas_limit,
+					Some(t.gas_price),
+					Some(t.gas_price),
+					Some(t.nonce),
+					t.action,
+					Vec::new(),
+				),
 				Transaction::EIP2930(t) => {
-					let base_fee = T::FeeCalculator::min_gas_price();
-					let priority_fee = t
-						.gas_price
-						.checked_sub(base_fee)
-						.ok_or_else(|| DispatchError::Other("Gas price too low"))?;
 					let access_list: Vec<(H160, Vec<H256>)> = t
 						.access_list
 						.iter()
@@ -766,8 +758,8 @@ impl<T: Config> Pallet<T> {
 						t.input.clone(),
 						t.value,
 						t.gas_limit,
-						Some(base_fee),
-						Some(priority_fee),
+						Some(t.gas_price),
+						Some(t.gas_price),
 						Some(t.nonce),
 						t.action,
 						access_list,

frame/ethereum/src/tests/eip1559.rs

@@ -88,7 +88,7 @@ fn transaction_with_to_low_nonce_should_not_work() {
 			call.validate_self_contained(&source).unwrap(),
 			ValidTransactionBuilder::default()
 				.and_provides((alice.address, U256::from(1)))
-				.priority(1u64)
+				.priority(0u64)
 				.and_requires((alice.address, U256::from(0)))
 				.build()
 		);

frame/evm/src/runner/stack.rs

@@ -65,39 +65,28 @@ impl<T: Config> Runner<T> {
 		) -> (ExitReason, R),
 	{
 		let base_fee = T::FeeCalculator::min_gas_price();
-		// Gas price check is skipped when performing a gas estimation.
-		let max_fee_per_gas = match max_fee_per_gas {
-			Some(max_fee_per_gas) => {
+
+		let max_fee_per_gas = match (max_fee_per_gas, max_priority_fee_per_gas) {
+			(Some(max_fee_per_gas), Some(max_priority_fee_per_gas)) => {
 				ensure!(max_fee_per_gas >= base_fee, Error::<T>::GasPriceTooLow);
+				ensure!(
+					max_fee_per_gas >= max_priority_fee_per_gas,
+					Error::<T>::GasPriceTooLow
+				);
 				max_fee_per_gas
 			}
-			None => Default::default(),
-		};
-
-		let vicinity = Vicinity {
-			gas_price: max_fee_per_gas,
-			origin: source,
+			(Some(max_fee_per_gas), None) => {
+				ensure!(max_fee_per_gas >= base_fee, Error::<T>::GasPriceTooLow);
+				max_fee_per_gas
+			}
+			// Gas price check is skipped when performing a gas estimation.
+			_ => Default::default(),
 		};
 
-		let metadata = StackSubstateMetadata::new(gas_limit, &config);
-		let state = SubstrateStackState::new(&vicinity, metadata);
-		let mut executor = StackExecutor::new_with_precompiles(state, config, precompiles);
-
 		// After eip-1559 we make sure the account can pay both the evm execution and priority fees.
-		let max_base_fee = max_fee_per_gas
+		let total_fee = max_fee_per_gas
 			.checked_mul(U256::from(gas_limit))
 			.ok_or(Error::<T>::FeeOverflow)?;
-		let max_priority_fee = if let Some(max_priority_fee) = max_priority_fee_per_gas {
-			max_priority_fee
-				.checked_mul(U256::from(gas_limit))
-				.ok_or(Error::<T>::FeeOverflow)?
-		} else {
-			U256::zero()
-		};
-
-		let total_fee = max_base_fee
-			.checked_add(max_priority_fee)
-			.ok_or(Error::<T>::FeeOverflow)?;
 
 		let total_payment = value
 			.checked_add(total_fee)
@@ -115,12 +104,24 @@ impl<T: Config> Runner<T> {
 		let fee = T::OnChargeTransaction::withdraw_fee(&source, total_fee)?;
 
 		// Execute the EVM call.
+		let vicinity = Vicinity {
+			gas_price: base_fee,
+			origin: source,
+		};
+
+		let metadata = StackSubstateMetadata::new(gas_limit, &config);
+		let state = SubstrateStackState::new(&vicinity, metadata);
+		let mut executor = StackExecutor::new_with_precompiles(state, config, precompiles);
+
 		let (reason, retv) = f(&mut executor);
 
+		// Post execution.
 		let used_gas = U256::from(executor.used_gas());
 		let (actual_fee, actual_priority_fee) =
 			if let Some(max_priority_fee) = max_priority_fee_per_gas {
-				let actual_priority_fee = max_priority_fee
+				let actual_priority_fee = max_fee_per_gas
+					.saturating_sub(base_fee)
+					.min(max_priority_fee)
 					.checked_mul(U256::from(used_gas))
 					.ok_or(Error::<T>::FeeOverflow)?;
 				let actual_fee = executor
@@ -156,11 +157,11 @@ impl<T: Config> Runner<T> {
 		// |        5 |        2 |
 		// +----------+----------+
 		//
-		// Initially withdrawn (10 + 6) * 20 = 320.
+		// Initially withdrawn 10 * 20 = 200.
 		// Actual cost (2 + 6) * 5 = 40.
-		// Refunded 320 - 40 = 280.
+		// Refunded 200 - 40 = 160.
 		// Tip 5 * 6 = 30.
-		// Burned 320 - (280 + 30) = 10. Which is equivalent to gas_used * base_fee.
+		// Burned 200 - (160 + 30) = 10. Which is equivalent to gas_used * base_fee.
 		T::OnChargeTransaction::correct_and_deposit_fee(&source, actual_fee, fee);
 		if let Some(actual_priority_fee) = actual_priority_fee {
 			T::OnChargeTransaction::pay_priority_fee(actual_priority_fee);

frame/evm/src/tests.rs

@@ -231,18 +231,19 @@ fn author_should_get_tip() {
 	new_test_ext().execute_with(|| {
 		let author = EVM::find_author();
 		let before_tip = EVM::account_basic(&author).balance;
-		let _ = EVM::call(
+		let result = EVM::call(
 			Origin::root(),
 			H160::default(),
 			H160::from_str("1000000000000000000000000000000000000001").unwrap(),
 			Vec::new(),
 			U256::from(1),
 			1000000,
-			U256::from(1_000_000_000),
+			U256::from(2_000_000_000),
 			Some(U256::from(1)),
 			None,
 			Vec::new(),
 		);
+		result.expect("EVM can be called");
 		let after_tip = EVM::account_basic(&author).balance;
 		assert_eq!(after_tip, (before_tip + 21000));
 	});
@@ -303,29 +304,83 @@ fn refunds_and_priority_should_work() {
 		let author = EVM::find_author();
 		let before_tip = EVM::account_basic(&author).balance;
 		let before_call = EVM::account_basic(&H160::default()).balance;
-		let tip = 5;
-		// The tip is deducted but never refunded to the caller.
-		let _ = EVM::call(
+		// We deliberately set a base fee + max tip > max fee.
+		// The effective priority tip will be 1GWEI instead 1.5GWEI:
+		// 		(max_fee_per_gas - base_fee).min(max_priority_fee)
+		//		(2 - 1).min(1.5)
+		let tip = U256::from(1_500_000_000);
+		let max_fee_per_gas = U256::from(2_000_000_000);
+		let used_gas = U256::from(21_000);
+		let result = EVM::call(
 			Origin::root(),
 			H160::default(),
 			H160::from_str("1000000000000000000000000000000000000001").unwrap(),
 			Vec::new(),
 			U256::from(1),
 			1000000,
-			U256::from(2_000_000_000),
-			Some(U256::from(tip)),
+			max_fee_per_gas,
+			Some(tip),
 			None,
 			Vec::new(),
 		);
-		let tip = tip * 21000;
-		let total_cost = (U256::from(21_000) * <Test as Config>::FeeCalculator::min_gas_price())
-			+ U256::from(1)
-			+ U256::from(tip);
+		let base_fee = <Test as Config>::FeeCalculator::min_gas_price();
+		let actual_tip = (max_fee_per_gas - base_fee).min(tip) * used_gas;
+		let total_cost = (used_gas * base_fee) + U256::from(actual_tip) + U256::from(1);
 		let after_call = EVM::account_basic(&H160::default()).balance;
+		// The tip is deducted but never refunded to the caller.
 		assert_eq!(after_call, before_call - total_cost);
 
 		let after_tip = EVM::account_basic(&author).balance;
-		assert_eq!(after_tip, (before_tip + tip));
+		assert_eq!(after_tip, (before_tip + actual_tip.low_u128()));
+	});
+}
+
+#[test]
+fn call_should_fail_with_priority_greater_than_max_fee() {
+	new_test_ext().execute_with(|| {
+		let author = EVM::find_author();
+		let before_tip = EVM::account_basic(&author).balance;
+		let before_call = EVM::account_basic(&H160::default()).balance;
+		// Max priority greater than max fee should fail.
+		let tip: u128 = 1_100_000_000;
+		let result = EVM::call(
+			Origin::root(),
+			H160::default(),
+			H160::from_str("1000000000000000000000000000000000000001").unwrap(),
+			Vec::new(),
+			U256::from(1),
+			1000000,
+			U256::from(1_000_000_000),
+			Some(U256::from(tip)),
+			None,
+			Vec::new(),
+		);
+		assert!(result.is_err());
+	});
+}
+
+#[test]
+fn call_should_succeed_with_priority_equal_to_max_fee() {
+	new_test_ext().execute_with(|| {
+		let author = EVM::find_author();
+		let before_tip = EVM::account_basic(&author).balance;
+		let before_call = EVM::account_basic(&H160::default()).balance;
+		let tip: u128 = 1_000_000_000;
+		// Mimics the input for pre-eip-1559 transaction types where `gas_price`
+		// is used for both `max_fee_per_gas` and `max_priority_fee_per_gas`.
+		let result = EVM::call(
+			Origin::root(),
+			H160::default(),
+			H160::from_str("1000000000000000000000000000000000000001").unwrap(),
+			Vec::new(),
+			U256::from(1),
+			1000000,
+			U256::from(1_000_000_000),
+			Some(U256::from(tip)),
+			None,
+			Vec::new(),
+		);
+		assert!(result.is_ok());
 	});
 }