diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs index 1eda07dda7cb5..2208c0903067f 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs @@ -50,7 +50,7 @@ use frame_support::{ weights::{Weight, WeightToFee as WeightToFeeT}, }; use hex_literal::hex; -use pallet_revive::{Code, DepositLimit, InstantiateReturnValue, NonceAlreadyIncremented}; +use pallet_revive::{Code, DepositLimit, InstantiateReturnValue}; use pallet_revive_fixtures::compile_module; use parachains_common::{AccountId, AssetIdForTrustBackedAssets, AuraId, Balance}; use sp_consensus_aura::SlotDuration; @@ -1514,7 +1514,6 @@ fn withdraw_and_deposit_erc20s() { Code::Upload(code), constructor_data, None, - NonceAlreadyIncremented::Yes, ); let Ok(InstantiateReturnValue { addr: erc20_address, .. }) = result.result else { unreachable!("contract should initialize") @@ -1628,7 +1627,6 @@ fn smart_contract_not_erc20_will_error() { Code::Upload(code), Vec::new(), None, - NonceAlreadyIncremented::Yes, ); let Ok(InstantiateReturnValue { addr: non_erc20_address, .. }) = result.result else { unreachable!("contract should initialize") @@ -1693,7 +1691,6 @@ fn smart_contract_does_not_return_bool_fails() { Code::Upload(code), constructor_data, None, - NonceAlreadyIncremented::Yes, ); let Ok(InstantiateReturnValue { addr: non_erc20_address, .. }) = result.result else { unreachable!("contract should initialize") @@ -1756,7 +1753,6 @@ fn expensive_erc20_runs_out_of_gas() { Code::Upload(code), constructor_data, None, - NonceAlreadyIncremented::Yes, ); let Ok(InstantiateReturnValue { addr: non_erc20_address, .. }) = result.result else { unreachable!("contract should initialize") diff --git a/prdoc/pr_8662.prdoc b/prdoc/pr_8662.prdoc new file mode 100644 index 0000000000000..6a976acb67370 --- /dev/null +++ b/prdoc/pr_8662.prdoc @@ -0,0 +1,10 @@ +title: '[pallet-revive] update dry-run logic' +doc: +- audience: Runtime Dev + description: |- + - Revert #8504 + - Add a `prepare_dry_run` that run before dry_run + +crates: +- name: pallet-revive + bump: major diff --git a/substrate/frame/revive/rpc/src/tests.rs b/substrate/frame/revive/rpc/src/tests.rs index 30013085afc56..8dcc59ecfdcc8 100644 --- a/substrate/frame/revive/rpc/src/tests.rs +++ b/substrate/frame/revive/rpc/src/tests.rs @@ -21,6 +21,8 @@ use crate::{ cli::{self, CliCommand}, example::TransactionBuilder, + subxt_client, + subxt_client::{src_chain::runtime_types::pallet_revive::primitives::Code, SrcChainConfig}, EthRpcClient, }; use clap::Parser; @@ -32,6 +34,7 @@ use pallet_revive::{ use static_init::dynamic; use std::{sync::Arc, thread}; use substrate_cli_test_utils::*; +use subxt::OnlineClient; /// Create a websocket client with a 120s timeout. async fn ws_client_with_retry(url: &str) -> WsClient { @@ -141,7 +144,6 @@ async fn deploy_and_call() -> anyhow::Result<()> { // Balance transfer let ethan = Account::from(subxt_signer::eth::dev::ethan()); let initial_balance = client.get_balance(ethan.address(), BlockTag::Latest.into()).await?; - let value = 1_000_000_000_000_000_000_000u128.into(); let tx = TransactionBuilder::new(&client).value(value).to(ethan.address()).send().await?; @@ -218,6 +220,37 @@ async fn deploy_and_call() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn runtime_api_dry_run_addr_works() -> anyhow::Result<()> { + let _lock = SHARED_RESOURCES.write(); + let client = std::sync::Arc::new(SharedResources::client().await); + + let account = Account::default(); + let origin: [u8; 32] = account.substrate_account().into(); + let data = b"hello world".to_vec(); + let value = 5_000_000_000_000u128; + let (bytes, _) = pallet_revive_fixtures::compile_module("dummy")?; + + let payload = subxt_client::apis().revive_api().instantiate( + subxt::utils::AccountId32(origin), + value, + None, + None, + Code::Upload(bytes), + data, + None, + ); + + let nonce = client.get_transaction_count(account.address(), BlockTag::Latest.into()).await?; + let contract_address = create1(&account.address(), nonce.try_into().unwrap()); + + let c = OnlineClient::::from_url("ws://localhost:45789").await?; + let res = c.runtime_api().at_latest().await?.call(payload).await?.result.unwrap(); + + assert_eq!(res.addr, contract_address); + Ok(()) +} + #[tokio::test] async fn invalid_transaction() -> anyhow::Result<()> { let _lock = SHARED_RESOURCES.write(); diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index 8fe0d1a6e77fe..0c5b1503902ec 100644 --- a/substrate/frame/revive/src/call_builder.rs +++ b/substrate/frame/revive/src/call_builder.rs @@ -33,8 +33,7 @@ use crate::{ transient_storage::MeterEntry, wasm::{PreparedCall, Runtime}, BalanceOf, Code, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DepositLimit, Error, - GasMeter, MomentOf, NonceAlreadyIncremented, Origin, Pallet as Contracts, PristineCode, - WasmBlob, Weight, + GasMeter, MomentOf, Origin, Pallet as Contracts, PristineCode, WasmBlob, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::{storage::child, traits::fungible::Mutate}; @@ -271,7 +270,6 @@ where Code::Upload(module.code), data, salt, - NonceAlreadyIncremented::No, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/evm/runtime.rs b/substrate/frame/revive/src/evm/runtime.rs index 1620316578389..dbb274e4b2e62 100644 --- a/substrate/frame/revive/src/evm/runtime.rs +++ b/substrate/frame/revive/src/evm/runtime.rs @@ -468,7 +468,7 @@ mod test { } fn estimate_gas(&mut self) { - let dry_run = crate::Pallet::::bare_eth_transact( + let dry_run = crate::Pallet::::dry_run_eth_transact( self.tx.clone(), Weight::MAX, |call, mut info| { diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 7797b7b2e475b..5effbb96da5d4 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -26,7 +26,7 @@ use crate::{ tracing::if_tracing, transient_storage::TransientStorage, BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, ConversionPrecision, - Error, Event, ImmutableData, ImmutableDataOf, NonceAlreadyIncremented, Pallet as Contracts, + Error, Event, ImmutableData, ImmutableDataOf, Pallet as Contracts, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -619,7 +619,6 @@ enum FrameArgs<'a, T: Config, E> { salt: Option<&'a [u8; 32]>, /// The input data is used in the contract address derivation of the new contract. input_data: &'a [u8], - nonce_already_incremented: NonceAlreadyIncremented, }, } @@ -813,7 +812,6 @@ where input_data: Vec, salt: Option<&[u8; 32]>, skip_transfer: bool, - nonce_already_incremented: NonceAlreadyIncremented, ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { @@ -821,7 +819,6 @@ where executable, salt, input_data: input_data.as_ref(), - nonce_already_incremented, }, Origin::from_account_id(origin), gas_meter, @@ -882,6 +879,7 @@ where storage_meter, BalanceOf::::max_value(), false, + true, )? else { return Ok(None); @@ -915,6 +913,7 @@ where storage_meter: &mut storage::meter::GenericMeter, deposit_limit: BalanceOf, read_only: bool, + origin_is_caller: bool, ) -> Result, ExecutableOrPrecompile)>, ExecError> { let (account_id, contract_info, executable, delegate, entry_point) = match frame_args { FrameArgs::Call { dest, cached_info, delegated_call } => { @@ -978,13 +977,7 @@ where (dest, contract, executable, delegated_call, ExportedFunction::Call) }, - FrameArgs::Instantiate { - sender, - executable, - salt, - input_data, - nonce_already_incremented, - } => { + FrameArgs::Instantiate { sender, executable, salt, input_data } => { let deployer = T::AddressMapper::to_address(&sender); let account_nonce = >::account_nonce(&sender); let address = if let Some(salt) = salt { @@ -995,7 +988,7 @@ where &deployer, // the Nonce from the origin has been incremented pre-dispatch, so we // need to subtract 1 to get the nonce at the time of the call. - if matches!(nonce_already_incremented, NonceAlreadyIncremented::Yes) { + if origin_is_caller { account_nonce.saturating_sub(1u32.into()).saturated_into() } else { account_nonce.saturated_into() @@ -1071,6 +1064,7 @@ where nested_storage, deposit_limit, read_only, + false, )? { self.frames.try_push(frame).map_err(|_| Error::::MaxCallDepthReached)?; Ok(Some(executable)) @@ -1680,7 +1674,6 @@ where executable, salt, input_data: input_data.as_ref(), - nonce_already_incremented: NonceAlreadyIncremented::No, }, value.try_into().map_err(|_| Error::::BalanceConversionFailed)?, gas_limit, diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index ef2b165fd337b..63d0ffb87bc42 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -566,7 +566,6 @@ fn input_data_to_instantiate() { vec![1, 2, 3, 4], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, ); assert_matches!(result, Ok(_)); }); @@ -1069,7 +1068,6 @@ fn refuse_instantiate_with_value_below_existential_deposit() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, ), Err(_) ); @@ -1103,7 +1101,6 @@ fn instantiation_work_with_success_output() { vec![], Some(&[0 ;32]), false, - NonceAlreadyIncremented::Yes, ), Ok((address, ref output)) if output.data == vec![80, 65, 83, 83] => address ); @@ -1148,7 +1145,6 @@ fn instantiation_fails_with_failing_output() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, ), Ok((address, ref output)) if output.data == vec![70, 65, 73, 76] => address ); @@ -1309,7 +1305,6 @@ fn termination_from_instantiate_fails() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, ), Err(ExecError { error: Error::::TerminatedInConstructor.into(), @@ -1436,7 +1431,6 @@ fn recursive_call_during_constructor_is_balance_transfer() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, ); assert_matches!(result, Ok(_)); }); @@ -1795,7 +1789,6 @@ fn nonce() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, ) .ok(); assert_eq!(System::account_nonce(&ALICE), 0); @@ -1809,7 +1802,6 @@ fn nonce() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 1); @@ -1822,7 +1814,6 @@ fn nonce() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 2); @@ -1835,7 +1826,6 @@ fn nonce() { vec![], Some(&[0; 32]), false, - NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 3); }); @@ -2815,7 +2805,6 @@ fn immutable_data_set_overrides() { vec![], None, false, - NonceAlreadyIncremented::Yes, ) .unwrap() .0; diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index c3a3b1ad81741..e84fac8b69efb 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -519,18 +519,6 @@ pub mod pallet { AddressMapping, } - #[derive(Clone, Copy, Debug, PartialEq, Eq)] - pub enum NonceAlreadyIncremented { - /// Indicates that the nonce has not been incremented yet. - /// - /// This happens when the instantiation is triggered by a dry-run or another contract. - No, - /// Indicates that the nonce has already been incremented. - /// - /// This happens when the instantiation is triggered by a transaction. - Yes, - } - /// A mapping from a contract's code hash to its code. #[pallet::storage] pub(crate) type PristineCode = StorageMap<_, Identity, H256, CodeVec>; @@ -815,7 +803,6 @@ pub mod pallet { Code::Existing(code_hash), data, salt, - NonceAlreadyIncremented::Yes, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -880,7 +867,6 @@ pub mod pallet { Code::Upload(code), data, salt, - NonceAlreadyIncremented::Yes, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -1082,6 +1068,17 @@ where } } + /// Prepare a dry run for the given account. + /// + /// + /// This function is public because it is called by the runtime API implementation + /// (see `impl_runtime_apis_plus_revive`). + pub fn prepare_dry_run(account: &T::AccountId) { + // Bump the nonce to simulate what would happen + // `pre-dispatch` if the transaction was executed. + frame_system::Pallet::::inc_account_nonce(account); + } + /// A generalized version of [`Self::instantiate`] or [`Self::instantiate_with_code`]. /// /// Identical to [`Self::instantiate`] or [`Self::instantiate_with_code`] but tailored towards @@ -1095,7 +1092,6 @@ where code: Code, data: Vec, salt: Option<[u8; 32]>, - nonce_already_incremented: NonceAlreadyIncremented, ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1138,7 +1134,6 @@ where data, salt.as_ref(), unchecked_deposit_limit, - nonce_already_incremented, ); storage_deposit = storage_meter .try_into_deposit(&instantiate_origin, unchecked_deposit_limit)? @@ -1156,14 +1151,14 @@ where } } - /// A version of [`Self::eth_transact`] used to dry-run Ethereum calls. + /// Dry-run Ethereum calls. /// /// # Parameters /// /// - `tx`: The Ethereum transaction to simulate. /// - `gas_limit`: The gas limit enforced during contract execution. /// - `tx_fee`: A function that returns the fee for the given call and dispatch info. - pub fn bare_eth_transact( + pub fn dry_run_eth_transact( mut tx: GenericTransaction, gas_limit: Weight, tx_fee: impl Fn(Call, DispatchInfo) -> BalanceOf, @@ -1176,10 +1171,11 @@ where T::Nonce: Into, T::Hash: frame_support::traits::IsType, { - log::trace!(target: LOG_TARGET, "bare_eth_transact: tx: {tx:?} gas_limit: {gas_limit:?}"); + log::trace!(target: LOG_TARGET, "dry_run_eth_transact: {tx:?} gas_limit: {gas_limit:?}"); let from = tx.from.unwrap_or_default(); let origin = T::AddressMapper::to_account_id(&from); + Self::prepare_dry_run(&origin); let storage_deposit_limit = if tx.gas.is_some() { DepositLimit::Balance(BalanceOf::::max_value()) @@ -1308,7 +1304,6 @@ where Code::Upload(code.to_vec()), data.to_vec(), None, - NonceAlreadyIncremented::No, ); let returned_data = match result.result { @@ -1580,7 +1575,7 @@ sp_api::decl_runtime_apis! { /// Perform an Ethereum call. /// - /// See [`crate::Pallet::bare_eth_transact`] + /// See [`crate::Pallet::dry_run_eth_transact`] fn eth_transact(tx: GenericTransaction) -> Result, EthTransactError>; /// Upload new code without instantiating a contract from it. @@ -1703,7 +1698,7 @@ macro_rules! impl_runtime_apis_plus_revive { let blockweights: $crate::BlockWeights = ::BlockWeights::get(); - $crate::Pallet::::bare_eth_transact(tx, blockweights.max_block, tx_fee) + $crate::Pallet::::dry_run_eth_transact(tx, blockweights.max_block, tx_fee) } fn call( @@ -1718,10 +1713,9 @@ macro_rules! impl_runtime_apis_plus_revive { let blockweights: $crate::BlockWeights = ::BlockWeights::get(); - let origin = - ::RuntimeOrigin::signed(origin); + $crate::Pallet::::prepare_dry_run(&origin); $crate::Pallet::::bare_call( - origin, + ::RuntimeOrigin::signed(origin), dest, value, gas_limit.unwrap_or(blockweights.max_block), @@ -1743,17 +1737,15 @@ macro_rules! impl_runtime_apis_plus_revive { let blockweights: $crate::BlockWeights = ::BlockWeights::get(); - let origin = - ::RuntimeOrigin::signed(origin); + $crate::Pallet::::prepare_dry_run(&origin); $crate::Pallet::::bare_instantiate( - origin, + ::RuntimeOrigin::signed(origin), value, gas_limit.unwrap_or(blockweights.max_block), $crate::DepositLimit::Balance(storage_deposit_limit.unwrap_or(u128::MAX)), code, data, salt, - $crate::NonceAlreadyIncremented::No, ) } diff --git a/substrate/frame/revive/src/test_utils/builder.rs b/substrate/frame/revive/src/test_utils/builder.rs index 120fde86e9bb7..799c633deef0b 100644 --- a/substrate/frame/revive/src/test_utils/builder.rs +++ b/substrate/frame/revive/src/test_utils/builder.rs @@ -18,7 +18,7 @@ use super::{deposit_limit, GAS_LIMIT}; use crate::{ address::AddressMapper, AccountIdOf, BalanceOf, Code, Config, ContractResult, DepositLimit, - ExecReturnValue, InstantiateReturnValue, NonceAlreadyIncremented, OriginFor, Pallet, Weight, + ExecReturnValue, InstantiateReturnValue, OriginFor, Pallet, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::pallet_prelude::DispatchResultWithPostInfo; @@ -138,7 +138,6 @@ builder!( code: Code, data: Vec, salt: Option<[u8; 32]>, - nonce_already_incremented: NonceAlreadyIncremented, ) -> ContractResult>; /// Build the instantiate call and unwrap the result. @@ -166,7 +165,6 @@ builder!( code, data: vec![], salt: Some([0; 32]), - nonce_already_incremented: NonceAlreadyIncremented::Yes, } } ); diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index b9251e1168b32..f1f9d67c0cb83 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4015,7 +4015,7 @@ fn skip_transfer_works() { // when gas is some (transfers enabled): bob has no money: fail assert_err!( - Pallet::::bare_eth_transact( + Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), input: code.clone().into(), @@ -4031,7 +4031,7 @@ fn skip_transfer_works() { ); // no gas specified (all transfers are skipped): even without money bob can deploy - assert_ok!(Pallet::::bare_eth_transact( + assert_ok!(Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), input: code.clone().into(), @@ -4049,7 +4049,7 @@ fn skip_transfer_works() { // call directly: fails with enabled transfers assert_err!( - Pallet::::bare_eth_transact( + Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(addr), @@ -4069,7 +4069,7 @@ fn skip_transfer_works() { // we didn't roll back the storage changes done by the previous // call. So the item already exists. We simply increase the size of // the storage item to incur some deposits (which bob can't pay). - assert!(Pallet::::bare_eth_transact( + assert!(Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(caller_addr), @@ -4083,7 +4083,7 @@ fn skip_transfer_works() { .is_err(),); // works when no gas is specified (skip transfer) - assert_ok!(Pallet::::bare_eth_transact( + assert_ok!(Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(addr), @@ -4095,7 +4095,7 @@ fn skip_transfer_works() { )); // call through contract works when transfers are skipped - assert_ok!(Pallet::::bare_eth_transact( + assert_ok!(Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(caller_addr), @@ -4108,7 +4108,7 @@ fn skip_transfer_works() { // works with transfers enabled if we don't incur a storage cost // we shrink the item so its actually a refund - assert_ok!(Pallet::::bare_eth_transact( + assert_ok!(Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(caller_addr), @@ -4121,7 +4121,7 @@ fn skip_transfer_works() { )); // fails when trying to increase the storage item size - assert!(Pallet::::bare_eth_transact( + assert!(Pallet::::dry_run_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(caller_addr), @@ -4598,46 +4598,3 @@ fn precompiles_with_info_creates_contract() { }); } } - -#[test] -fn nonce_incremented_dry_run_vs_execute() { - let (wasm, _code_hash) = compile_module("dummy").unwrap(); - - ExtBuilder::default().build().execute_with(|| { - let _ = ::Currency::set_balance(&ALICE, 1_000_000); - - // Set a known nonce - let initial_nonce = 5; - frame_system::Account::::mutate(&ALICE, |account| { - account.nonce = initial_nonce; - }); - - // stimulate a dry run - let dry_run_result = builder::bare_instantiate(Code::Upload(wasm.clone())) - .nonce_already_incremented(crate::NonceAlreadyIncremented::No) - .salt(None) - .build(); - - let dry_run_addr = dry_run_result.result.unwrap().addr; - - let deployer = ::AddressMapper::to_address(&ALICE); - let expected_addr = create1(&deployer, initial_nonce.into()); - - assert_eq!(dry_run_addr, expected_addr); - - // reset nonce to initial value - frame_system::Account::::mutate(&ALICE, |account| { - account.nonce = initial_nonce; - }); - - // stimulate an actual execution - let exec_result = builder::bare_instantiate(Code::Upload(wasm.clone())).salt(None).build(); - - let exec_addr = exec_result.result.unwrap().addr; - - let deployer = ::AddressMapper::to_address(&ALICE); - let expected_addr = create1(&deployer, (initial_nonce - 1).into()); - - assert_eq!(exec_addr, expected_addr); - }); -}