Fix generated address returned by Substrate RPC runtime call

.github/workflows/tests-evm.yml

@@ -80,6 +80,7 @@ jobs:
           echo "Run the tests"
           echo "bash init.sh --kitchensink -- --matter-labs -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH"
           bash init.sh --kitchensink -- --matter-labs -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH
+          bash init.sh --kitchensink -- --eth-rpc -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH
 
       - name: Collect tests results
         if: always()

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

@@ -2353,6 +2353,7 @@ impl_runtime_apis! {
 				gas_limit.unwrap_or(blockweights.max_block),
 				pallet_revive::DepositLimit::Balance(storage_deposit_limit.unwrap_or(u128::MAX)),
 				input_data,
+				pallet_revive::ExecContext::Transaction,
 			)
 		}
 
@@ -2375,6 +2376,7 @@ impl_runtime_apis! {
 				code,
 				data,
 				salt,
+				pallet_revive::ExecContext::Transaction,
 			)
 		}
 

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: patch

substrate/bin/node/runtime/src/lib.rs

@@ -3432,6 +3432,7 @@ impl_runtime_apis! {
 				gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block),
 				pallet_revive::DepositLimit::Balance(storage_deposit_limit.unwrap_or(u128::MAX)),
 				input_data,
+				pallet_revive::ExecContext::Transaction,
 			)
 		}
 
@@ -3453,6 +3454,7 @@ impl_runtime_apis! {
 				code,
 				data,
 				salt,
+				pallet_revive::ExecContext::Transaction,
 			)
 		}
 

substrate/frame/revive/src/call_builder.rs

@@ -270,6 +270,7 @@ where
 			Code::Upload(module.code),
 			data,
 			salt,
+			crate::ExecContext::Transaction,
 		);
 
 		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, ExecContext, ImmutableData, ImmutableDataOf, Pallet as Contracts,
 };
 use alloc::vec::Vec;
 use core::{fmt::Debug, marker::PhantomData, mem};
@@ -512,9 +512,8 @@ pub struct Stack<'a, T: Config, E> {
 	first_frame: Frame<T>,
 	/// Transient storage used to store data, which is kept for the duration of a transaction.
 	transient_storage: TransientStorage<T>,
-	/// Whether or not actual transfer of funds should be performed.
-	/// This is set to `true` exclusively when we simulate a call through eth_transact.
-	skip_transfer: bool,
+	/// The context associated with the current execution.
+	exec_context: ExecContext,
 	/// No executable is held by the struct but influences its behaviour.
 	_phantom: PhantomData<E>,
 }
@@ -614,6 +613,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],
+		exec_context: ExecContext,
 	},
 }
 
@@ -759,7 +759,7 @@ where
 		storage_meter: &'a mut storage::meter::Meter<T>,
 		value: U256,
 		input_data: Vec<u8>,
-		skip_transfer: bool,
+		exec_context: ExecContext,
 	) -> ExecResult {
 		let dest = T::AddressMapper::to_account_id(&dest);
 		if let Some((mut stack, executable)) = Self::new(
@@ -768,7 +768,7 @@ where
 			gas_meter,
 			storage_meter,
 			value,
-			skip_transfer,
+			exec_context,
 		)? {
 			stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output)
 		} else {
@@ -806,20 +806,21 @@ where
 		value: U256,
 		input_data: Vec<u8>,
 		salt: Option<&[u8; 32]>,
-		skip_transfer: bool,
+		exec_context: ExecContext,
 	) -> Result<(H160, ExecReturnValue), ExecError> {
 		let (mut stack, executable) = Self::new(
 			FrameArgs::Instantiate {
 				sender: origin.clone(),
 				executable,
 				salt,
 				input_data: input_data.as_ref(),
+				exec_context,
 			},
 			Origin::from_account_id(origin),
 			gas_meter,
 			storage_meter,
 			value,
-			skip_transfer,
+			exec_context,
 		)?
 		.expect(FRAME_ALWAYS_EXISTS_ON_INSTANTIATE);
 		let address = T::AddressMapper::to_address(&stack.top_frame().account_id);
@@ -846,7 +847,7 @@ where
 			gas_meter,
 			storage_meter,
 			value.into(),
-			false,
+			ExecContext::Transaction,
 		)
 		.unwrap()
 		.unwrap();
@@ -863,7 +864,7 @@ where
 		gas_meter: &'a mut GasMeter<T>,
 		storage_meter: &'a mut storage::meter::Meter<T>,
 		value: U256,
-		skip_transfer: bool,
+		exec_context: ExecContext,
 	) -> Result<Option<(Self, ExecutableOrPrecompile<T, E, Self>)>, ExecError> {
 		origin.ensure_mapped()?;
 		let Some((first_frame, executable)) = Self::new_frame(
@@ -889,7 +890,7 @@ where
 			first_frame,
 			frames: Default::default(),
 			transient_storage: TransientStorage::new(limits::TRANSIENT_STORAGE_BYTES),
-			skip_transfer,
+			exec_context,
 			_phantom: Default::default(),
 		};
 
@@ -972,7 +973,7 @@ where
 
 				(dest, contract, executable, delegated_call, ExportedFunction::Call)
 			},
-			FrameArgs::Instantiate { sender, executable, salt, input_data } => {
+			FrameArgs::Instantiate { sender, executable, salt, input_data, exec_context } => {
 				let deployer = T::AddressMapper::to_address(&sender);
 				let account_nonce = <System<T>>::account_nonce(&sender);
 				let address = if let Some(salt) = salt {
@@ -983,7 +984,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 origin_is_caller && matches!(exec_context, ExecContext::Transaction) {
 							account_nonce.saturating_sub(1u32.into()).saturated_into()
 						} else {
 							account_nonce.saturated_into()
@@ -1119,10 +1120,14 @@ where
 
 				let ed = <Contracts<T>>::min_balance();
 				frame.nested_storage.record_charge(&StorageDeposit::Charge(ed));
-				if self.skip_transfer {
-					T::Currency::set_balance(account_id, ed);
-				} else {
-					T::Currency::transfer(origin, account_id, ed, Preservation::Preserve)?;
+
+				match self.exec_context {
+					ExecContext::DryRun { skip_transfer: true } => {
+						T::Currency::set_balance(account_id, ed);
+					},
+					_ => {
+						T::Currency::transfer(origin, account_id, ed, Preservation::Preserve)?;
+					},
 				}
 
 				// A consumer is added at account creation and removed it on termination, otherwise
@@ -1665,6 +1670,7 @@ where
 				executable,
 				salt,
 				input_data: input_data.as_ref(),
+				exec_context: self.exec_context,
 			},
 			value.try_into().map_err(|_| Error::<T>::BalanceConversionFailed)?,
 			gas_limit,