Fix estimate_gas execution doesn't match the actual execution (polk…

…adot-evm#1257) (#18)

* Ensure the actual executed proof base size is the same as the estimate approach

* Add tests

* Fix estimate-gas bug

* Fix the broken ts test, very tricky

* Rewrite the total fee per gas part and code clean

* Fix clippy

client/rpc/src/eth/execute.rs

@@ -462,19 +462,6 @@ where
 			}
 		}
 
-		let (gas_price, max_fee_per_gas, max_priority_fee_per_gas) = {
-			let details = fee_details(
-				request.gas_price,
-				request.max_fee_per_gas,
-				request.max_priority_fee_per_gas,
-			)?;
-			(
-				details.gas_price,
-				details.max_fee_per_gas,
-				details.max_priority_fee_per_gas,
-			)
-		};
-
 		let block_gas_limit = {
 			let schema = fc_storage::onchain_storage_schema(client.as_ref(), substrate_hash);
 			let block = block_data_cache.current_block(schema, substrate_hash).await;
@@ -505,10 +492,23 @@ where
 			},
 		};
 
+		let (gas_price, max_fee_per_gas, max_priority_fee_per_gas, fee_cap) = {
+			let details = fee_details(
+				request.gas_price,
+				request.max_fee_per_gas,
+				request.max_priority_fee_per_gas,
+			)?;
+			(
+				details.gas_price,
+				details.max_fee_per_gas,
+				details.max_priority_fee_per_gas,
+				details.fee_cap,
+			)
+		};
+
 		// Recap the highest gas allowance with account's balance.
 		if let Some(from) = request.from {
-			let gas_price = gas_price.unwrap_or_default();
-			if gas_price > U256::zero() {
+			if fee_cap > U256::zero() {
 				let balance = api
 					.account_basic(substrate_hash, from)
 					.map_err(|err| internal_err(format!("runtime error: {err}")))?
@@ -520,14 +520,14 @@ where
 					}
 					available -= value;
 				}
-				let allowance = available / gas_price;
+				let allowance = available / fee_cap;
 				if highest > allowance {
 					log::warn!(
 							"Gas estimation capped by limited funds original {} balance {} sent {} feecap {} fundable {}",
 							highest,
 							balance,
 							request.value.unwrap_or_default(),
-							gas_price,
+							fee_cap,
 							allowance
 						);
 					highest = allowance;
@@ -1007,50 +1007,49 @@ struct FeeDetails {
 	gas_price: Option<U256>,
 	max_fee_per_gas: Option<U256>,
 	max_priority_fee_per_gas: Option<U256>,
+	fee_cap: U256,
 }
 
 fn fee_details(
 	request_gas_price: Option<U256>,
-	request_max_fee: Option<U256>,
-	request_priority: Option<U256>,
+	request_max_fee_per_gas: Option<U256>,
+	request_priority_fee_per_gas: Option<U256>,
 ) -> RpcResult<FeeDetails> {
-	match (request_gas_price, request_max_fee, request_priority) {
-		(gas_price, None, None) => {
-			// Legacy request, all default to gas price.
-			// A zero-set gas price is None.
-			let gas_price = if gas_price.unwrap_or_default().is_zero() {
-				None
-			} else {
-				gas_price
-			};
-			Ok(FeeDetails {
-				gas_price,
-				max_fee_per_gas: gas_price,
-				max_priority_fee_per_gas: gas_price,
-			})
-		}
-		(_, max_fee, max_priority) => {
-			// eip-1559
-			// A zero-set max fee is None.
-			let max_fee = if max_fee.unwrap_or_default().is_zero() {
-				None
-			} else {
-				max_fee
-			};
-			// Ensure `max_priority_fee_per_gas` is less or equal to `max_fee_per_gas`.
-			if let Some(max_priority) = max_priority {
-				let max_fee = max_fee.unwrap_or_default();
-				if max_priority > max_fee {
-					return Err(internal_err(
-						"Invalid input: `max_priority_fee_per_gas` greater than `max_fee_per_gas`",
-					));
-				}
+	match (
+		request_gas_price,
+		request_max_fee_per_gas,
+		request_priority_fee_per_gas,
+	) {
+		(Some(_), Some(_), Some(_)) => Err(internal_err(
+			"both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified",
+		)),
+		// Legacy or EIP-2930 transaction.
+		(gas_price, None, None) if gas_price.is_some() => Ok(FeeDetails {
+			gas_price,
+			max_fee_per_gas: None,
+			max_priority_fee_per_gas: None,
+			fee_cap: gas_price.unwrap_or_default(),
+		}),
+		// EIP-1559 transaction
+		(None, Some(max_fee), Some(max_priority)) => {
+			if max_priority > max_fee {
+				return Err(internal_err(
+					"Invalid input: `max_priority_fee_per_gas` greater than `max_fee_per_gas`",
+				));
 			}
 			Ok(FeeDetails {
-				gas_price: max_fee,
-				max_fee_per_gas: max_fee,
-				max_priority_fee_per_gas: max_priority,
+				gas_price: None,
+				max_fee_per_gas: Some(max_fee),
+				max_priority_fee_per_gas: Some(max_priority),
+				fee_cap: max_fee,
 			})
 		}
+		// Default to EIP-1559 transaction
+		_ => Ok(FeeDetails {
+			gas_price: None,
+			max_fee_per_gas: Some(U256::zero()),
+			max_priority_fee_per_gas: Some(U256::zero()),
+			fee_cap: U256::zero(),
+		}),
 	}
 }

frame/ethereum/src/lib.rs

@@ -365,7 +365,7 @@ impl<T: Config> Pallet<T> {
 		) {
 			weight_limit if weight_limit.proof_size() > 0 => (
 				Some(weight_limit),
-				Some(transaction_data.proof_size_base_cost.unwrap_or_default()),
+				Some(transaction_data.proof_size_base_cost()),
 			),
 			_ => (None, None),
 		}

frame/ethereum/src/tests/eip1559.rs

@@ -19,7 +19,7 @@
 
 use super::*;
 use evm::{ExitReason, ExitRevert, ExitSucceed};
-use fp_ethereum::ValidatedTransaction;
+use fp_ethereum::{TransactionData, ValidatedTransaction};
 use frame_support::{dispatch::DispatchClass, traits::Get, weights::Weight};
 use pallet_evm::{AddressMapping, GasWeightMapping};
 
@@ -585,3 +585,39 @@ fn proof_size_weight_limit_validation_works() {
 		);
 	});
 }
+
+#[test]
+fn proof_size_base_cost_should_keep_the_same_in_execution_and_estimate() {
+	let (pairs, mut ext) = new_test_ext(1);
+	let alice = &pairs[0];
+
+	ext.execute_with(|| {
+		let raw_tx = EIP1559UnsignedTransaction {
+			nonce: U256::zero(),
+			max_priority_fee_per_gas: U256::zero(),
+			max_fee_per_gas: U256::zero(),
+			gas_limit: U256::from(21_000),
+			action: ethereum::TransactionAction::Create,
+			value: U256::from(100),
+			input: vec![9; 100],
+		};
+
+		let tx_data: TransactionData = (&raw_tx.sign(&alice.private_key, Some(100))).into();
+		let estimate_tx_data = TransactionData::new(
+			raw_tx.action,
+			raw_tx.input,
+			raw_tx.nonce,
+			raw_tx.gas_limit,
+			None,
+			Some(raw_tx.max_fee_per_gas),
+			Some(raw_tx.max_priority_fee_per_gas),
+			raw_tx.value,
+			Some(100),
+			vec![],
+		);
+		assert_eq!(
+			estimate_tx_data.proof_size_base_cost(),
+			tx_data.proof_size_base_cost()
+		);
+	});
+}

frame/ethereum/src/tests/eip2930.rs

@@ -19,7 +19,7 @@
 
 use super::*;
 use evm::{ExitReason, ExitRevert, ExitSucceed};
-use fp_ethereum::ValidatedTransaction;
+use fp_ethereum::{TransactionData, ValidatedTransaction};
 use frame_support::{
 	dispatch::{DispatchClass, GetDispatchInfo},
 	weights::Weight,
@@ -511,3 +511,38 @@ fn proof_size_weight_limit_validation_works() {
 		);
 	});
 }
+
+#[test]
+fn proof_size_base_cost_should_keep_the_same_in_execution_and_estimate() {
+	let (pairs, mut ext) = new_test_ext(1);
+	let alice = &pairs[0];
+
+	ext.execute_with(|| {
+		let raw_tx = EIP2930UnsignedTransaction {
+			nonce: U256::zero(),
+			gas_price: U256::zero(),
+			gas_limit: U256::from(21_000),
+			action: ethereum::TransactionAction::Create,
+			value: U256::from(100),
+			input: vec![9; 100],
+		};
+
+		let tx_data: TransactionData = (&raw_tx.sign(&alice.private_key, Some(100))).into();
+		let estimate_tx_data = TransactionData::new(
+			raw_tx.action,
+			raw_tx.input,
+			raw_tx.nonce,
+			raw_tx.gas_limit,
+			Some(raw_tx.gas_price),
+			None,
+			None,
+			raw_tx.value,
+			Some(100),
+			vec![],
+		);
+		assert_eq!(
+			estimate_tx_data.proof_size_base_cost(),
+			tx_data.proof_size_base_cost()
+		);
+	});
+}

frame/ethereum/src/tests/legacy.rs

@@ -19,7 +19,7 @@
 
 use super::*;
 use evm::{ExitReason, ExitRevert, ExitSucceed};
-use fp_ethereum::ValidatedTransaction;
+use fp_ethereum::{TransactionData, ValidatedTransaction};
 use frame_support::{
 	dispatch::{DispatchClass, GetDispatchInfo},
 	weights::Weight,
@@ -511,3 +511,38 @@ fn proof_size_weight_limit_validation_works() {
 		);
 	});
 }
+
+#[test]
+fn proof_size_base_cost_should_keep_the_same_in_execution_and_estimate() {
+	let (pairs, mut ext) = new_test_ext(1);
+	let alice = &pairs[0];
+
+	ext.execute_with(|| {
+		let raw_tx = LegacyUnsignedTransaction {
+			nonce: U256::zero(),
+			gas_price: U256::zero(),
+			gas_limit: U256::from(21_000),
+			action: ethereum::TransactionAction::Create,
+			value: U256::from(100),
+			input: vec![9; 100],
+		};
+
+		let tx_data: TransactionData = (&raw_tx.sign(&alice.private_key)).into();
+		let estimate_tx_data = TransactionData::new(
+			raw_tx.action,
+			raw_tx.input,
+			raw_tx.nonce,
+			raw_tx.gas_limit,
+			Some(raw_tx.gas_price),
+			None,
+			None,
+			raw_tx.value,
+			Some(100),
+			vec![],
+		);
+		assert_eq!(
+			estimate_tx_data.proof_size_base_cost(),
+			tx_data.proof_size_base_cost()
+		);
+	});
+}

frame/evm/src/runner/stack.rs

@@ -193,35 +193,33 @@ where
 			});
 		}
 
-		let (total_fee_per_gas, _actual_priority_fee_per_gas) =
-			match (max_fee_per_gas, max_priority_fee_per_gas, is_transactional) {
+		let total_fee_per_gas = if is_transactional {
+			match (max_fee_per_gas, max_priority_fee_per_gas) {
 				// Zero max_fee_per_gas for validated transactional calls exist in XCM -> EVM
 				// because fees are already withdrawn in the xcm-executor.
-				(Some(max_fee), _, true) if max_fee.is_zero() => (U256::zero(), U256::zero()),
+				(Some(max_fee), _) if max_fee.is_zero() => U256::zero(),
 				// With no tip, we pay exactly the base_fee
-				(Some(_), None, _) => (base_fee, U256::zero()),
+				(Some(_), None) => base_fee,
 				// 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), _) => {
+				(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,
-					)
+
+					base_fee.saturating_add(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,
 					})
 				}
-			};
+			}
+		} else {
+			// Gas price check is skipped for non-transactional calls or creates
+			Default::default()
+		};
 
 		// After eip-1559 we make sure the account can pay both the evm execution and priority fees.
 		let total_fee =