Fix generated address returned by Substrate RPC runtime call

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,
 			)
 		}
 

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

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,
 			)
 		}
 

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;

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,13 +808,15 @@ where
 		input_data: Vec<u8>,
 		salt: Option<&[u8; 32]>,
 		skip_transfer: bool,
+		nonce_already_incremented: NonceAlreadyIncremented,
 	) -> Result<(H160, ExecReturnValue), ExecError> {
 		let (mut stack, executable) = Self::new(
 			FrameArgs::Instantiate {
 				sender: origin.clone(),
 				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::<T>::max_value(),
 			false,
-			true,
 		)?
 		else {
 			return Ok(None);
@@ -908,7 +910,6 @@ where
 		storage_meter: &mut storage::meter::GenericMeter<T, S>,
 		deposit_limit: BalanceOf<T>,
 		read_only: bool,
-		origin_is_caller: bool,
 	) -> Result<Option<(Frame<T>, ExecutableOrPrecompile<T, E, Self>)>, 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 = <System<T>>::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::<T>::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::<T>::BalanceConversionFailed)?,
 			gas_limit,

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::<Test>::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;

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<T: Config> = 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<u8>,
 		salt: Option<[u8; 32]>,
+		nonce_already_incremented: NonceAlreadyIncremented,
 	) -> ContractResult<InstantiateReturnValue, BalanceOf<T>> {
 		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 {

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<u8>,
 		salt: Option<[u8; 32]>,
+		nonce_already_incremented: NonceAlreadyIncremented,
 	) -> ContractResult<InstantiateReturnValue, BalanceOf<T>>;
 
 	/// 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,
 		}
 	}
 );