diff --git a/client/rpc/src/eth/execute.rs b/client/rpc/src/eth/execute.rs index 1c1e37c737..cdcd909d4c 100644 --- a/client/rpc/src/eth/execute.rs +++ b/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, max_fee_per_gas: Option, max_priority_fee_per_gas: Option, + fee_cap: U256, } fn fee_details( request_gas_price: Option, - request_max_fee: Option, - request_priority: Option, + request_max_fee_per_gas: Option, + request_priority_fee_per_gas: Option, ) -> RpcResult { - 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(), + }), } } diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index e9aa870f11..4ce3303887 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -365,7 +365,7 @@ impl Pallet { ) { 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), } diff --git a/frame/ethereum/src/tests/eip1559.rs b/frame/ethereum/src/tests/eip1559.rs index 03edf8414b..073ceb79fd 100644 --- a/frame/ethereum/src/tests/eip1559.rs +++ b/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() + ); + }); +} diff --git a/frame/ethereum/src/tests/eip2930.rs b/frame/ethereum/src/tests/eip2930.rs index 772823a878..7eb800bda1 100644 --- a/frame/ethereum/src/tests/eip2930.rs +++ b/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() + ); + }); +} diff --git a/frame/ethereum/src/tests/legacy.rs b/frame/ethereum/src/tests/legacy.rs index 48532a33b0..4b630e78d3 100644 --- a/frame/ethereum/src/tests/legacy.rs +++ b/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() + ); + }); +} diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index aa0f7137c1..255e79383e 100644 --- a/frame/evm/src/runner/stack.rs +++ b/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::::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 = diff --git a/primitives/ethereum/src/lib.rs b/primitives/ethereum/src/lib.rs index 2aeee6cbff..765ace3000 100644 --- a/primitives/ethereum/src/lib.rs +++ b/primitives/ethereum/src/lib.rs @@ -47,7 +47,6 @@ pub struct TransactionData { pub value: U256, pub chain_id: Option, pub access_list: Vec<(H160, Vec)>, - pub proof_size_base_cost: Option, } impl TransactionData { @@ -64,7 +63,7 @@ impl TransactionData { chain_id: Option, access_list: Vec<(H160, Vec)>, ) -> Self { - let mut transaction_data = Self { + Self { action, input, nonce, @@ -75,20 +74,19 @@ impl TransactionData { value, chain_id, access_list, - proof_size_base_cost: None, - }; - let proof_size_base_cost = transaction_data - .encode() + } + } + + // The transact call wrapped in the extrinsic is part of the PoV, record this as a base cost for the size of the proof. + pub fn proof_size_base_cost(&self) -> u64 { + self.encode() .len() // signature .saturating_add(65) // pallet index .saturating_add(1) // call index - .saturating_add(1) as u64; - transaction_data.proof_size_base_cost = Some(proof_size_base_cost); - - transaction_data + .saturating_add(1) as u64 } } @@ -115,15 +113,6 @@ impl From for CheckEvmTransactionInput { impl From<&Transaction> for TransactionData { fn from(t: &Transaction) -> Self { - // The call wrapped in the extrinsic is part of the PoV, record this as a base cost for the size of the proof. - let proof_size_base_cost = t - .encode() - .len() - // pallet index - .saturating_add(1) - // call index - .saturating_add(1) as u64; - match t { Transaction::Legacy(t) => TransactionData { action: t.action, @@ -136,7 +125,6 @@ impl From<&Transaction> for TransactionData { value: t.value, chain_id: t.signature.chain_id(), access_list: Vec::new(), - proof_size_base_cost: Some(proof_size_base_cost), }, Transaction::EIP2930(t) => TransactionData { action: t.action, @@ -153,7 +141,6 @@ impl From<&Transaction> for TransactionData { .iter() .map(|d| (d.address, d.storage_keys.clone())) .collect(), - proof_size_base_cost: Some(proof_size_base_cost), }, Transaction::EIP1559(t) => TransactionData { action: t.action, @@ -170,7 +157,6 @@ impl From<&Transaction> for TransactionData { .iter() .map(|d| (d.address, d.storage_keys.clone())) .collect(), - proof_size_base_cost: Some(proof_size_base_cost), }, } }