diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs index db4465a70862b..fd993b1ba87ca 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs @@ -62,7 +62,7 @@ use frame_system::{ }; use pallet_asset_conversion_tx_payment::SwapAssetAdapter; use pallet_nfts::{DestroyWitness, PalletFeatures}; -use pallet_revive::{evm::runtime::EthExtra, AddressMapper}; +use pallet_revive::{evm::runtime::EthExtra, AddressMapper, NonceAlreadyIncremented}; use pallet_xcm::EnsureXcm; use parachains_common::{ impls::DealWithFees, message_queue::*, AccountId, AssetIdForTrustBackedAssets, AuraId, Balance, @@ -2364,6 +2364,7 @@ impl_runtime_apis! { code, data, salt, + NonceAlreadyIncremented::No, ) } diff --git a/prdoc/pr_8504.prdoc b/prdoc/pr_8504.prdoc new file mode 100644 index 0000000000000..762d407e8582f --- /dev/null +++ b/prdoc/pr_8504.prdoc @@ -0,0 +1,74 @@ +title: Fix generated address returned by Substrate RPC runtime call +doc: +- audience: Runtime Dev + description: |- + ## Description + + When dry-running a contract deployment through the runtime API, the returned address does not match the actual address that will be used when the transaction is submitted. This inconsistency occurs because the address derivation logic doesn't properly account for the difference between transaction execution and dry-run execution contexts. + + The issue stems from the `create1` address derivation logic in `exec.rs`: + + ```rust + address::create1( + &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 origin_is_caller { + account_nonce.saturating_sub(1u32.into()).saturated_into() + } else { + account_nonce.saturated_into() + }, + ) + ``` + + The code correctly subtracts 1 from the account nonce during a transaction execution (because the nonce is incremented pre-dispatch), but doesn't account for execution context - whether it's a real transaction or a dry run through the RPC. + + ## Review Notes + + This PR adds a new condition to check for the `ExecContext` when calculating the nonce for address derivation: + + ```rust + address::create1( + &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 origin_is_caller && matches!(exec_context, ExecContext::Transaction) { + account_nonce.saturating_sub(1u32.into()).saturated_into() + } else { + account_nonce.saturated_into() + }, + ) + ``` + + A new test `nonce_not_incremented_in_dry_run()` has been added to verify the behavior. + + ## Before Fix + + - Dry-run contract deployment returns address derived with nonce N + - Actual transaction deployment creates contract at address derived with nonce N-1 + - Result: Inconsistent addresses between simulation and actual execution + + ## After Fix + + - Dry-run and actual transaction deployments both create contracts at the same address + - Result: Consistent contract addresses regardless of execution context + - Added test case to verify nonce handling in different execution contexts + + This fix ensures that users can rely on the address returned by a dry run to match the actual address that will be used when the transaction is submitted. + + Fixes https://github.com/paritytech/contract-issues/issues/37 + + # Checklist + + * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. + * [x] My PR follows the [labeling requirements]( + https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process + ) of this project (at minimum one label for `T` required) + * External contributors: ask maintainers to put the right label on your PR. + * [x] I have made corresponding changes to the documentation (if applicable) + * [x] I have added tests that prove my fix is effective or that my feature works (if applicable) +crates: +- name: asset-hub-westend-runtime + bump: patch +- name: pallet-revive + bump: major diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index ad1aa3da39085..e3a400b257349 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -85,7 +85,7 @@ use pallet_im_online::sr25519::AuthorityId as ImOnlineId; use pallet_nfts::PalletFeatures; use pallet_nis::WithMaximumOf; use pallet_nomination_pools::PoolId; -use pallet_revive::{evm::runtime::EthExtra, AddressMapper}; +use pallet_revive::{evm::runtime::EthExtra, AddressMapper, NonceAlreadyIncremented}; use pallet_session::historical as pallet_session_historical; use sp_core::U256; use sp_runtime::traits::TransactionExtension; @@ -3443,6 +3443,7 @@ impl_runtime_apis! { code, data, salt, + NonceAlreadyIncremented::No, ) } diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index eb882378e3d2b..e9f0aed920f4f 100644 --- a/substrate/frame/revive/src/call_builder.rs +++ b/substrate/frame/revive/src/call_builder.rs @@ -33,7 +33,8 @@ use crate::{ transient_storage::MeterEntry, wasm::{PreparedCall, Runtime}, BalanceOf, Code, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DepositLimit, Error, - GasMeter, MomentOf, Origin, Pallet as Contracts, PristineCode, WasmBlob, Weight, + GasMeter, MomentOf, NonceAlreadyIncremented, Origin, Pallet as Contracts, PristineCode, + WasmBlob, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::{storage::child, traits::fungible::Mutate}; @@ -270,6 +271,7 @@ where Code::Upload(module.code), data, salt, + NonceAlreadyIncremented::No, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 25a1d1ebc2146..aa4c7d3ce1418 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, Pallet as Contracts, + Error, Event, ImmutableData, ImmutableDataOf, NonceAlreadyIncremented, Pallet as Contracts, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -614,6 +614,7 @@ 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, }, } @@ -807,6 +808,7 @@ 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 { @@ -814,6 +816,7 @@ where executable, salt, input_data: input_data.as_ref(), + nonce_already_incremented, }, Origin::from_account_id(origin), gas_meter, @@ -874,7 +877,6 @@ where storage_meter, BalanceOf::::max_value(), false, - true, )? else { return Ok(None); @@ -908,7 +910,6 @@ 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 } => { @@ -972,7 +973,13 @@ where (dest, contract, executable, delegated_call, ExportedFunction::Call) }, - FrameArgs::Instantiate { sender, executable, salt, input_data } => { + FrameArgs::Instantiate { + sender, + executable, + salt, + input_data, + nonce_already_incremented, + } => { let deployer = T::AddressMapper::to_address(&sender); let account_nonce = >::account_nonce(&sender); let address = if let Some(salt) = salt { @@ -983,7 +990,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 origin_is_caller { + if matches!(nonce_already_incremented, NonceAlreadyIncremented::Yes) { account_nonce.saturating_sub(1u32.into()).saturated_into() } else { account_nonce.saturated_into() @@ -1059,7 +1066,6 @@ where nested_storage, deposit_limit, read_only, - false, )? { self.frames.try_push(frame).map_err(|_| Error::::MaxCallDepthReached)?; Ok(Some(executable)) @@ -1665,6 +1671,7 @@ 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 b04a9360aee78..7558495568cdb 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -566,6 +566,7 @@ fn input_data_to_instantiate() { vec![1, 2, 3, 4], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, ); assert_matches!(result, Ok(_)); }); @@ -1069,6 +1070,7 @@ fn refuse_instantiate_with_value_below_existential_deposit() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, ), Err(_) ); @@ -1104,6 +1106,7 @@ 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 ); @@ -1150,6 +1153,7 @@ 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 ); @@ -1313,6 +1317,7 @@ fn termination_from_instantiate_fails() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, ), Err(ExecError { error: Error::::TerminatedInConstructor.into(), @@ -1441,6 +1446,7 @@ fn recursive_call_during_constructor_is_balance_transfer() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, ); assert_matches!(result, Ok(_)); }); @@ -1802,6 +1808,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, ) .ok(); assert_eq!(System::account_nonce(&ALICE), 0); @@ -1815,6 +1822,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 1); @@ -1827,6 +1835,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 2); @@ -1839,6 +1848,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, + NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 3); }); @@ -2825,6 +2835,7 @@ 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 2c8a6f4766a63..dbfbaf76094ea 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -513,6 +513,18 @@ 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>; @@ -797,6 +809,7 @@ pub mod pallet { Code::Existing(code_hash), data, salt, + NonceAlreadyIncremented::Yes, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -861,6 +874,7 @@ pub mod pallet { Code::Upload(code), data, salt, + NonceAlreadyIncremented::Yes, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -1075,6 +1089,7 @@ 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(); @@ -1117,6 +1132,7 @@ where data, salt.as_ref(), unchecked_deposit_limit, + nonce_already_incremented, ); storage_deposit = storage_meter .try_into_deposit(&instantiate_origin, unchecked_deposit_limit)? @@ -1286,6 +1302,7 @@ where Code::Upload(code.to_vec()), data.to_vec(), None, + NonceAlreadyIncremented::No, ); let returned_data = match result.result { diff --git a/substrate/frame/revive/src/test_utils/builder.rs b/substrate/frame/revive/src/test_utils/builder.rs index 799c633deef0b..120fde86e9bb7 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, OriginFor, Pallet, Weight, + ExecReturnValue, InstantiateReturnValue, NonceAlreadyIncremented, OriginFor, Pallet, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::pallet_prelude::DispatchResultWithPostInfo; @@ -138,6 +138,7 @@ builder!( code: Code, data: Vec, salt: Option<[u8; 32]>, + nonce_already_incremented: NonceAlreadyIncremented, ) -> ContractResult>; /// Build the instantiate call and unwrap the result. @@ -165,6 +166,7 @@ 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 69f5ed60d51dc..12c7a6becab03 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4565,3 +4565,46 @@ 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); + }); +}