diff --git a/.github/workflows/tests-evm.yml b/.github/workflows/tests-evm.yml index b193e060bfe56..8a3bc2f4e037d 100644 --- a/.github/workflows/tests-evm.yml +++ b/.github/workflows/tests-evm.yml @@ -99,7 +99,7 @@ jobs: uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 with: repository: paritytech/evm-test-suite - ref: c2422cace2fb8a4337fc1c704c49e458c8a79d6b + ref: 460b2c9aa3a3019d3508bb5a34a2498ea86035ff path: evm-test-suite - uses: denoland/setup-deno@v1 diff --git a/prdoc/pr_10148.prdoc b/prdoc/pr_10148.prdoc new file mode 100644 index 0000000000000..9c94e82ee29eb --- /dev/null +++ b/prdoc/pr_10148.prdoc @@ -0,0 +1,23 @@ +title: revive fix reported gas used +doc: +- audience: Runtime Dev + description: |- + Fix `gas_used` calculation introduced in #9418 to use the actual gas instead of just `ref_time`. + + With these changes we now guarantee that `tx_cost = effective_gas_price * gas`. + Note that since we compute gas as `fee / gas_price`, this can lead to rounding errors when the chain uses `SlowAdjustingFeeUpdate` (i.e. the fee is not a multiple of the gas price). + The changes in this PR ensure the fee still matches by burning the rounding remainder. + + This PR also fixes how the actual fee is computed and introduces a new `compute_actual_fee` in `Config::FeeInfo`. + The previous fee calculation was skipping the `extension_weight` in the fee calculation. + + The updated tests ensure that the tx cost reported in the receipt matches the fees deducted from the user account: + + https://github.com/paritytech/evm-test-suite/blob/460b2c9aa3a3019d3508bb5a34a2498ea86035ff/src/gas.test.ts?plain=1#L31-L61 +crates: +- name: pallet-revive + bump: patch +- name: revive-dev-runtime + bump: patch +- name: pallet-revive-eth-rpc + bump: patch diff --git a/substrate/frame/revive/rpc/src/receipt_extractor.rs b/substrate/frame/revive/rpc/src/receipt_extractor.rs index 3e8126096d106..7bece956c7f2e 100644 --- a/substrate/frame/revive/rpc/src/receipt_extractor.rs +++ b/substrate/frame/revive/rpc/src/receipt_extractor.rs @@ -17,7 +17,6 @@ use crate::{ client::{runtime_api::RuntimeApi, SubstrateBlock, SubstrateBlockNumber}, subxt_client::{ - self, revive::{ calls::types::EthTransact, events::{ContractEmitted, EthExtrinsicRevert}, @@ -36,10 +35,6 @@ use sp_core::keccak_256; use std::{future::Future, pin::Pin, sync::Arc}; use subxt::{blocks::ExtrinsicDetails, OnlineClient}; -type FetchGasPriceFn = Arc< - dyn Fn(H256) -> Pin> + Send>> + Send + Sync, ->; - type FetchReceiptDataFn = Arc< dyn Fn(H256) -> Pin>> + Send>> + Send + Sync, >; @@ -58,9 +53,6 @@ pub struct ReceiptExtractor { /// Fetch ethereum block hash. fetch_eth_block_hash: FetchEthBlockHashFn, - /// Fetch the gas price from the chain. - fetch_gas_price: FetchGasPriceFn, - /// Earliest block number to consider when searching for transaction receipts. earliest_receipt_block: Option, @@ -109,20 +101,6 @@ impl ReceiptExtractor { Box::pin(fut) as Pin> }); - let api_inner = api.clone(); - let fetch_gas_price = Arc::new(move |block_hash| { - let api_inner = api_inner.clone(); - - let fut = async move { - let runtime_api = api_inner.runtime_api().at(block_hash); - let payload = subxt_client::apis().revive_api().gas_price(); - let base_gas_price = runtime_api.call(payload).await?; - Ok(*base_gas_price) - }; - - Box::pin(fut) as Pin> - }); - let api_inner = api.clone(); let fetch_receipt_data = Arc::new(move |block_hash| { let api_inner = api_inner.clone(); @@ -138,7 +116,6 @@ impl ReceiptExtractor { Ok(Self { fetch_receipt_data, fetch_eth_block_hash, - fetch_gas_price, earliest_receipt_block, recover_eth_address: recover_eth_address_fn, }) @@ -154,13 +131,10 @@ impl ReceiptExtractor { let eth_block_hash = H256::from(keccak_256(&bytes)); Box::pin(std::future::ready(Some(eth_block_hash))) as Pin> }); - let fetch_gas_price = - Arc::new(|_| Box::pin(std::future::ready(Ok(U256::from(1000)))) as Pin>); Self { fetch_receipt_data, fetch_eth_block_hash, - fetch_gas_price, earliest_receipt_block: None, recover_eth_address: Arc::new(|signed_tx: &TransactionSigned| { signed_tx.recover_eth_address() @@ -197,11 +171,11 @@ impl ReceiptExtractor { ClientError::RecoverEthAddressFailed })?; - let base_gas_price = (self.fetch_gas_price)(substrate_block.hash()).await?; - let tx_info = - GenericTransaction::from_signed(signed_tx.clone(), base_gas_price, Some(from)); - - let gas_price = tx_info.gas_price.unwrap_or_default(); + let tx_info = GenericTransaction::from_signed( + signed_tx.clone(), + receipt_gas_info.effective_gas_price, + Some(from), + ); // get logs from ContractEmitted event let logs = events @@ -244,7 +218,7 @@ impl ReceiptExtractor { from, logs, tx_info.to, - gas_price, + receipt_gas_info.effective_gas_price, U256::from(receipt_gas_info.gas_used), success, transaction_hash, diff --git a/substrate/frame/revive/src/benchmarking.rs b/substrate/frame/revive/src/benchmarking.rs index e5b19c5976e4d..2ff6dbf7154c1 100644 --- a/substrate/frame/revive/src/benchmarking.rs +++ b/substrate/frame/revive/src/benchmarking.rs @@ -287,10 +287,13 @@ mod benchmarks { c: Linear<0, { 100 * 1024 }>, i: Linear<0, { limits::CALLDATA_BYTES }>, d: Linear<0, 1>, - ) { - let pallet_account = whitelisted_pallet_account::(); + ) -> Result<(), BenchmarkError> { let input = vec![42u8; i as usize]; + // Use an `effective_gas_price` that is not a multiple of `T::NativeToEthRatio` + // to hit the code that charge the rounding error so that tx_cost == effective_gas_price * + // gas_used + let effective_gas_price = Pallet::::evm_base_fee() + 1; let value = Pallet::::min_balance(); let dust = 42u32 * d; let evm_value = @@ -303,7 +306,6 @@ mod benchmarks { let deployer = T::AddressMapper::to_address(&caller); let nonce = System::::account_nonce(&caller).try_into().unwrap_or_default(); let addr = crate::address::create1(&deployer, nonce); - let account_id = T::AddressMapper::to_fallback_account_id(&addr); assert!(AccountInfoOf::::get(&deployer).is_none()); @@ -319,33 +321,13 @@ mod benchmarks { code, input, TransactionSigned::default().signed_payload(), - 0u32.into(), + effective_gas_price, 0, ); - let deposit = - T::Currency::balance_on_hold(&HoldReason::StorageDepositReserve.into(), &account_id); - // uploading the code reserves some balance in the pallet account - let code_deposit = T::Currency::balance_on_hold( - &HoldReason::CodeUploadDepositReserve.into(), - &pallet_account, - ); - let mapping_deposit = - T::Currency::balance_on_hold(&HoldReason::AddressMapping.into(), &caller); - - assert_eq!( - ::FeeInfo::remaining_txfee(), - caller_funding::() - deposit - code_deposit - Pallet::::min_balance(), - ); - assert_eq!( - Pallet::::evm_balance(&deployer), - Pallet::::convert_native_to_evm( - caller_funding::() - Pallet::::min_balance() - value - mapping_deposit, - ) - dust, - ); - // contract has the full value assert_eq!(Pallet::::evm_balance(&addr), evm_value); + Ok(()) } #[benchmark(pov_mode = Measured)] @@ -451,11 +433,14 @@ mod benchmarks { // `d`: with or without dust value to transfer #[benchmark(pov_mode = Measured)] fn eth_call(d: Linear<0, 1>) -> Result<(), BenchmarkError> { - let pallet_account = whitelisted_pallet_account::(); let data = vec![42u8; 1024]; let instance = Contract::::with_caller(whitelisted_caller(), VmBinaryModule::dummy(), vec![])?; + // Use an `effective_gas_price` that is not a multiple of `T::NativeToEthRatio` + // to hit the code that charge the rounding error so that tx_cost == effective_gas_price * + // gas_used + let effective_gas_price = Pallet::::evm_base_fee() + 1; let value = Pallet::::min_balance(); let dust = 42u32 * d; let evm_value = @@ -466,7 +451,6 @@ mod benchmarks { ::Currency::issue(caller_funding::()), ); - let caller_addr = T::AddressMapper::to_address(&instance.caller); let origin = Origin::EthTransaction(instance.caller.clone()); let before = Pallet::::evm_balance(&instance.address); @@ -478,30 +462,9 @@ mod benchmarks { Weight::MAX, data, TransactionSigned::default().signed_payload(), - 0u32.into(), + effective_gas_price, 0, ); - let deposit = T::Currency::balance_on_hold( - &HoldReason::StorageDepositReserve.into(), - &instance.account_id, - ); - let code_deposit = T::Currency::balance_on_hold( - &HoldReason::CodeUploadDepositReserve.into(), - &pallet_account, - ); - let mapping_deposit = - T::Currency::balance_on_hold(&HoldReason::AddressMapping.into(), &instance.caller); - // value and value transferred via call should be removed from the caller - assert_eq!( - Pallet::::evm_balance(&caller_addr), - Pallet::::convert_native_to_evm( - caller_funding::() - - Pallet::::min_balance() - - Pallet::::min_balance() - - value - deposit - code_deposit - - mapping_deposit, - ) - dust, - ); // contract should have received the value assert_eq!(Pallet::::evm_balance(&instance.address), before + evm_value); @@ -2741,7 +2704,10 @@ mod benchmarks { // Create input data of fixed size for consistent transaction payloads let input_data = vec![0x42u8; fixed_payload_size]; - let gas_used = Weight::from_parts(1_000_000, 1000); + let receipt_gas_info = ReceiptGasInfo { + gas_used: U256::from(1_000_000), + effective_gas_price: Pallet::::evm_base_fee(), + }; for _ in 0..n { // Create real signed transaction with fixed-size input data @@ -2763,7 +2729,7 @@ mod benchmarks { block_builder.process_transaction( signed_transaction, true, - gas_used, + receipt_gas_info.clone(), encoded_logs, bloom, ); @@ -2814,7 +2780,10 @@ mod benchmarks { // Create input data of variable size p for realistic transaction payloads let input_data = vec![0x42u8; d as usize]; - let gas_used = Weight::from_parts(1_000_000, 1000); + let receipt_gas_info = ReceiptGasInfo { + gas_used: U256::from(1_000_000), + effective_gas_price: Pallet::::evm_base_fee(), + }; for _ in 0..fixed_tx_count { // Create real signed transaction with variable-size input data @@ -2836,7 +2805,7 @@ mod benchmarks { block_builder.process_transaction( signed_transaction, true, - gas_used, + receipt_gas_info.clone(), encoded_logs, bloom, ); @@ -2888,7 +2857,10 @@ mod benchmarks { input_data.clone(), ); - let gas_used = Weight::from_parts(1_000_000, 1000); + let receipt_gas_info = ReceiptGasInfo { + gas_used: U256::from(1_000_000), + effective_gas_price: Pallet::::evm_base_fee(), + }; // Store transaction let _ = block_storage::bench_with_ethereum_context(|| { @@ -2900,7 +2872,7 @@ mod benchmarks { block_builder.process_transaction( signed_transaction, true, - gas_used, + receipt_gas_info.clone(), encoded_logs, bloom, ); @@ -2950,7 +2922,10 @@ mod benchmarks { input_data.clone(), ); - let gas_used = Weight::from_parts(1_000_000, 1000); + let receipt_gas_info = ReceiptGasInfo { + gas_used: U256::from(1_000_000), + effective_gas_price: Pallet::::evm_base_fee(), + }; // Store transaction let _ = block_storage::bench_with_ethereum_context(|| { @@ -2962,7 +2937,7 @@ mod benchmarks { block_builder.process_transaction( signed_transaction, true, - gas_used, + receipt_gas_info, encoded_logs, bloom, ); diff --git a/substrate/frame/revive/src/evm.rs b/substrate/frame/revive/src/evm.rs index b22322fdf3b55..5a2264c7169d3 100644 --- a/substrate/frame/revive/src/evm.rs +++ b/substrate/frame/revive/src/evm.rs @@ -35,4 +35,8 @@ pub use block_hash::ReceiptGasInfo; /// Ethereum block storage module. pub(crate) mod block_storage; +/// Transfer with dust functionality. +mod transfer_with_dust; +pub(crate) use transfer_with_dust::*; + type OnChargeTransactionBalanceOf = <::OnChargeTransaction as pallet_transaction_payment::OnChargeTransaction>::Balance; diff --git a/substrate/frame/revive/src/evm/block_hash.rs b/substrate/frame/revive/src/evm/block_hash.rs index 545fc08b6cc92..a713bbd19bab3 100644 --- a/substrate/frame/revive/src/evm/block_hash.rs +++ b/substrate/frame/revive/src/evm/block_hash.rs @@ -42,6 +42,9 @@ use sp_core::{H256, U256}; pub struct ReceiptGasInfo { /// The amount of gas used for this specific transaction alone. pub gas_used: U256, + + /// The effective gas price for this transaction. + pub effective_gas_price: U256, } impl Block { diff --git a/substrate/frame/revive/src/evm/block_hash/block_builder.rs b/substrate/frame/revive/src/evm/block_hash/block_builder.rs index 848429b69ce40..72369b3461d96 100644 --- a/substrate/frame/revive/src/evm/block_hash/block_builder.rs +++ b/substrate/frame/revive/src/evm/block_hash/block_builder.rs @@ -31,7 +31,7 @@ use crate::{ use alloc::{vec, vec::Vec}; use codec::{Decode, Encode}; -use frame_support::{weights::Weight, DefaultNoBound}; +use frame_support::DefaultNoBound; use scale_info::TypeInfo; use sp_core::{keccak_256, H160, H256, U256}; @@ -97,7 +97,7 @@ impl EthereumBlockBuilder { &mut self, transaction_encoded: Vec, success: bool, - gas_used: Weight, + receipt_gas_info: ReceiptGasInfo, encoded_logs: Vec, receipt_bloom: LogsBloom, ) { @@ -108,7 +108,7 @@ impl EthereumBlockBuilder { let transaction_type = Self::extract_transaction_type(transaction_encoded.as_slice()); // Update gas and logs bloom. - self.gas_used = self.gas_used.saturating_add(gas_used.ref_time().into()); + self.gas_used = self.gas_used.saturating_add(receipt_gas_info.gas_used); self.logs_bloom.accrue_bloom(&receipt_bloom); // Update the receipt trie. @@ -120,7 +120,7 @@ impl EthereumBlockBuilder { transaction_type, ); - self.gas_info.push(ReceiptGasInfo { gas_used: gas_used.ref_time().into() }); + self.gas_info.push(receipt_gas_info); // The first transaction and receipt are returned to be stored in the pallet storage. // The index of the incremental hash builders already expects the next items. @@ -394,7 +394,10 @@ mod test { tx_info.transaction_signed.signed_payload(), logs, receipt_info.status.unwrap_or_default() == 1.into(), - receipt_info.gas_used.as_u64(), + ReceiptGasInfo { + gas_used: receipt_info.gas_used, + effective_gas_price: receipt_info.effective_gas_price, + }, ) }) .collect(); @@ -402,7 +405,7 @@ mod test { ExtBuilder::default().build().execute_with(|| { // Build the ethereum block incrementally. let mut incremental_block = EthereumBlockBuilder::::default(); - for (signed, logs, success, gas_used) in transaction_details { + for (signed, logs, success, receipt_gas_info) in transaction_details { let mut log_size = 0; let mut accumulate_receipt = AccumulateReceipt::new(); @@ -415,7 +418,7 @@ mod test { incremental_block.process_transaction( signed, success, - gas_used.into(), + receipt_gas_info, accumulate_receipt.encoding, accumulate_receipt.bloom, ); diff --git a/substrate/frame/revive/src/evm/block_storage.rs b/substrate/frame/revive/src/evm/block_storage.rs index 04451f1e46da8..f7d9b037feba3 100644 --- a/substrate/frame/revive/src/evm/block_storage.rs +++ b/substrate/frame/revive/src/evm/block_storage.rs @@ -15,22 +15,28 @@ // See the License for the specific language governing permissions and // limitations under the License. use crate::{ - evm::block_hash::{AccumulateReceipt, EthereumBlockBuilder, LogsBloom}, + dispatch_result, + evm::{ + block_hash::{AccumulateReceipt, EthereumBlockBuilder, LogsBloom}, + burn_with_dust, + fees::InfoT, + }, limits, sp_runtime::traits::One, weights::WeightInfo, - BlockHash, Config, EthBlockBuilderIR, EthereumBlock, Event, Pallet, ReceiptInfoData, - UniqueSaturatedInto, H160, H256, + AccountIdOf, BalanceOf, BalanceWithDust, BlockHash, Config, ContractResult, Error, + EthBlockBuilderIR, EthereumBlock, Event, ExecReturnValue, Pallet, ReceiptGasInfo, + ReceiptInfoData, StorageDeposit, UniqueSaturatedInto, Weight, H160, H256, LOG_TARGET, }; use alloc::vec::Vec; use environmental::environmental; use frame_support::{ + dispatch::DispatchInfo, pallet_prelude::{DispatchError, DispatchResultWithPostInfo}, storage::with_transaction, - weights::Weight, }; use sp_core::U256; -use sp_runtime::TransactionOutcome; +use sp_runtime::{Saturating, TransactionOutcome}; /// The maximum number of block hashes to keep in the history. /// @@ -41,6 +47,76 @@ pub const BLOCK_HASH_COUNT: u32 = 256; // that are needed to construct the final transaction receipt. environmental!(receipt: AccumulateReceipt); +/// Result of an Ethereum context call execution. +pub(crate) struct EthereumCallResult { + /// Receipt gas information. + pub receipt_gas_info: ReceiptGasInfo, + /// The dispatch result with post-dispatch information. + pub result: DispatchResultWithPostInfo, +} + +impl EthereumCallResult { + /// Create a new `EthereumCallResult` from contract execution details. + /// + /// # Parameters + /// + /// - `signer`: The signer of the transaction + /// - `output`: The execution result + /// - `gas_consumed`: The weight consumed during execution + /// - `base_call_weight`: The base call weight + /// - `encoded_len`: The length of the encoded transaction in bytes + /// - `info`: Dispatch information used for fee computation + /// - `effective_gas_price`: The EVM gas price + pub(crate) fn new( + signer: AccountIdOf, + mut output: ContractResult>, + base_call_weight: Weight, + encoded_len: u32, + info: &DispatchInfo, + effective_gas_price: U256, + ) -> Self { + let effective_gas_price = effective_gas_price.max(Pallet::::evm_base_fee()); + + if let Ok(retval) = &output.result { + if retval.did_revert() { + output.result = Err(>::ContractReverted.into()); + } + } + + // Refund pre-charged revert event weight if the call succeeds. + if output.result.is_ok() { + output + .gas_consumed + .saturating_reduce(T::WeightInfo::deposit_eth_extrinsic_revert_event()) + } + + let result = dispatch_result(output.result, output.gas_consumed, base_call_weight); + let native_fee = T::FeeInfo::compute_actual_fee(encoded_len, &info, &result); + let result = T::FeeInfo::ensure_not_overdrawn(native_fee, result); + + let fee = Pallet::::convert_native_to_evm(match output.storage_deposit { + StorageDeposit::Refund(refund) => native_fee.saturating_sub(refund), + StorageDeposit::Charge(amount) => native_fee.saturating_add(amount), + }); + + let (mut gas_used, rest) = fee.div_mod(effective_gas_price); + if !rest.is_zero() { + gas_used = gas_used.saturating_add(1_u32.into()); + } + + let tx_cost = gas_used.saturating_mul(effective_gas_price); + if tx_cost > fee { + let round_up_fee = BalanceWithDust::>::from_value::(tx_cost - fee) + .expect("value fits into BalanceOf; qed"); + log::debug!(target: LOG_TARGET, "Collecting round_up fee from {signer:?}: {round_up_fee:?}"); + let _ = burn_with_dust::(&signer, round_up_fee) + .inspect_err(|e| log::debug!(target: LOG_TARGET, "Failed to collect round up fee {round_up_fee:?} from {signer:?}: {e:?}")); + } + + Self { receipt_gas_info: ReceiptGasInfo { gas_used, effective_gas_price }, result } + } +} + /// Capture the Ethereum log for the current transaction. /// /// This method does nothing if called from outside of the ethereum context. @@ -72,21 +148,21 @@ pub fn bench_with_ethereum_context(f: impl FnOnce() -> R) -> R { /// /// # Parameters /// - transaction_encoded: The RLP encoded transaction bytes. -/// - call: A closure that executes the transaction logic and returns the gas consumed and result. +/// - call: A closure that executes the transaction logic and returns an `EthereumCallResult`. pub fn with_ethereum_context( transaction_encoded: Vec, - call: impl FnOnce() -> (Weight, DispatchResultWithPostInfo), + call: impl FnOnce() -> EthereumCallResult, ) -> DispatchResultWithPostInfo { receipt::using(&mut AccumulateReceipt::new(), || { - let (err, gas_consumed, mut post_info) = + let (err, receipt_gas_info, post_info) = with_transaction(|| -> TransactionOutcome> { - let (gas_consumed, result) = call(); + let EthereumCallResult { receipt_gas_info, result } = call(); match result { Ok(post_info) => - TransactionOutcome::Commit(Ok((None, gas_consumed, post_info))), + TransactionOutcome::Commit(Ok((None, receipt_gas_info, post_info))), Err(err) => TransactionOutcome::Rollback(Ok(( Some(err.error), - gas_consumed, + receipt_gas_info, err.post_info, ))), } @@ -97,7 +173,7 @@ pub fn with_ethereum_context( crate::block_storage::process_transaction::( transaction_encoded, false, - gas_consumed, + receipt_gas_info, ); Ok(post_info) } else { @@ -105,11 +181,11 @@ pub fn with_ethereum_context( #[cfg(feature = "runtime-benchmarks")] deposit_eth_extrinsic_revert_event::(crate::Error::::BenchmarkingError.into()); - crate::block_storage::process_transaction::(transaction_encoded, true, gas_consumed); - post_info - .actual_weight - .as_mut() - .map(|w| w.saturating_reduce(T::WeightInfo::deposit_eth_extrinsic_revert_event())); + crate::block_storage::process_transaction::( + transaction_encoded, + true, + receipt_gas_info, + ); Ok(post_info) } }) @@ -177,7 +253,7 @@ pub fn on_finalize_build_eth_block( pub fn process_transaction( transaction_encoded: Vec, success: bool, - gas_used: Weight, + receipt_gas_info: ReceiptGasInfo, ) { // Method returns `None` only when called from outside of the ethereum context. // This is not the case here, since this is called from within the @@ -187,7 +263,13 @@ pub fn process_transaction( let block_builder_ir = EthBlockBuilderIR::::get(); let mut block_builder = EthereumBlockBuilder::::from_ir(block_builder_ir); - block_builder.process_transaction(transaction_encoded, success, gas_used, encoded_logs, bloom); + block_builder.process_transaction( + transaction_encoded, + success, + receipt_gas_info, + encoded_logs, + bloom, + ); EthBlockBuilderIR::::put(block_builder.to_ir()); } diff --git a/substrate/frame/revive/src/evm/call.rs b/substrate/frame/revive/src/evm/call.rs index b35301455710d..fd3e8cced8147 100644 --- a/substrate/frame/revive/src/evm/call.rs +++ b/substrate/frame/revive/src/evm/call.rs @@ -170,10 +170,10 @@ where .saturating_mul_int(>::saturated_from(adjusted)); unadjusted }; - - let weight_limit = ::FeeInfo::fee_to_weight(remaining_fee) + let remaining_fee_weight = ::FeeInfo::fee_to_weight(remaining_fee); + let weight_limit = remaining_fee_weight .checked_sub(&info.total_weight()).ok_or_else(|| { - log::debug!(target: LOG_TARGET, "Not enough gas supplied to cover the weight of the extrinsic."); + log::debug!(target: LOG_TARGET, "Not enough gas supplied to cover the weight ({:?}) of the extrinsic. remaining_fee_weight: {remaining_fee_weight:?}", info.total_weight(),); InvalidTransaction::Payment })?; diff --git a/substrate/frame/revive/src/evm/fees.rs b/substrate/frame/revive/src/evm/fees.rs index 8e3724fe09693..019c96dd63174 100644 --- a/substrate/frame/revive/src/evm/fees.rs +++ b/substrate/frame/revive/src/evm/fees.rs @@ -40,7 +40,9 @@ use pallet_transaction_payment::{ }; use sp_runtime::{ generic::UncheckedExtrinsic, - traits::{Block as BlockT, Dispatchable, TransactionExtension}, + traits::{ + Block as BlockT, Dispatchable, ExtensionPostDispatchWeightHandler, TransactionExtension, + }, FixedPointNumber, FixedU128, SaturatedConversion, Saturating, }; @@ -107,8 +109,7 @@ pub trait InfoT: seal::Sealed { /// Makes sure that not too much storage deposit was withdrawn. fn ensure_not_overdrawn( - _encoded_len: u32, - _info: &DispatchInfo, + _fee: BalanceOf, result: DispatchResultWithPostInfo, ) -> DispatchResultWithPostInfo { result @@ -156,6 +157,15 @@ pub trait InfoT: seal::Sealed { fn remaining_txfee() -> BalanceOf { Default::default() } + + /// Compute the actual post_dispatch fee + fn compute_actual_fee( + _encoded_len: u32, + _info: &DispatchInfo, + _result: &DispatchResultWithPostInfo, + ) -> BalanceOf { + Default::default() + } } /// Which function to use in order to combine `ref_time` and `proof_size` to a fee. @@ -260,8 +270,7 @@ where } fn ensure_not_overdrawn( - encoded_len: u32, - info: &DispatchInfo, + fee: BalanceOf, result: DispatchResultWithPostInfo, ) -> DispatchResultWithPostInfo { // if tx is already failing we can ignore @@ -270,9 +279,6 @@ where return result; }; - let fee: BalanceOf = - >::compute_actual_fee(encoded_len, info, &post_info, Zero::zero()) - .into(); let available = Self::remaining_txfee(); if fee > available { log::debug!(target: LOG_TARGET, "Drew too much from the txhold. \ @@ -348,6 +354,21 @@ where fn remaining_txfee() -> BalanceOf { TxPallet::::remaining_txfee() } + + fn compute_actual_fee( + encoded_len: u32, + info: &DispatchInfo, + result: &DispatchResultWithPostInfo, + ) -> BalanceOf { + let mut post_info = *match result { + Ok(post_info) => post_info, + Err(err) => &err.post_info, + }; + + post_info.set_extension_weight(info); + >::compute_actual_fee(encoded_len, info, &post_info, Zero::zero()) + .into() + } } impl InfoT for () {} diff --git a/substrate/frame/revive/src/evm/runtime.rs b/substrate/frame/revive/src/evm/runtime.rs index 5674088de4efc..c7edc432b051b 100644 --- a/substrate/frame/revive/src/evm/runtime.rs +++ b/substrate/frame/revive/src/evm/runtime.rs @@ -374,7 +374,7 @@ mod test { }; use frame_support::{error::LookupError, traits::fungible::Mutate}; use pallet_revive_fixtures::compile_module; - use sp_runtime::traits::{self, Checkable, DispatchTransaction, Get}; + use sp_runtime::traits::{self, Checkable, DispatchTransaction}; type AccountIdOf = ::AccountId; @@ -529,7 +529,8 @@ mod test { let builder = UncheckedExtrinsicBuilder::call_with(H160::from([1u8; 20])); let (expected_encoded_len, call, _, tx, gas_required, signed_transaction) = builder.check().unwrap(); - let expected_effective_gas_price: u32 = ::NativeToEthRatio::get(); + let expected_effective_gas_price = + ExtBuilder::default().build().execute_with(|| Pallet::::evm_base_fee()); match call { RuntimeCall::Contracts(crate::Call::eth_call:: { @@ -544,7 +545,7 @@ mod test { value == tx.value.unwrap_or_default().as_u64().into() && data == tx.input.to_vec() && transaction_encoded == signed_transaction.signed_payload() && - effective_gas_price == expected_effective_gas_price.into() => + effective_gas_price == expected_effective_gas_price => { assert_eq!(encoded_len, expected_encoded_len); assert!( @@ -566,7 +567,8 @@ mod test { ); let (expected_encoded_len, call, _, tx, gas_required, signed_transaction) = builder.check().unwrap(); - let expected_effective_gas_price: u32 = ::NativeToEthRatio::get(); + let expected_effective_gas_price = + ExtBuilder::default().build().execute_with(|| Pallet::::evm_base_fee()); let expected_value = tx.value.unwrap_or_default().as_u64().into(); match call { @@ -582,7 +584,7 @@ mod test { code == expected_code && data == expected_data && transaction_encoded == signed_transaction.signed_payload() && - effective_gas_price == expected_effective_gas_price.into() => + effective_gas_price == expected_effective_gas_price => { assert_eq!(encoded_len, expected_encoded_len); assert!( diff --git a/substrate/frame/revive/src/evm/transfer_with_dust.rs b/substrate/frame/revive/src/evm/transfer_with_dust.rs new file mode 100644 index 0000000000000..dbc48b41f091c --- /dev/null +++ b/substrate/frame/revive/src/evm/transfer_with_dust.rs @@ -0,0 +1,388 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Transfer with dust functionality for pallet-revive. + +use crate::{ + address::AddressMapper, exec::AccountIdOf, primitives::BalanceWithDust, storage::AccountInfo, + AccountInfoOf, BalanceOf, Config, Error, LOG_TARGET, +}; +use frame_support::{ + dispatch::DispatchResult, + traits::{ + fungible::Mutate, + tokens::{Fortitude, Precision, Preservation}, + Get, + }, +}; + +/// Transfer balance between two accounts. +fn transfer_balance( + from: &AccountIdOf, + to: &AccountIdOf, + value: BalanceOf, +) -> DispatchResult { + T::Currency::transfer(from, to, value, Preservation::Preserve) + .map_err(|err| { + log::debug!(target: LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}"); + Error::::TransferFailed + })?; + Ok(()) +} + +/// Transfer dust between two account infos. +fn transfer_dust( + from: &mut AccountInfo, + to: &mut AccountInfo, + dust: u32, +) -> DispatchResult { + from.dust = from.dust.checked_sub(dust).ok_or_else(|| Error::::TransferFailed)?; + to.dust = to.dust.checked_add(dust).ok_or_else(|| Error::::TransferFailed)?; + Ok(()) +} + +/// Ensure an account has sufficient dust to perform an operation. +/// +/// If the account doesn't have enough dust, this function will burn one unit of the native +/// currency (1 plank) and convert it to dust by adding `NativeToEthRatio` worth of dust +/// to the account's dust balance. +fn ensure_sufficient_dust( + from: &AccountIdOf, + from_info: &mut AccountInfo, + required_dust: u32, +) -> DispatchResult { + if from_info.dust >= required_dust { + return Ok(()) + } + + let plank = T::NativeToEthRatio::get(); + + T::Currency::burn_from( + from, + 1u32.into(), + Preservation::Preserve, + Precision::Exact, + Fortitude::Polite, + ) + .map_err(|err| { + log::debug!(target: LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}"); + Error::::TransferFailed + })?; + + from_info.dust = from_info.dust.checked_add(plank).ok_or_else(|| Error::::TransferFailed)?; + + Ok(()) +} + +/// Transfer a balance with dust between two accounts. +pub(crate) fn transfer_with_dust( + from: &AccountIdOf, + to: &AccountIdOf, + value: BalanceWithDust>, +) -> DispatchResult { + let from_addr = >::to_address(from); + let mut from_info = AccountInfoOf::::get(&from_addr).unwrap_or_default(); + + if from_info.balance(from) < value { + log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} to {to:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from)); + return Err(Error::::TransferFailed.into()) + } else if from == to || value.is_zero() { + return Ok(()) + } + + let (value, dust) = value.deconstruct(); + if dust == 0 { + return transfer_balance::(from, to, value) + } + + let to_addr = >::to_address(to); + let mut to_info = AccountInfoOf::::get(&to_addr).unwrap_or_default(); + + ensure_sufficient_dust::(from, &mut from_info, dust)?; + transfer_balance::(from, to, value)?; + transfer_dust::(&mut from_info, &mut to_info, dust)?; + + let plank = T::NativeToEthRatio::get(); + if to_info.dust >= plank { + T::Currency::mint_into(to, 1u32.into())?; + to_info.dust = to_info.dust.checked_sub(plank).ok_or_else(|| Error::::TransferFailed)?; + } + + AccountInfoOf::::set(&from_addr, Some(from_info)); + AccountInfoOf::::set(&to_addr, Some(to_info)); + + Ok(()) +} + +/// Burn a balance with dust from an account. +pub(crate) fn burn_with_dust( + from: &AccountIdOf, + value: BalanceWithDust>, +) -> DispatchResult { + let from_addr = >::to_address(from); + let mut from_info = AccountInfoOf::::get(&from_addr).unwrap_or_default(); + + if from_info.balance(from) < value { + log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from)); + return Err(Error::::TransferFailed.into()) + } else if value.is_zero() { + return Ok(()) + } + + let (value, dust) = value.deconstruct(); + if dust == 0 { + // No dust to handle, just burn the balance + T::Currency::burn_from( + from, + value, + Preservation::Preserve, + Precision::Exact, + Fortitude::Polite, + ) + .map_err(|err| { + log::debug!(target: LOG_TARGET, "Burning {value:?} from {from:?} failed. Err: {err:?}"); + Error::::TransferFailed + })?; + return Ok(()) + } + + ensure_sufficient_dust::(from, &mut from_info, dust)?; + T::Currency::burn_from( + from, + value, + Preservation::Preserve, + Precision::Exact, + Fortitude::Polite, + ) + .map_err(|err| { + log::debug!(target: LOG_TARGET, "Burning {value:?} from {from:?} failed. Err: {err:?}"); + Error::::TransferFailed + })?; + + from_info.dust = from_info.dust.checked_sub(dust).ok_or_else(|| Error::::TransferFailed)?; + AccountInfoOf::::set(&from_addr, Some(from_info)); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + test_utils::{ALICE_ADDR, BOB_ADDR}, + tests::{builder, test_utils::set_balance_with_dust, ExtBuilder, Test}, + Config, Error, Pallet, H160, + }; + use frame_support::{assert_err, traits::Get}; + use sp_runtime::{traits::Zero, DispatchError}; + + #[test] + fn transfer_with_dust_works() { + struct TestCase { + description: &'static str, + from: H160, + to: H160, + from_balance: BalanceWithDust, + to_balance: BalanceWithDust, + amount: BalanceWithDust, + expected_from_balance: BalanceWithDust, + expected_to_balance: BalanceWithDust, + total_issuance_diff: i64, + expected_error: Option, + } + + let plank: u32 = ::NativeToEthRatio::get(); + + let test_cases = vec![ + TestCase { + description: "without dust", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(100, 0), + to_balance: BalanceWithDust::new_unchecked::(0, 0), + amount: BalanceWithDust::new_unchecked::(1, 0), + expected_from_balance: BalanceWithDust::new_unchecked::(99, 0), + expected_to_balance: BalanceWithDust::new_unchecked::(1, 0), + total_issuance_diff: 0, + expected_error: None, + }, + TestCase { + description: "with dust", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(100, 0), + to_balance: BalanceWithDust::new_unchecked::(0, 0), + amount: BalanceWithDust::new_unchecked::(1, 10), + expected_from_balance: BalanceWithDust::new_unchecked::(98, plank - 10), + expected_to_balance: BalanceWithDust::new_unchecked::(1, 10), + total_issuance_diff: 1, + expected_error: None, + }, + TestCase { + description: "just dust", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(100, 0), + to_balance: BalanceWithDust::new_unchecked::(0, 0), + amount: BalanceWithDust::new_unchecked::(0, 10), + expected_from_balance: BalanceWithDust::new_unchecked::(99, plank - 10), + expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), + total_issuance_diff: 1, + expected_error: None, + }, + TestCase { + description: "with existing dust", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(100, 5), + to_balance: BalanceWithDust::new_unchecked::(0, plank - 5), + amount: BalanceWithDust::new_unchecked::(1, 10), + expected_from_balance: BalanceWithDust::new_unchecked::(98, plank - 5), + expected_to_balance: BalanceWithDust::new_unchecked::(2, 5), + total_issuance_diff: 0, + expected_error: None, + }, + TestCase { + description: "with enough existing dust", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(100, 10), + to_balance: BalanceWithDust::new_unchecked::(0, plank - 10), + amount: BalanceWithDust::new_unchecked::(1, 10), + expected_from_balance: BalanceWithDust::new_unchecked::(99, 0), + expected_to_balance: BalanceWithDust::new_unchecked::(2, 0), + total_issuance_diff: -1, + expected_error: None, + }, + TestCase { + description: "receiver dust less than 1 plank", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(100, plank / 10), + to_balance: BalanceWithDust::new_unchecked::(0, plank / 2), + amount: BalanceWithDust::new_unchecked::(1, plank / 10 * 3), + expected_from_balance: BalanceWithDust::new_unchecked::(98, plank / 10 * 8), + expected_to_balance: BalanceWithDust::new_unchecked::(1, plank / 10 * 8), + total_issuance_diff: 1, + expected_error: None, + }, + TestCase { + description: "insufficient balance", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(10, 0), + to_balance: BalanceWithDust::new_unchecked::(10, 0), + amount: BalanceWithDust::new_unchecked::(20, 0), + expected_from_balance: BalanceWithDust::new_unchecked::(10, 0), + expected_to_balance: BalanceWithDust::new_unchecked::(10, 0), + total_issuance_diff: 0, + expected_error: Some(Error::::TransferFailed.into()), + }, + TestCase { + description: "from = to with insufficient balance", + from: ALICE_ADDR, + to: ALICE_ADDR, + from_balance: BalanceWithDust::new_unchecked::(10, 0), + to_balance: BalanceWithDust::new_unchecked::(10, 0), + amount: BalanceWithDust::new_unchecked::(20, 0), + expected_from_balance: BalanceWithDust::new_unchecked::(10, 0), + expected_to_balance: BalanceWithDust::new_unchecked::(10, 0), + total_issuance_diff: 0, + expected_error: Some(Error::::TransferFailed.into()), + }, + TestCase { + description: "from = to with insufficient balance", + from: ALICE_ADDR, + to: ALICE_ADDR, + from_balance: BalanceWithDust::new_unchecked::(0, 10), + to_balance: BalanceWithDust::new_unchecked::(0, 10), + amount: BalanceWithDust::new_unchecked::(0, 20), + expected_from_balance: BalanceWithDust::new_unchecked::(0, 10), + expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), + total_issuance_diff: 0, + expected_error: Some(Error::::TransferFailed.into()), + }, + TestCase { + description: "from = to", + from: ALICE_ADDR, + to: ALICE_ADDR, + from_balance: BalanceWithDust::new_unchecked::(0, 10), + to_balance: BalanceWithDust::new_unchecked::(0, 10), + amount: BalanceWithDust::new_unchecked::(0, 5), + expected_from_balance: BalanceWithDust::new_unchecked::(0, 10), + expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), + total_issuance_diff: 0, + expected_error: None, + }, + ]; + + for TestCase { + description, + from, + to, + from_balance, + to_balance, + amount, + expected_from_balance, + expected_to_balance, + total_issuance_diff, + expected_error, + } in test_cases.into_iter() + { + ExtBuilder::default().build().execute_with(|| { + set_balance_with_dust(&from, from_balance); + set_balance_with_dust(&to, to_balance); + + let total_issuance = ::Currency::total_issuance(); + let evm_value = Pallet::::convert_native_to_evm(amount); + + let (value, dust) = amount.deconstruct(); + assert_eq!(Pallet::::has_dust(evm_value), !dust.is_zero()); + assert_eq!(Pallet::::has_balance(evm_value), !value.is_zero()); + + let result = builder::bare_call(to).evm_value(evm_value).build(); + + if let Some(expected_error) = expected_error { + assert_err!(result.result, expected_error); + } else { + assert_eq!( + result.result.unwrap(), + Default::default(), + "{description} tx failed" + ); + } + + assert_eq!( + Pallet::::evm_balance(&from), + Pallet::::convert_native_to_evm(expected_from_balance), + "{description}: invalid from balance" + ); + + assert_eq!( + Pallet::::evm_balance(&to), + Pallet::::convert_native_to_evm(expected_to_balance), + "{description}: invalid to balance" + ); + + assert_eq!( + total_issuance as i64 - total_issuance_diff, + ::Currency::total_issuance() as i64, + "{description}: total issuance should match" + ); + }); + } + } +} diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 06b13eb49567c..4115cd5c6895b 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -20,6 +20,7 @@ use crate::{ evm::{ block_storage, fees::{Combinator, InfoT}, + transfer_with_dust, }, gas::GasMeter, limits, @@ -29,9 +30,9 @@ use crate::{ storage::{self, meter::Diff, AccountIdOrAddress, WriteOutcome}, tracing::if_tracing, transient_storage::TransientStorage, - AccountInfo, AccountInfoOf, BalanceOf, BalanceWithDust, Code, CodeInfo, CodeInfoOf, - CodeRemoved, Config, ContractInfo, Error, Event, ImmutableData, ImmutableDataOf, - Pallet as Contracts, RuntimeCosts, LOG_TARGET, + AccountInfo, BalanceOf, BalanceWithDust, Code, CodeInfo, CodeInfoOf, CodeRemoved, Config, + ContractInfo, Error, Event, ImmutableData, ImmutableDataOf, Pallet as Contracts, RuntimeCosts, + LOG_TARGET, }; use alloc::{ collections::{BTreeMap, BTreeSet}, @@ -44,7 +45,6 @@ use frame_support::{ storage::{with_transaction, TransactionOutcome}, traits::{ fungible::{Inspect, Mutate}, - tokens::{Fortitude, Precision, Preservation}, Time, }, weights::Weight, @@ -61,7 +61,7 @@ use sp_core::{ }; use sp_io::{crypto::secp256k1_ecdsa_recover_compressed, hashing::blake2_256}; use sp_runtime::{ - traits::{BadOrigin, Bounded, Saturating, TrailingZeroInput, Zero}, + traits::{BadOrigin, Bounded, Saturating, TrailingZeroInput}, DispatchError, SaturatedConversion, }; @@ -1521,86 +1521,6 @@ where storage_meter: &mut storage::meter::GenericMeter, exec_config: &ExecConfig, ) -> DispatchResult { - fn transfer_with_dust( - from: &AccountIdOf, - to: &AccountIdOf, - value: BalanceWithDust>, - ) -> DispatchResult { - fn transfer_balance( - from: &AccountIdOf, - to: &AccountIdOf, - value: BalanceOf, - ) -> DispatchResult { - T::Currency::transfer(from, to, value, Preservation::Preserve) - .map_err(|err| { - log::debug!(target: LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}"); - Error::::TransferFailed - })?; - Ok(()) - } - - fn transfer_dust( - from: &mut AccountInfo, - to: &mut AccountInfo, - dust: u32, - ) -> DispatchResult { - from.dust = - from.dust.checked_sub(dust).ok_or_else(|| Error::::TransferFailed)?; - to.dust = to.dust.checked_add(dust).ok_or_else(|| Error::::TransferFailed)?; - Ok(()) - } - - let from_addr = >::to_address(from); - let mut from_info = AccountInfoOf::::get(&from_addr).unwrap_or_default(); - - if from_info.balance(from) < value { - log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} to {to:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from)); - return Err(Error::::TransferFailed.into()) - } else if from == to || value.is_zero() { - return Ok(()) - } - - let (value, dust) = value.deconstruct(); - if dust.is_zero() { - return transfer_balance::(from, to, value) - } - - let to_addr = >::to_address(to); - let mut to_info = AccountInfoOf::::get(&to_addr).unwrap_or_default(); - - let plank = T::NativeToEthRatio::get(); - - if from_info.dust < dust { - T::Currency::burn_from( - from, - 1u32.into(), - Preservation::Preserve, - Precision::Exact, - Fortitude::Polite, - ) - .map_err(|err| { - log::debug!(target: LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}"); - Error::::TransferFailed - })?; - - from_info.dust = - from_info.dust.checked_add(plank).ok_or_else(|| Error::::TransferFailed)?; - } - - transfer_balance::(from, to, value)?; - transfer_dust::(&mut from_info, &mut to_info, dust)?; - - if to_info.dust >= plank { - T::Currency::mint_into(to, 1u32.into())?; - to_info.dust = - to_info.dust.checked_sub(plank).ok_or_else(|| Error::::TransferFailed)?; - } - - AccountInfoOf::::set(&from_addr, Some(from_info)); - AccountInfoOf::::set(&to_addr, Some(to_info)); - - Ok(()) - } let value = BalanceWithDust::>::from_value::(value) .map_err(|_| Error::::BalanceConversionFailed)?; if value.is_zero() { diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index 9b483e922fa5e..420f93332ac6d 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -2839,12 +2839,7 @@ fn block_hash_returns_proper_values() { let bob_code_hash = MockLoader::insert(Call, |ctx, _| { ctx.ext.block_number = 1u32.into(); assert_eq!(ctx.ext.block_hash(U256::from(1)), None); - assert_eq!( - ctx.ext.block_hash(U256::from(0)), - Some(H256::from(hex_literal::hex!( - "c2fed5de763ecfa4cb65e8f57838c5517f69db4ede78a30093bf3fad66c5a8cd" - ))) - ); + assert!(ctx.ext.block_hash(U256::from(0)).is_some()); ctx.ext.block_number = 300u32.into(); assert_eq!(ctx.ext.block_hash(U256::from(300)), None); diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 8383e71730b0b..2e3fee1a89b83 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -1224,7 +1224,8 @@ pub mod pallet { effective_gas_price: U256, encoded_len: u32, ) -> DispatchResultWithPostInfo { - let origin = Self::ensure_eth_origin(origin)?; + let signer = Self::ensure_eth_signed(origin)?; + let origin = OriginFor::::signed(signer.clone()); Self::ensure_non_contract_if_signed(&origin)?; let mut call = Call::::eth_instantiate_with_code { value, @@ -1241,7 +1242,7 @@ pub mod pallet { drop(call); block_storage::with_ethereum_context::(transaction_encoded, || { - let mut output = Self::bare_instantiate( + let output = Self::bare_instantiate( origin, value, gas_limit, @@ -1256,19 +1257,14 @@ pub mod pallet { ), ); - if let Ok(retval) = &output.result { - if retval.result.did_revert() { - output.result = Err(>::ContractReverted.into()); - } - } - - let result = dispatch_result( - output.result.map(|result| result.result), - output.gas_consumed, + block_storage::EthereumCallResult::new::( + signer, + output.map_result(|r| r.result), base_info.call_weight, - ); - let result = T::FeeInfo::ensure_not_overdrawn(encoded_len, &info, result); - (output.gas_consumed, result) + encoded_len, + &info, + effective_gas_price, + ) }) } @@ -1290,7 +1286,9 @@ pub mod pallet { effective_gas_price: U256, encoded_len: u32, ) -> DispatchResultWithPostInfo { - let origin = Self::ensure_eth_origin(origin)?; + let signer = Self::ensure_eth_signed(origin)?; + let origin = OriginFor::::signed(signer.clone()); + Self::ensure_non_contract_if_signed(&origin)?; let mut call = Call::::eth_call { dest, @@ -1307,7 +1305,7 @@ pub mod pallet { drop(call); block_storage::with_ethereum_context::(transaction_encoded, || { - let mut output = Self::bare_call( + let output = Self::bare_call( origin, dest, value, @@ -1321,16 +1319,14 @@ pub mod pallet { ), ); - if let Ok(return_value) = &output.result { - if return_value.did_revert() { - output.result = Err(>::ContractReverted.into()); - } - } - - let result = - dispatch_result(output.result, output.gas_consumed, base_info.call_weight); - let result = T::FeeInfo::ensure_not_overdrawn(encoded_len, &info, result); - (output.gas_consumed, result) + block_storage::EthereumCallResult::new::( + signer, + output, + base_info.call_weight, + encoded_len, + &info, + effective_gas_price, + ) }) } @@ -2293,10 +2289,10 @@ impl Pallet { >::deposit_event(::RuntimeEvent::from(event)) } - /// Tranform a [`Origin::EthTransaction`] into a signed origin. - fn ensure_eth_origin(origin: OriginFor) -> Result, DispatchError> { + // Returns Ok with the account that signed the eth transaction. + fn ensure_eth_signed(origin: OriginFor) -> Result, DispatchError> { match ::RuntimeOrigin::from(origin).into() { - Ok(Origin::EthTransaction(signer)) => Ok(OriginFor::::signed(signer)), + Ok(Origin::EthTransaction(signer)) => Ok(signer), _ => Err(BadOrigin.into()), } } diff --git a/substrate/frame/revive/src/primitives.rs b/substrate/frame/revive/src/primitives.rs index 1b4a92e64b2f5..d1ab4203ede0b 100644 --- a/substrate/frame/revive/src/primitives.rs +++ b/substrate/frame/revive/src/primitives.rs @@ -235,6 +235,17 @@ pub enum StorageDeposit { Charge(Balance), } +impl ContractResult { + pub fn map_result(self, map_fn: impl FnOnce(T) -> V) -> ContractResult { + ContractResult { + gas_consumed: self.gas_consumed, + gas_required: self.gas_required, + storage_deposit: self.storage_deposit, + result: self.result.map(map_fn), + } + } +} + impl Default for StorageDeposit { fn default() -> Self { Self::Charge(Zero::zero()) diff --git a/substrate/frame/revive/src/tests/block_hash.rs b/substrate/frame/revive/src/tests/block_hash.rs index 2fafa4cac7fb8..68685481c6b5e 100644 --- a/substrate/frame/revive/src/tests/block_hash.rs +++ b/substrate/frame/revive/src/tests/block_hash.rs @@ -35,7 +35,8 @@ use pallet_revive_fixtures::compile_module; #[test] fn on_initialize_clears_storage() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let receipt_data = vec![ReceiptGasInfo { gas_used: 1.into() }]; + let receipt_data = + vec![ReceiptGasInfo { gas_used: 1.into(), effective_gas_price: 1.into() }]; ReceiptInfoData::::put(receipt_data.clone()); assert_eq!(ReceiptInfoData::::get(), receipt_data); diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index 0cca103e66607..b9cde375ac3f2 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -60,206 +60,12 @@ use frame_system::{EventRecord, Phase}; use pallet_revive_fixtures::compile_module; use pallet_revive_uapi::{ReturnErrorCode as RuntimeReturnCode, ReturnFlags}; use pretty_assertions::{assert_eq, assert_ne}; -use sp_core::{Get, U256}; +use sp_core::U256; use sp_io::hashing::blake2_256; use sp_runtime::{ - testing::H256, traits::Zero, AccountId32, BoundedVec, DispatchError, SaturatedConversion, - TokenError, + testing::H256, AccountId32, BoundedVec, DispatchError, SaturatedConversion, TokenError, }; -#[test] -fn transfer_with_dust_works() { - struct TestCase { - description: &'static str, - from: H160, - to: H160, - from_balance: BalanceWithDust, - to_balance: BalanceWithDust, - amount: BalanceWithDust, - expected_from_balance: BalanceWithDust, - expected_to_balance: BalanceWithDust, - total_issuance_diff: i64, - expected_error: Option, - } - - let plank: u32 = ::NativeToEthRatio::get(); - - let test_cases = vec![ - TestCase { - description: "without dust", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(100, 0), - to_balance: BalanceWithDust::new_unchecked::(0, 0), - amount: BalanceWithDust::new_unchecked::(1, 0), - expected_from_balance: BalanceWithDust::new_unchecked::(99, 0), - expected_to_balance: BalanceWithDust::new_unchecked::(1, 0), - total_issuance_diff: 0, - expected_error: None, - }, - TestCase { - description: "with dust", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(100, 0), - to_balance: BalanceWithDust::new_unchecked::(0, 0), - amount: BalanceWithDust::new_unchecked::(1, 10), - expected_from_balance: BalanceWithDust::new_unchecked::(98, plank - 10), - expected_to_balance: BalanceWithDust::new_unchecked::(1, 10), - total_issuance_diff: 1, - expected_error: None, - }, - TestCase { - description: "just dust", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(100, 0), - to_balance: BalanceWithDust::new_unchecked::(0, 0), - amount: BalanceWithDust::new_unchecked::(0, 10), - expected_from_balance: BalanceWithDust::new_unchecked::(99, plank - 10), - expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), - total_issuance_diff: 1, - expected_error: None, - }, - TestCase { - description: "with existing dust", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(100, 5), - to_balance: BalanceWithDust::new_unchecked::(0, plank - 5), - amount: BalanceWithDust::new_unchecked::(1, 10), - expected_from_balance: BalanceWithDust::new_unchecked::(98, plank - 5), - expected_to_balance: BalanceWithDust::new_unchecked::(2, 5), - total_issuance_diff: 0, - expected_error: None, - }, - TestCase { - description: "with enough existing dust", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(100, 10), - to_balance: BalanceWithDust::new_unchecked::(0, plank - 10), - amount: BalanceWithDust::new_unchecked::(1, 10), - expected_from_balance: BalanceWithDust::new_unchecked::(99, 0), - expected_to_balance: BalanceWithDust::new_unchecked::(2, 0), - total_issuance_diff: -1, - expected_error: None, - }, - TestCase { - description: "receiver dust less than 1 plank", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(100, plank / 10), - to_balance: BalanceWithDust::new_unchecked::(0, plank / 2), - amount: BalanceWithDust::new_unchecked::(1, plank / 10 * 3), - expected_from_balance: BalanceWithDust::new_unchecked::(98, plank / 10 * 8), - expected_to_balance: BalanceWithDust::new_unchecked::(1, plank / 10 * 8), - total_issuance_diff: 1, - expected_error: None, - }, - TestCase { - description: "insufficient balance", - from: ALICE_ADDR, - to: BOB_ADDR, - from_balance: BalanceWithDust::new_unchecked::(10, 0), - to_balance: BalanceWithDust::new_unchecked::(10, 0), - amount: BalanceWithDust::new_unchecked::(20, 0), - expected_from_balance: BalanceWithDust::new_unchecked::(10, 0), - expected_to_balance: BalanceWithDust::new_unchecked::(10, 0), - total_issuance_diff: 0, - expected_error: Some(Error::::TransferFailed.into()), - }, - TestCase { - description: "from = to with insufficient balance", - from: ALICE_ADDR, - to: ALICE_ADDR, - from_balance: BalanceWithDust::new_unchecked::(10, 0), - to_balance: BalanceWithDust::new_unchecked::(10, 0), - amount: BalanceWithDust::new_unchecked::(20, 0), - expected_from_balance: BalanceWithDust::new_unchecked::(10, 0), - expected_to_balance: BalanceWithDust::new_unchecked::(10, 0), - total_issuance_diff: 0, - expected_error: Some(Error::::TransferFailed.into()), - }, - TestCase { - description: "from = to with insufficient balance", - from: ALICE_ADDR, - to: ALICE_ADDR, - from_balance: BalanceWithDust::new_unchecked::(0, 10), - to_balance: BalanceWithDust::new_unchecked::(0, 10), - amount: BalanceWithDust::new_unchecked::(0, 20), - expected_from_balance: BalanceWithDust::new_unchecked::(0, 10), - expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), - total_issuance_diff: 0, - expected_error: Some(Error::::TransferFailed.into()), - }, - TestCase { - description: "from = to", - from: ALICE_ADDR, - to: ALICE_ADDR, - from_balance: BalanceWithDust::new_unchecked::(0, 10), - to_balance: BalanceWithDust::new_unchecked::(0, 10), - amount: BalanceWithDust::new_unchecked::(0, 5), - expected_from_balance: BalanceWithDust::new_unchecked::(0, 10), - expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), - total_issuance_diff: 0, - expected_error: None, - }, - ]; - - for TestCase { - description, - from, - to, - from_balance, - to_balance, - amount, - expected_from_balance, - expected_to_balance, - total_issuance_diff, - expected_error, - } in test_cases.into_iter() - { - ExtBuilder::default().build().execute_with(|| { - set_balance_with_dust(&from, from_balance); - set_balance_with_dust(&to, to_balance); - - let total_issuance = ::Currency::total_issuance(); - let evm_value = Pallet::::convert_native_to_evm(amount); - - let (value, dust) = amount.deconstruct(); - assert_eq!(Pallet::::has_dust(evm_value), !dust.is_zero()); - assert_eq!(Pallet::::has_balance(evm_value), !value.is_zero()); - - let result = builder::bare_call(to).evm_value(evm_value).build(); - - if let Some(expected_error) = expected_error { - assert_err!(result.result, expected_error); - } else { - assert_eq!(result.result.unwrap(), Default::default(), "{description} tx failed"); - } - - assert_eq!( - Pallet::::evm_balance(&from), - Pallet::::convert_native_to_evm(expected_from_balance), - "{description}: invalid from balance" - ); - - assert_eq!( - Pallet::::evm_balance(&to), - Pallet::::convert_native_to_evm(expected_to_balance), - "{description}: invalid to balance" - ); - - assert_eq!( - total_issuance as i64 - total_issuance_diff, - ::Currency::total_issuance() as i64, - "{description}: total issuance should match" - ); - }); - } -} - #[test] fn eth_call_transfer_with_dust_works() { let (binary, _) = compile_module("dummy").unwrap();