From 75cffc38bbdae238dfda8c10f641c441bc2ac8e8 Mon Sep 17 00:00:00 2001 From: castillax Date: Mon, 12 May 2025 17:55:02 +0100 Subject: [PATCH 01/17] Fix generated address returned by Substrate RPC runtime call --- .../assets/asset-hub-westend/src/lib.rs | 2 + substrate/bin/node/runtime/src/lib.rs | 2 + substrate/frame/revive/src/call_builder.rs | 1 + substrate/frame/revive/src/exec.rs | 50 ++++---- substrate/frame/revive/src/exec/tests.rs | 120 +++++++++--------- substrate/frame/revive/src/lib.rs | 23 +++- .../frame/revive/src/test_utils/builder.rs | 6 +- 7 files changed, 114 insertions(+), 90 deletions(-) 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 fcdbf9f4bc228..bd112d931d0f5 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/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, ) } diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 9d696030136e6..5b59ddc3a047b 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/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, ) } diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index eb882378e3d2b..5aa99eb24d212 100644 --- a/substrate/frame/revive/src/call_builder.rs +++ b/substrate/frame/revive/src/call_builder.rs @@ -270,6 +270,7 @@ where Code::Upload(module.code), data, salt, + crate::ExecContext::DryRun{skip_transfer: true}, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 25a1d1ebc2146..91ce09d721092 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -16,17 +16,7 @@ // limitations under the License. use crate::{ - address::{self, AddressMapper}, - gas::GasMeter, - limits, - precompiles::{All as AllPrecompiles, Instance as PrecompileInstance, Precompiles}, - primitives::{ExecReturnValue, StorageDeposit}, - runtime_decl_for_revive_api::{Decode, Encode, RuntimeDebugNoBound, TypeInfo}, - storage::{self, meter::Diff, WriteOutcome}, - tracing::if_tracing, - transient_storage::TransientStorage, - BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, ConversionPrecision, - Error, Event, ImmutableData, ImmutableDataOf, Pallet as Contracts, + address::{self, AddressMapper}, gas::GasMeter, limits, precompiles::{All as AllPrecompiles, Instance as PrecompileInstance, Precompiles}, primitives::{ExecReturnValue, StorageDeposit}, runtime_decl_for_revive_api::{Decode, Encode, RuntimeDebugNoBound, TypeInfo}, storage::{self, meter::Diff, WriteOutcome}, tracing::if_tracing, transient_storage::TransientStorage, BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, ConversionPrecision, Error, Event, ExecContext, ImmutableData, ImmutableDataOf, Pallet as Contracts }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -512,9 +502,8 @@ pub struct Stack<'a, T: Config, E> { first_frame: Frame, /// Transient storage used to store data, which is kept for the duration of a transaction. transient_storage: TransientStorage, - /// 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, } @@ -614,6 +603,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 +749,7 @@ where storage_meter: &'a mut storage::meter::Meter, value: U256, input_data: Vec, - 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 +758,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,7 +796,7 @@ where value: U256, input_data: Vec, salt: Option<&[u8; 32]>, - skip_transfer: bool, + exec_context: ExecContext, ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { @@ -814,12 +804,13 @@ where 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 +837,7 @@ where gas_meter, storage_meter, value.into(), - false, + ExecContext::DryRun{skip_transfer: false}, ) .unwrap() .unwrap(); @@ -863,7 +854,7 @@ where gas_meter: &'a mut GasMeter, storage_meter: &'a mut storage::meter::Meter, value: U256, - skip_transfer: bool, + exec_context: ExecContext, ) -> Result)>, ExecError> { origin.ensure_mapped()?; let Some((first_frame, executable)) = Self::new_frame( @@ -889,7 +880,7 @@ where first_frame, frames: Default::default(), transient_storage: TransientStorage::new(limits::TRANSIENT_STORAGE_BYTES), - skip_transfer, + exec_context, _phantom: Default::default(), }; @@ -972,7 +963,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 = >::account_nonce(&sender); let address = if let Some(salt) = salt { @@ -983,7 +974,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 +1110,14 @@ where let ed = >::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 +1660,7 @@ where executable, salt, input_data: input_data.as_ref(), + exec_context: self.exec_context, }, 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..1c4fc8e17a22a 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -219,7 +219,7 @@ fn it_works() { &mut storage_meter, value.into(), vec![], - false, + ExecContext::Transaction, ), Ok(_) ); @@ -311,7 +311,7 @@ fn correct_transfer_on_call() { &mut storage_meter, value.into(), vec![], - false, + ExecContext::Transaction, ) .unwrap(); @@ -350,7 +350,7 @@ fn correct_transfer_on_delegate_call() { &mut storage_meter, value.into(), vec![], - false, + ExecContext::Transaction, )); assert_eq!(get_balance(&ALICE), 100 - value); @@ -384,7 +384,7 @@ fn delegate_call_missing_contract() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); // add missing contract code @@ -396,7 +396,7 @@ fn delegate_call_missing_contract() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -424,7 +424,7 @@ fn changes_are_reverted_on_failing_call() { &mut storage_meter, 55u64.into(), vec![], - false, + ExecContext::Transaction, ) .unwrap(); @@ -473,7 +473,7 @@ fn output_is_returned_on_success() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); let output = result.unwrap(); @@ -502,7 +502,7 @@ fn output_is_returned_on_failure() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); let output = result.unwrap(); @@ -531,7 +531,7 @@ fn input_data_to_call() { &mut storage_meter, U256::zero(), vec![1, 2, 3, 4], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -565,7 +565,7 @@ fn input_data_to_instantiate() { min_balance.into(), vec![1, 2, 3, 4], Some(&[0; 32]), - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -619,7 +619,7 @@ fn max_depth() { &mut storage_meter, value.into(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); @@ -681,7 +681,7 @@ fn caller_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); @@ -744,7 +744,7 @@ fn origin_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); @@ -776,7 +776,7 @@ fn is_contract_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -810,7 +810,7 @@ fn to_account_id_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -847,7 +847,7 @@ fn code_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -873,7 +873,7 @@ fn own_code_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -909,7 +909,7 @@ fn caller_is_origin_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -935,7 +935,7 @@ fn root_caller_succeeds() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -961,7 +961,7 @@ fn root_caller_does_not_succeed_when_value_not_zero() { &mut storage_meter, 1u64.into(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Err(_)); }); @@ -997,7 +997,7 @@ fn root_caller_succeeds_with_consecutive_calls() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -1042,7 +1042,7 @@ fn address_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); @@ -1068,7 +1068,7 @@ fn refuse_instantiate_with_value_below_existential_deposit() { U256::zero(), // <- zero value vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, ), Err(_) ); @@ -1103,7 +1103,7 @@ fn instantiation_work_with_success_output() { min_balance.into(), vec![], Some(&[0 ;32]), - false, + ExecContext::Transaction, ), Ok((address, ref output)) if output.data == vec![80, 65, 83, 83] => address ); @@ -1149,7 +1149,7 @@ fn instantiation_fails_with_failing_output() { min_balance.into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, ), Ok((address, ref output)) if output.data == vec![70, 65, 73, 76] => address ); @@ -1211,7 +1211,7 @@ fn instantiation_from_contract() { &mut storage_meter, (min_balance * 10).into(), vec![], - false, + ExecContext::Transaction, ), Ok(_) ); @@ -1277,7 +1277,7 @@ fn instantiation_traps() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ), Ok(_) ); @@ -1312,7 +1312,7 @@ fn termination_from_instantiate_fails() { 100u64.into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, ), Err(ExecError { error: Error::::TerminatedInConstructor.into(), @@ -1379,7 +1379,7 @@ fn in_memory_changes_not_discarded() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -1440,7 +1440,7 @@ fn recursive_call_during_constructor_is_balance_transfer() { 10u64.into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -1486,7 +1486,7 @@ fn cannot_send_more_balance_than_available_to_self() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap(); }); @@ -1518,7 +1518,7 @@ fn call_reentry_direct_recursion() { &mut storage_meter, U256::zero(), CHARLIE_ADDR.as_bytes().to_vec(), - false, + ExecContext::Transaction, )); // Calling into oneself fails @@ -1530,7 +1530,7 @@ fn call_reentry_direct_recursion() { &mut storage_meter, U256::zero(), BOB_ADDR.as_bytes().to_vec(), - false, + ExecContext::Transaction, ) .map_err(|e| e.error), >::ReentranceDenied, @@ -1580,7 +1580,7 @@ fn call_deny_reentry() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ) .map_err(|e| e.error), >::ReentranceDenied, @@ -1614,7 +1614,7 @@ fn call_runtime_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap(); @@ -1686,7 +1686,7 @@ fn call_runtime_filter() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap(); @@ -1801,7 +1801,7 @@ fn nonce() { (min_balance * 100).into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, ) .ok(); assert_eq!(System::account_nonce(&ALICE), 0); @@ -1814,7 +1814,7 @@ fn nonce() { (min_balance * 100).into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, )); assert_eq!(System::account_nonce(&ALICE), 1); @@ -1826,7 +1826,7 @@ fn nonce() { (min_balance * 200).into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, )); assert_eq!(System::account_nonce(&ALICE), 2); @@ -1838,7 +1838,7 @@ fn nonce() { (min_balance * 200).into(), vec![], Some(&[0; 32]), - false, + ExecContext::Transaction, )); assert_eq!(System::account_nonce(&ALICE), 3); }); @@ -1906,7 +1906,7 @@ fn set_storage_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2005,7 +2005,7 @@ fn set_storage_varsized_key_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2044,7 +2044,7 @@ fn get_storage_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2083,7 +2083,7 @@ fn get_storage_size_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2133,7 +2133,7 @@ fn get_storage_varsized_key_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2183,7 +2183,7 @@ fn get_storage_size_varsized_key_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2258,7 +2258,7 @@ fn set_transient_storage_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2328,7 +2328,7 @@ fn get_transient_storage_works() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -2366,7 +2366,7 @@ fn get_transient_storage_size_works() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, )); }); } @@ -2428,7 +2428,7 @@ fn rollback_transient_storage_works() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -2459,7 +2459,7 @@ fn ecdsa_to_eth_address_returns_proper_value() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -2532,7 +2532,7 @@ fn last_frame_output_works_on_instantiate() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap() }); @@ -2600,7 +2600,7 @@ fn last_frame_output_works_on_nested_call() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -2668,7 +2668,7 @@ fn last_frame_output_is_always_reset() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ); assert_matches!(result, Ok(_)); }); @@ -2716,7 +2716,7 @@ fn immutable_data_access_checks_work() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap() }); @@ -2785,7 +2785,7 @@ fn correct_immutable_data_in_delegate_call() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap() }); @@ -2824,7 +2824,7 @@ fn immutable_data_set_overrides() { U256::zero(), vec![], None, - false, + ExecContext::Transaction, ) .unwrap() .0; @@ -2836,7 +2836,7 @@ fn immutable_data_set_overrides() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap() }); @@ -2880,7 +2880,7 @@ fn immutable_data_set_errors_with_empty_data() { &mut storage_meter, U256::zero(), vec![], - false, + ExecContext::Transaction, ) .unwrap() }); @@ -2935,7 +2935,7 @@ fn block_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - false, + ExecContext::Transaction, ), Ok(_) ); diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 3ffb88c6b8a87..6c0a26c6a90f9 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -512,6 +512,15 @@ pub mod pallet { AddressMapping, } + #[derive(Clone, Copy, Debug, PartialEq, Eq)] + pub enum ExecContext { + /// A normal transaction execution, where the nonce has been pre-incremented. + Transaction, + + /// A dry run execution (through RPC), where the nonce has not been incremented. + DryRun { skip_transfer: bool }, + } + /// A mapping from a contract's code hash to its code. #[pallet::storage] pub(crate) type PristineCode = StorageMap<_, Identity, H256, CodeVec>; @@ -757,6 +766,7 @@ pub mod pallet { gas_limit, DepositLimit::Balance(storage_deposit_limit), data, + ExecContext::Transaction, ); if let Ok(return_value) = &output.result { @@ -794,6 +804,7 @@ pub mod pallet { Code::Existing(code_hash), data, salt, + ExecContext::Transaction, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -858,6 +869,7 @@ pub mod pallet { Code::Upload(code), data, salt, + ExecContext::Transaction, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -1023,6 +1035,7 @@ where gas_limit: Weight, storage_deposit_limit: DepositLimit>, data: Vec, + exec_context: ExecContext, ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1040,7 +1053,7 @@ where &mut storage_meter, Self::convert_native_to_evm(value), data, - storage_deposit_limit.is_unchecked(), + exec_context, )?; storage_deposit = storage_meter .try_into_deposit(&origin, storage_deposit_limit.is_unchecked()) @@ -1071,6 +1084,7 @@ where code: Code, data: Vec, salt: Option<[u8; 32]>, + exec_context: ExecContext ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1112,7 +1126,7 @@ where Self::convert_native_to_evm(value), data, salt.as_ref(), - unchecked_deposit_limit, + exec_context, ); storage_deposit = storage_meter .try_into_deposit(&instantiate_origin, unchecked_deposit_limit)? @@ -1161,6 +1175,9 @@ where DepositLimit::Unchecked }; + let exec_context = + ExecContext::DryRun { skip_transfer: storage_deposit_limit.is_unchecked() }; + if tx.nonce.is_none() { tx.nonce = Some(>::account_nonce(&origin).into()); } @@ -1222,6 +1239,7 @@ where gas_limit, storage_deposit_limit, input.clone(), + exec_context, ); let data = match result.result { @@ -1282,6 +1300,7 @@ where Code::Upload(code.to_vec()), data.to_vec(), None, + exec_context, ); 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..e59a1c3da62dc 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, + ExecContext, ExecReturnValue, InstantiateReturnValue, 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]>, + exec_context: ExecContext, ) -> ContractResult>; /// Build the instantiate call and unwrap the result. @@ -165,6 +166,7 @@ builder!( code, data: vec![], salt: Some([0; 32]), + exec_context: crate::ExecContext::Transaction, } } ); @@ -200,6 +202,7 @@ builder!( gas_limit: Weight, storage_deposit_limit: DepositLimit>, data: Vec, + exec_context: ExecContext, ) -> ContractResult>; /// Build the call and unwrap the result. @@ -216,6 +219,7 @@ builder!( gas_limit: GAS_LIMIT, storage_deposit_limit: DepositLimit::Balance(deposit_limit::()), data: vec![], + exec_context: crate::ExecContext::Transaction, } } ); From 41fa5d3352f79df8fa9b9e6025007c2fa340dc28 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 13 May 2025 07:20:34 +0000 Subject: [PATCH 02/17] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_8504.prdoc | 74 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 prdoc/pr_8504.prdoc diff --git a/prdoc/pr_8504.prdoc b/prdoc/pr_8504.prdoc new file mode 100644 index 0000000000000..465e9d76bed40 --- /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: patch From e2171b53b695aa24030e5adb00f09ac47ca88b81 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 13 May 2025 08:35:19 +0000 Subject: [PATCH 03/17] Update from github-actions[bot] running command 'fmt' --- substrate/frame/revive/src/call_builder.rs | 2 +- substrate/frame/revive/src/exec.rs | 14 ++++++++++++-- substrate/frame/revive/src/lib.rs | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index 5aa99eb24d212..60bbbb3884384 100644 --- a/substrate/frame/revive/src/call_builder.rs +++ b/substrate/frame/revive/src/call_builder.rs @@ -270,7 +270,7 @@ where Code::Upload(module.code), data, salt, - crate::ExecContext::DryRun{skip_transfer: true}, + crate::ExecContext::DryRun { skip_transfer: true }, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 91ce09d721092..7c15281e6ddf1 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -16,7 +16,17 @@ // limitations under the License. use crate::{ - address::{self, AddressMapper}, gas::GasMeter, limits, precompiles::{All as AllPrecompiles, Instance as PrecompileInstance, Precompiles}, primitives::{ExecReturnValue, StorageDeposit}, runtime_decl_for_revive_api::{Decode, Encode, RuntimeDebugNoBound, TypeInfo}, storage::{self, meter::Diff, WriteOutcome}, tracing::if_tracing, transient_storage::TransientStorage, BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, ConversionPrecision, Error, Event, ExecContext, ImmutableData, ImmutableDataOf, Pallet as Contracts + address::{self, AddressMapper}, + gas::GasMeter, + limits, + precompiles::{All as AllPrecompiles, Instance as PrecompileInstance, Precompiles}, + primitives::{ExecReturnValue, StorageDeposit}, + runtime_decl_for_revive_api::{Decode, Encode, RuntimeDebugNoBound, TypeInfo}, + storage::{self, meter::Diff, WriteOutcome}, + tracing::if_tracing, + transient_storage::TransientStorage, + BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, ConversionPrecision, + Error, Event, ExecContext, ImmutableData, ImmutableDataOf, Pallet as Contracts, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -837,7 +847,7 @@ where gas_meter, storage_meter, value.into(), - ExecContext::DryRun{skip_transfer: false}, + ExecContext::DryRun { skip_transfer: false }, ) .unwrap() .unwrap(); diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 6c0a26c6a90f9..7c23f79bc11a3 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -1084,7 +1084,7 @@ where code: Code, data: Vec, salt: Option<[u8; 32]>, - exec_context: ExecContext + exec_context: ExecContext, ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); From a1cb3e0dc140bc5d259afc637294d9e5502092a1 Mon Sep 17 00:00:00 2001 From: castillax Date: Tue, 13 May 2025 11:25:55 +0100 Subject: [PATCH 04/17] Update exec contexts in some places --- substrate/frame/revive/src/call_builder.rs | 2 +- substrate/frame/revive/src/exec.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index 60bbbb3884384..947c9980bd8fe 100644 --- a/substrate/frame/revive/src/call_builder.rs +++ b/substrate/frame/revive/src/call_builder.rs @@ -270,7 +270,7 @@ where Code::Upload(module.code), data, salt, - crate::ExecContext::DryRun { skip_transfer: true }, + crate::ExecContext::Transaction, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 7c15281e6ddf1..d8d349038974b 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -847,7 +847,7 @@ where gas_meter, storage_meter, value.into(), - ExecContext::DryRun { skip_transfer: false }, + ExecContext::Transaction, ) .unwrap() .unwrap(); From b4047ff7699f7738c5bff6437b6d4cf88b20a671 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 13 May 2025 11:07:35 +0200 Subject: [PATCH 05/17] Update tests-evm --- .github/workflows/tests-evm.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/tests-evm.yml b/.github/workflows/tests-evm.yml index db8f0e558b623..de1a50b236855 100644 --- a/.github/workflows/tests-evm.yml +++ b/.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() From b686c94dabd0e1d2137804f62376ec9f972831c3 Mon Sep 17 00:00:00 2001 From: castillax Date: Wed, 14 May 2025 19:39:34 +0100 Subject: [PATCH 06/17] Use IncrementOnce enum and disassociate skip_transfer --- .../assets/asset-hub-westend/src/lib.rs | 5 +- substrate/bin/node/runtime/src/lib.rs | 5 +- substrate/frame/revive/src/call_builder.rs | 4 +- substrate/frame/revive/src/exec.rs | 46 +++---- substrate/frame/revive/src/exec/tests.rs | 130 ++++++++++-------- substrate/frame/revive/src/lib.rs | 30 ++-- .../frame/revive/src/test_utils/builder.rs | 8 +- 7 files changed, 114 insertions(+), 114 deletions(-) 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 bd112d931d0f5..c35a3bf57fee5 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, IncrementOnce}; use pallet_xcm::EnsureXcm; use parachains_common::{ impls::DealWithFees, message_queue::*, AccountId, AssetIdForTrustBackedAssets, AuraId, Balance, @@ -2353,7 +2353,6 @@ 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, ) } @@ -2376,7 +2375,7 @@ impl_runtime_apis! { code, data, salt, - pallet_revive::ExecContext::Transaction, + IncrementOnce::AlreadyIncremented, ) } diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 5b59ddc3a047b..42655ceb05154 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, IncrementOnce}; use pallet_session::historical as pallet_session_historical; use sp_core::U256; use sp_runtime::traits::TransactionExtension; @@ -3432,7 +3432,6 @@ 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, ) } @@ -3454,7 +3453,7 @@ impl_runtime_apis! { code, data, salt, - pallet_revive::ExecContext::Transaction, + IncrementOnce::AlreadyIncremented, ) } diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index 947c9980bd8fe..7847b843919de 100644 --- a/substrate/frame/revive/src/call_builder.rs +++ b/substrate/frame/revive/src/call_builder.rs @@ -33,7 +33,7 @@ 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, IncrementOnce, MomentOf, Origin, Pallet as Contracts, PristineCode, WasmBlob, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::{storage::child, traits::fungible::Mutate}; @@ -270,7 +270,7 @@ where Code::Upload(module.code), data, salt, - crate::ExecContext::Transaction, + IncrementOnce::No, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index d8d349038974b..5b0fdafb88577 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, ExecContext, ImmutableData, ImmutableDataOf, Pallet as Contracts, + Error, Event, ImmutableData, ImmutableDataOf, IncrementOnce, Pallet as Contracts, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -512,8 +512,9 @@ pub struct Stack<'a, T: Config, E> { first_frame: Frame, /// Transient storage used to store data, which is kept for the duration of a transaction. transient_storage: TransientStorage, - /// The context associated with the current execution. - exec_context: ExecContext, + /// 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, /// No executable is held by the struct but influences its behaviour. _phantom: PhantomData, } @@ -613,7 +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], - exec_context: ExecContext, + increment_once: IncrementOnce, }, } @@ -759,7 +760,7 @@ where storage_meter: &'a mut storage::meter::Meter, value: U256, input_data: Vec, - exec_context: ExecContext, + skip_transfer: bool, ) -> ExecResult { let dest = T::AddressMapper::to_account_id(&dest); if let Some((mut stack, executable)) = Self::new( @@ -768,7 +769,7 @@ where gas_meter, storage_meter, value, - exec_context, + skip_transfer, )? { stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output) } else { @@ -806,7 +807,8 @@ where value: U256, input_data: Vec, salt: Option<&[u8; 32]>, - exec_context: ExecContext, + skip_transfer: bool, + increment_once: IncrementOnce, ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { @@ -814,13 +816,13 @@ where executable, salt, input_data: input_data.as_ref(), - exec_context, + increment_once, }, Origin::from_account_id(origin), gas_meter, storage_meter, value, - exec_context, + skip_transfer, )? .expect(FRAME_ALWAYS_EXISTS_ON_INSTANTIATE); let address = T::AddressMapper::to_address(&stack.top_frame().account_id); @@ -847,7 +849,7 @@ where gas_meter, storage_meter, value.into(), - ExecContext::Transaction, + false, ) .unwrap() .unwrap(); @@ -864,7 +866,7 @@ where gas_meter: &'a mut GasMeter, storage_meter: &'a mut storage::meter::Meter, value: U256, - exec_context: ExecContext, + skip_transfer: bool, ) -> Result)>, ExecError> { origin.ensure_mapped()?; let Some((first_frame, executable)) = Self::new_frame( @@ -890,7 +892,7 @@ where first_frame, frames: Default::default(), transient_storage: TransientStorage::new(limits::TRANSIENT_STORAGE_BYTES), - exec_context, + skip_transfer, _phantom: Default::default(), }; @@ -973,7 +975,7 @@ where (dest, contract, executable, delegated_call, ExportedFunction::Call) }, - FrameArgs::Instantiate { sender, executable, salt, input_data, exec_context } => { + FrameArgs::Instantiate { sender, executable, salt, input_data, increment_once } => { let deployer = T::AddressMapper::to_address(&sender); let account_nonce = >::account_nonce(&sender); let address = if let Some(salt) = salt { @@ -984,7 +986,9 @@ 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 && matches!(exec_context, ExecContext::Transaction) { + if origin_is_caller && + matches!(increment_once, IncrementOnce::AlreadyIncremented) + { account_nonce.saturating_sub(1u32.into()).saturated_into() } else { account_nonce.saturated_into() @@ -1120,14 +1124,10 @@ where let ed = >::min_balance(); frame.nested_storage.record_charge(&StorageDeposit::Charge(ed)); - - 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)?; - }, + if self.skip_transfer { + T::Currency::set_balance(account_id, ed); + } else { + T::Currency::transfer(origin, account_id, ed, Preservation::Preserve)?; } // A consumer is added at account creation and removed it on termination, otherwise @@ -1670,7 +1670,7 @@ where executable, salt, input_data: input_data.as_ref(), - exec_context: self.exec_context, + increment_once: IncrementOnce::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 1c4fc8e17a22a..91b59aa4203ec 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -219,7 +219,7 @@ fn it_works() { &mut storage_meter, value.into(), vec![], - ExecContext::Transaction, + false, ), Ok(_) ); @@ -311,7 +311,7 @@ fn correct_transfer_on_call() { &mut storage_meter, value.into(), vec![], - ExecContext::Transaction, + false, ) .unwrap(); @@ -350,7 +350,7 @@ fn correct_transfer_on_delegate_call() { &mut storage_meter, value.into(), vec![], - ExecContext::Transaction, + false, )); assert_eq!(get_balance(&ALICE), 100 - value); @@ -384,7 +384,7 @@ fn delegate_call_missing_contract() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); // add missing contract code @@ -396,7 +396,7 @@ fn delegate_call_missing_contract() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -424,7 +424,7 @@ fn changes_are_reverted_on_failing_call() { &mut storage_meter, 55u64.into(), vec![], - ExecContext::Transaction, + false, ) .unwrap(); @@ -473,7 +473,7 @@ fn output_is_returned_on_success() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); let output = result.unwrap(); @@ -502,7 +502,7 @@ fn output_is_returned_on_failure() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); let output = result.unwrap(); @@ -531,7 +531,7 @@ fn input_data_to_call() { &mut storage_meter, U256::zero(), vec![1, 2, 3, 4], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -565,7 +565,8 @@ fn input_data_to_instantiate() { min_balance.into(), vec![1, 2, 3, 4], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ); assert_matches!(result, Ok(_)); }); @@ -619,7 +620,7 @@ fn max_depth() { &mut storage_meter, value.into(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); @@ -681,7 +682,7 @@ fn caller_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); @@ -744,7 +745,7 @@ fn origin_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); @@ -776,7 +777,7 @@ fn is_contract_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -810,7 +811,7 @@ fn to_account_id_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -847,7 +848,7 @@ fn code_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -873,7 +874,7 @@ fn own_code_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -909,7 +910,7 @@ fn caller_is_origin_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -935,7 +936,7 @@ fn root_caller_succeeds() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -961,7 +962,7 @@ fn root_caller_does_not_succeed_when_value_not_zero() { &mut storage_meter, 1u64.into(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Err(_)); }); @@ -997,7 +998,7 @@ fn root_caller_succeeds_with_consecutive_calls() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -1042,7 +1043,7 @@ fn address_returns_proper_values() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); @@ -1068,7 +1069,8 @@ fn refuse_instantiate_with_value_below_existential_deposit() { U256::zero(), // <- zero value vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ), Err(_) ); @@ -1103,7 +1105,8 @@ fn instantiation_work_with_success_output() { min_balance.into(), vec![], Some(&[0 ;32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ), Ok((address, ref output)) if output.data == vec![80, 65, 83, 83] => address ); @@ -1149,7 +1152,8 @@ fn instantiation_fails_with_failing_output() { min_balance.into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ), Ok((address, ref output)) if output.data == vec![70, 65, 73, 76] => address ); @@ -1211,7 +1215,7 @@ fn instantiation_from_contract() { &mut storage_meter, (min_balance * 10).into(), vec![], - ExecContext::Transaction, + false, ), Ok(_) ); @@ -1277,7 +1281,7 @@ fn instantiation_traps() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ), Ok(_) ); @@ -1312,7 +1316,8 @@ fn termination_from_instantiate_fails() { 100u64.into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ), Err(ExecError { error: Error::::TerminatedInConstructor.into(), @@ -1379,7 +1384,7 @@ fn in_memory_changes_not_discarded() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -1440,7 +1445,8 @@ fn recursive_call_during_constructor_is_balance_transfer() { 10u64.into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ); assert_matches!(result, Ok(_)); }); @@ -1486,7 +1492,7 @@ fn cannot_send_more_balance_than_available_to_self() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap(); }); @@ -1518,7 +1524,7 @@ fn call_reentry_direct_recursion() { &mut storage_meter, U256::zero(), CHARLIE_ADDR.as_bytes().to_vec(), - ExecContext::Transaction, + false, )); // Calling into oneself fails @@ -1530,7 +1536,7 @@ fn call_reentry_direct_recursion() { &mut storage_meter, U256::zero(), BOB_ADDR.as_bytes().to_vec(), - ExecContext::Transaction, + false, ) .map_err(|e| e.error), >::ReentranceDenied, @@ -1580,7 +1586,7 @@ fn call_deny_reentry() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ) .map_err(|e| e.error), >::ReentranceDenied, @@ -1614,7 +1620,7 @@ fn call_runtime_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap(); @@ -1686,7 +1692,7 @@ fn call_runtime_filter() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap(); @@ -1801,7 +1807,8 @@ fn nonce() { (min_balance * 100).into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ) .ok(); assert_eq!(System::account_nonce(&ALICE), 0); @@ -1814,7 +1821,8 @@ fn nonce() { (min_balance * 100).into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, )); assert_eq!(System::account_nonce(&ALICE), 1); @@ -1826,7 +1834,8 @@ fn nonce() { (min_balance * 200).into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, )); assert_eq!(System::account_nonce(&ALICE), 2); @@ -1838,7 +1847,8 @@ fn nonce() { (min_balance * 200).into(), vec![], Some(&[0; 32]), - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, )); assert_eq!(System::account_nonce(&ALICE), 3); }); @@ -1906,7 +1916,7 @@ fn set_storage_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2005,7 +2015,7 @@ fn set_storage_varsized_key_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2044,7 +2054,7 @@ fn get_storage_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2083,7 +2093,7 @@ fn get_storage_size_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2133,7 +2143,7 @@ fn get_storage_varsized_key_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2183,7 +2193,7 @@ fn get_storage_size_varsized_key_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2258,7 +2268,7 @@ fn set_transient_storage_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2328,7 +2338,7 @@ fn get_transient_storage_works() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -2366,7 +2376,7 @@ fn get_transient_storage_size_works() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, )); }); } @@ -2428,7 +2438,7 @@ fn rollback_transient_storage_works() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -2459,7 +2469,7 @@ fn ecdsa_to_eth_address_returns_proper_value() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -2532,7 +2542,7 @@ fn last_frame_output_works_on_instantiate() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap() }); @@ -2600,7 +2610,7 @@ fn last_frame_output_works_on_nested_call() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -2668,7 +2678,7 @@ fn last_frame_output_is_always_reset() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ); assert_matches!(result, Ok(_)); }); @@ -2716,7 +2726,7 @@ fn immutable_data_access_checks_work() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap() }); @@ -2785,7 +2795,7 @@ fn correct_immutable_data_in_delegate_call() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap() }); @@ -2824,7 +2834,8 @@ fn immutable_data_set_overrides() { U256::zero(), vec![], None, - ExecContext::Transaction, + false, + IncrementOnce::AlreadyIncremented, ) .unwrap() .0; @@ -2836,7 +2847,7 @@ fn immutable_data_set_overrides() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap() }); @@ -2880,7 +2891,7 @@ fn immutable_data_set_errors_with_empty_data() { &mut storage_meter, U256::zero(), vec![], - ExecContext::Transaction, + false, ) .unwrap() }); @@ -2935,7 +2946,6 @@ fn block_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], - ExecContext::Transaction, ), Ok(_) ); diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 7c23f79bc11a3..2de8a592680f7 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -513,12 +513,11 @@ pub mod pallet { } #[derive(Clone, Copy, Debug, PartialEq, Eq)] - pub enum ExecContext { - /// A normal transaction execution, where the nonce has been pre-incremented. - Transaction, - - /// A dry run execution (through RPC), where the nonce has not been incremented. - DryRun { skip_transfer: bool }, + pub enum IncrementOnce { + /// Indicates that the nonce has not been incremented yet. + No, + /// Indicates that the nonce has already been incremented. + AlreadyIncremented, } /// A mapping from a contract's code hash to its code. @@ -766,7 +765,6 @@ pub mod pallet { gas_limit, DepositLimit::Balance(storage_deposit_limit), data, - ExecContext::Transaction, ); if let Ok(return_value) = &output.result { @@ -804,7 +802,7 @@ pub mod pallet { Code::Existing(code_hash), data, salt, - ExecContext::Transaction, + IncrementOnce::AlreadyIncremented, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -869,7 +867,7 @@ pub mod pallet { Code::Upload(code), data, salt, - ExecContext::Transaction, + IncrementOnce::AlreadyIncremented, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -1035,7 +1033,6 @@ where gas_limit: Weight, storage_deposit_limit: DepositLimit>, data: Vec, - exec_context: ExecContext, ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1053,7 +1050,7 @@ where &mut storage_meter, Self::convert_native_to_evm(value), data, - exec_context, + storage_deposit_limit.is_unchecked(), )?; storage_deposit = storage_meter .try_into_deposit(&origin, storage_deposit_limit.is_unchecked()) @@ -1084,7 +1081,7 @@ where code: Code, data: Vec, salt: Option<[u8; 32]>, - exec_context: ExecContext, + increment_once: IncrementOnce, ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1126,7 +1123,8 @@ where Self::convert_native_to_evm(value), data, salt.as_ref(), - exec_context, + unchecked_deposit_limit, + increment_once, ); storage_deposit = storage_meter .try_into_deposit(&instantiate_origin, unchecked_deposit_limit)? @@ -1175,9 +1173,6 @@ where DepositLimit::Unchecked }; - let exec_context = - ExecContext::DryRun { skip_transfer: storage_deposit_limit.is_unchecked() }; - if tx.nonce.is_none() { tx.nonce = Some(>::account_nonce(&origin).into()); } @@ -1239,7 +1234,6 @@ where gas_limit, storage_deposit_limit, input.clone(), - exec_context, ); let data = match result.result { @@ -1300,7 +1294,7 @@ where Code::Upload(code.to_vec()), data.to_vec(), None, - exec_context, + IncrementOnce::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 e59a1c3da62dc..9729b5a5fffdf 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, - ExecContext, ExecReturnValue, InstantiateReturnValue, OriginFor, Pallet, Weight, + ExecReturnValue, IncrementOnce, InstantiateReturnValue, OriginFor, Pallet, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::pallet_prelude::DispatchResultWithPostInfo; @@ -138,7 +138,7 @@ builder!( code: Code, data: Vec, salt: Option<[u8; 32]>, - exec_context: ExecContext, + increment_once: IncrementOnce, ) -> ContractResult>; /// Build the instantiate call and unwrap the result. @@ -166,7 +166,7 @@ builder!( code, data: vec![], salt: Some([0; 32]), - exec_context: crate::ExecContext::Transaction, + increment_once: IncrementOnce::AlreadyIncremented, } } ); @@ -202,7 +202,6 @@ builder!( gas_limit: Weight, storage_deposit_limit: DepositLimit>, data: Vec, - exec_context: ExecContext, ) -> ContractResult>; /// Build the call and unwrap the result. @@ -219,7 +218,6 @@ builder!( gas_limit: GAS_LIMIT, storage_deposit_limit: DepositLimit::Balance(deposit_limit::()), data: vec![], - exec_context: crate::ExecContext::Transaction, } } ); From 8c2d486a2c23c1be30ca343f2570dcba9ae42753 Mon Sep 17 00:00:00 2001 From: castillax Date: Wed, 14 May 2025 20:35:16 +0100 Subject: [PATCH 07/17] fix tests --- substrate/frame/revive/src/exec/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index 91b59aa4203ec..4d5d9d83d5079 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -2946,6 +2946,7 @@ fn block_hash_returns_proper_values() { &mut storage_meter, U256::zero(), vec![0], + false, ), Ok(_) ); From a69a1f2c006dd6ca87ae119fa5bc255df905ac9d Mon Sep 17 00:00:00 2001 From: castillax Date: Thu, 15 May 2025 14:52:05 +0100 Subject: [PATCH 08/17] Change enum to NonceAlreadyIncremented --- .../assets/asset-hub-westend/src/lib.rs | 4 ++-- substrate/bin/node/runtime/src/lib.rs | 4 ++-- substrate/frame/revive/src/call_builder.rs | 5 +++-- substrate/frame/revive/src/exec.rs | 20 +++++++++++------ substrate/frame/revive/src/exec/tests.rs | 22 +++++++++---------- substrate/frame/revive/src/lib.rs | 18 +++++++++------ .../frame/revive/src/test_utils/builder.rs | 6 ++--- 7 files changed, 45 insertions(+), 34 deletions(-) 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 c35a3bf57fee5..56ec3989f0948 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, IncrementOnce}; +use pallet_revive::{evm::runtime::EthExtra, AddressMapper, NonceAlreadyIncremented}; use pallet_xcm::EnsureXcm; use parachains_common::{ impls::DealWithFees, message_queue::*, AccountId, AssetIdForTrustBackedAssets, AuraId, Balance, @@ -2375,7 +2375,7 @@ impl_runtime_apis! { code, data, salt, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ) } diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 42655ceb05154..e57e3abab00e9 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, IncrementOnce}; +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; @@ -3453,7 +3453,7 @@ impl_runtime_apis! { code, data, salt, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ) } diff --git a/substrate/frame/revive/src/call_builder.rs b/substrate/frame/revive/src/call_builder.rs index 7847b843919de..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, IncrementOnce, 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,7 +271,7 @@ where Code::Upload(module.code), data, salt, - IncrementOnce::No, + NonceAlreadyIncremented::No, ); let address = outcome.result?.addr; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 5b0fdafb88577..7919f975c23e4 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, IncrementOnce, Pallet as Contracts, + Error, Event, ImmutableData, ImmutableDataOf, NonceAlreadyIncremented, Pallet as Contracts, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -614,7 +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], - increment_once: IncrementOnce, + nonce_already_incremented: NonceAlreadyIncremented, }, } @@ -808,7 +808,7 @@ where input_data: Vec, salt: Option<&[u8; 32]>, skip_transfer: bool, - increment_once: IncrementOnce, + nonce_already_incremented: NonceAlreadyIncremented, ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { @@ -816,7 +816,7 @@ where executable, salt, input_data: input_data.as_ref(), - increment_once, + nonce_already_incremented, }, Origin::from_account_id(origin), gas_meter, @@ -975,7 +975,13 @@ where (dest, contract, executable, delegated_call, ExportedFunction::Call) }, - FrameArgs::Instantiate { sender, executable, salt, input_data, increment_once } => { + 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 { @@ -987,7 +993,7 @@ where // 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!(increment_once, IncrementOnce::AlreadyIncremented) + matches!(nonce_already_incremented, NonceAlreadyIncremented::Yes) { account_nonce.saturating_sub(1u32.into()).saturated_into() } else { @@ -1670,7 +1676,7 @@ where executable, salt, input_data: input_data.as_ref(), - increment_once: IncrementOnce::No, + 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 4d5d9d83d5079..7558495568cdb 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -566,7 +566,7 @@ fn input_data_to_instantiate() { vec![1, 2, 3, 4], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ); assert_matches!(result, Ok(_)); }); @@ -1070,7 +1070,7 @@ fn refuse_instantiate_with_value_below_existential_deposit() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ), Err(_) ); @@ -1106,7 +1106,7 @@ fn instantiation_work_with_success_output() { vec![], Some(&[0 ;32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ), Ok((address, ref output)) if output.data == vec![80, 65, 83, 83] => address ); @@ -1153,7 +1153,7 @@ fn instantiation_fails_with_failing_output() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ), Ok((address, ref output)) if output.data == vec![70, 65, 73, 76] => address ); @@ -1317,7 +1317,7 @@ fn termination_from_instantiate_fails() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ), Err(ExecError { error: Error::::TerminatedInConstructor.into(), @@ -1446,7 +1446,7 @@ fn recursive_call_during_constructor_is_balance_transfer() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ); assert_matches!(result, Ok(_)); }); @@ -1808,7 +1808,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ) .ok(); assert_eq!(System::account_nonce(&ALICE), 0); @@ -1822,7 +1822,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 1); @@ -1835,7 +1835,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 2); @@ -1848,7 +1848,7 @@ fn nonce() { vec![], Some(&[0; 32]), false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, )); assert_eq!(System::account_nonce(&ALICE), 3); }); @@ -2835,7 +2835,7 @@ fn immutable_data_set_overrides() { vec![], None, false, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ) .unwrap() .0; diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 2de8a592680f7..74b6155472646 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -513,11 +513,15 @@ pub mod pallet { } #[derive(Clone, Copy, Debug, PartialEq, Eq)] - pub enum IncrementOnce { + 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. - AlreadyIncremented, + /// + /// This happens when the instantiation is triggered by a transaction. + Yes, } /// A mapping from a contract's code hash to its code. @@ -802,7 +806,7 @@ pub mod pallet { Code::Existing(code_hash), data, salt, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -867,7 +871,7 @@ pub mod pallet { Code::Upload(code), data, salt, - IncrementOnce::AlreadyIncremented, + NonceAlreadyIncremented::Yes, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -1081,7 +1085,7 @@ where code: Code, data: Vec, salt: Option<[u8; 32]>, - increment_once: IncrementOnce, + nonce_already_incremented: NonceAlreadyIncremented, ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1124,7 +1128,7 @@ where data, salt.as_ref(), unchecked_deposit_limit, - increment_once, + nonce_already_incremented, ); storage_deposit = storage_meter .try_into_deposit(&instantiate_origin, unchecked_deposit_limit)? @@ -1294,7 +1298,7 @@ where Code::Upload(code.to_vec()), data.to_vec(), None, - IncrementOnce::No, + 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 9729b5a5fffdf..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, IncrementOnce, InstantiateReturnValue, OriginFor, Pallet, Weight, + ExecReturnValue, InstantiateReturnValue, NonceAlreadyIncremented, OriginFor, Pallet, Weight, }; use alloc::{vec, vec::Vec}; use frame_support::pallet_prelude::DispatchResultWithPostInfo; @@ -138,7 +138,7 @@ builder!( code: Code, data: Vec, salt: Option<[u8; 32]>, - increment_once: IncrementOnce, + nonce_already_incremented: NonceAlreadyIncremented, ) -> ContractResult>; /// Build the instantiate call and unwrap the result. @@ -166,7 +166,7 @@ builder!( code, data: vec![], salt: Some([0; 32]), - increment_once: IncrementOnce::AlreadyIncremented, + nonce_already_incremented: NonceAlreadyIncremented::Yes, } } ); From 4d21dc33d019fb19375be7bed12e4190d6a4da44 Mon Sep 17 00:00:00 2001 From: castillax Date: Thu, 15 May 2025 17:29:35 +0100 Subject: [PATCH 09/17] Runtime bare_instantiates are dry runs --- .../parachains/runtimes/assets/asset-hub-westend/src/lib.rs | 2 +- prdoc/pr_8504.prdoc | 2 +- substrate/bin/node/runtime/src/lib.rs | 2 +- substrate/frame/revive/src/exec.rs | 4 +--- 4 files changed, 4 insertions(+), 6 deletions(-) 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 6d2273e1a0f00..3c3d8aac35eb2 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs @@ -2375,7 +2375,7 @@ impl_runtime_apis! { code, data, salt, - NonceAlreadyIncremented::Yes, + NonceAlreadyIncremented::No, ) } diff --git a/prdoc/pr_8504.prdoc b/prdoc/pr_8504.prdoc index 465e9d76bed40..762d407e8582f 100644 --- a/prdoc/pr_8504.prdoc +++ b/prdoc/pr_8504.prdoc @@ -71,4 +71,4 @@ crates: - name: asset-hub-westend-runtime bump: patch - name: pallet-revive - bump: patch + bump: major diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 6f079c72b3858..5d2dca4a0774d 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -3453,7 +3453,7 @@ impl_runtime_apis! { code, data, salt, - NonceAlreadyIncremented::Yes, + NonceAlreadyIncremented::No, ) } diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 7919f975c23e4..db37589c7680a 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -992,9 +992,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 && - matches!(nonce_already_incremented, NonceAlreadyIncremented::Yes) - { + if matches!(nonce_already_incremented, NonceAlreadyIncremented::Yes) { account_nonce.saturating_sub(1u32.into()).saturated_into() } else { account_nonce.saturated_into() From 5dc70928432cda7c89018cb32e3bf8c47632f9ca Mon Sep 17 00:00:00 2001 From: castillax Date: Fri, 16 May 2025 10:40:59 +0100 Subject: [PATCH 10/17] Update .github/workflows/tests-evm.yml Co-authored-by: PG Herveou --- .github/workflows/tests-evm.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/tests-evm.yml b/.github/workflows/tests-evm.yml index de1a50b236855..db8f0e558b623 100644 --- a/.github/workflows/tests-evm.yml +++ b/.github/workflows/tests-evm.yml @@ -80,7 +80,6 @@ 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() From b215946a54278c17276dace6d143e3b7db0d0670 Mon Sep 17 00:00:00 2001 From: castillax Date: Fri, 16 May 2025 11:37:39 +0100 Subject: [PATCH 11/17] remove unused arg --- substrate/frame/revive/src/exec.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index db37589c7680a..aa4c7d3ce1418 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -877,7 +877,6 @@ where storage_meter, BalanceOf::::max_value(), false, - true, )? else { return Ok(None); @@ -911,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 } => { @@ -1068,7 +1066,6 @@ where nested_storage, deposit_limit, read_only, - false, )? { self.frames.try_push(frame).map_err(|_| Error::::MaxCallDepthReached)?; Ok(Some(executable)) From be30983d1f3e37c31102b3135e4bb76f66c7fb02 Mon Sep 17 00:00:00 2001 From: castillax Date: Mon, 19 May 2025 15:00:02 +0100 Subject: [PATCH 12/17] Add test for nonce_already_incremented --- substrate/frame/revive/src/exec/tests.rs | 72 ++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index 7558495568cdb..ba3a67fe78f3a 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -1854,6 +1854,78 @@ fn nonce() { }); } +#[test] +fn nonce_already_incremented_works() { + let simple_constructor = MockLoader::insert(Constructor, |_, _| exec_success()); + + ExtBuilder::default() + .with_code_hashes(MockLoader::code_hashes()) + .build() + .execute_with(|| { + let min_balance = ::Currency::minimum_balance(); + + // Set up initial state + set_balance(&ALICE, min_balance * 1000); + + frame_system::Account::::mutate(&ALICE, |account| { + account.nonce = 5; + }); + + let origin = Origin::from_account_id(ALICE); + let mut storage_meter = + storage::meter::Meter::new(&origin, deposit_limit::(), min_balance).unwrap(); + + let mut gas_meter_1 = GasMeter::::new(GAS_LIMIT); + let executable_1 = + MockExecutable::from_storage(simple_constructor, &mut gas_meter_1).unwrap(); + + let (address_yes, _) = MockStack::run_instantiate( + ALICE, + executable_1, + &mut gas_meter_1, + &mut storage_meter, + min_balance.into(), + vec![], + None, + false, + NonceAlreadyIncremented::Yes, + ) + .unwrap(); + + frame_system::Account::::mutate(&ALICE, |account| { + account.nonce = 5; + }); + + let mut gas_meter_2 = GasMeter::::new(GAS_LIMIT); + let executable_2 = + MockExecutable::from_storage(simple_constructor, &mut gas_meter_2).unwrap(); + + let (address_no, _) = MockStack::run_instantiate( + ALICE, + executable_2, + &mut gas_meter_2, + &mut storage_meter, + min_balance.into(), + vec![], + None, + false, + NonceAlreadyIncremented::No, + ) + .unwrap(); + + assert_ne!(address_yes, address_no); + + let deployer = ::AddressMapper::to_address(&ALICE); + let expected_address_yes = address::create1(&deployer, 4); + let expected_address_no = address::create1(&deployer, 5); + + assert_eq!(address_yes, expected_address_yes); + assert_eq!(address_no, expected_address_no); + + assert_eq!(System::account_nonce(&ALICE), 6); + }); +} + #[test] fn set_storage_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { From 6c307600030b47e142f0f64dfff52d9cc8f99df7 Mon Sep 17 00:00:00 2001 From: castillax Date: Tue, 20 May 2025 09:19:58 +0100 Subject: [PATCH 13/17] Fix comments on tests --- substrate/frame/revive/src/tests.rs | 58 +++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 69f5ed60d51dc..13543807cce80 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4565,3 +4565,61 @@ 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 = Contracts::bare_instantiate( + RuntimeOrigin::signed(ALICE), + 0, + GAS_LIMIT, + DepositLimit::Balance(deposit_limit::()), + Code::Upload(wasm.clone()), + vec![], + None, // Use create1 + crate::NonceAlreadyIncremented::No, + ); + + 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 = Contracts::bare_instantiate( + RuntimeOrigin::signed(ALICE), + 0, + GAS_LIMIT, + DepositLimit::Balance(deposit_limit::()), + Code::Upload(wasm.clone()), + vec![], + None, + crate::NonceAlreadyIncremented::Yes, + ); + + 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); + }); +} From 998f58ec25c8c1c1e7f296a11b48e4e67e13c0eb Mon Sep 17 00:00:00 2001 From: castillax Date: Tue, 20 May 2025 09:21:11 +0100 Subject: [PATCH 14/17] remove the other test --- substrate/frame/revive/src/exec/tests.rs | 72 ------------------------ 1 file changed, 72 deletions(-) diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index ba3a67fe78f3a..7558495568cdb 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -1854,78 +1854,6 @@ fn nonce() { }); } -#[test] -fn nonce_already_incremented_works() { - let simple_constructor = MockLoader::insert(Constructor, |_, _| exec_success()); - - ExtBuilder::default() - .with_code_hashes(MockLoader::code_hashes()) - .build() - .execute_with(|| { - let min_balance = ::Currency::minimum_balance(); - - // Set up initial state - set_balance(&ALICE, min_balance * 1000); - - frame_system::Account::::mutate(&ALICE, |account| { - account.nonce = 5; - }); - - let origin = Origin::from_account_id(ALICE); - let mut storage_meter = - storage::meter::Meter::new(&origin, deposit_limit::(), min_balance).unwrap(); - - let mut gas_meter_1 = GasMeter::::new(GAS_LIMIT); - let executable_1 = - MockExecutable::from_storage(simple_constructor, &mut gas_meter_1).unwrap(); - - let (address_yes, _) = MockStack::run_instantiate( - ALICE, - executable_1, - &mut gas_meter_1, - &mut storage_meter, - min_balance.into(), - vec![], - None, - false, - NonceAlreadyIncremented::Yes, - ) - .unwrap(); - - frame_system::Account::::mutate(&ALICE, |account| { - account.nonce = 5; - }); - - let mut gas_meter_2 = GasMeter::::new(GAS_LIMIT); - let executable_2 = - MockExecutable::from_storage(simple_constructor, &mut gas_meter_2).unwrap(); - - let (address_no, _) = MockStack::run_instantiate( - ALICE, - executable_2, - &mut gas_meter_2, - &mut storage_meter, - min_balance.into(), - vec![], - None, - false, - NonceAlreadyIncremented::No, - ) - .unwrap(); - - assert_ne!(address_yes, address_no); - - let deployer = ::AddressMapper::to_address(&ALICE); - let expected_address_yes = address::create1(&deployer, 4); - let expected_address_no = address::create1(&deployer, 5); - - assert_eq!(address_yes, expected_address_yes); - assert_eq!(address_no, expected_address_no); - - assert_eq!(System::account_nonce(&ALICE), 6); - }); -} - #[test] fn set_storage_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { From 846b885ddd795646ad3bd8a42ea9bca7e7151789 Mon Sep 17 00:00:00 2001 From: castillax Date: Tue, 20 May 2025 10:24:03 +0100 Subject: [PATCH 15/17] Use builder pattern --- substrate/frame/revive/src/tests.rs | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 13543807cce80..4f3c6b00fac54 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4580,16 +4580,10 @@ fn nonce_incremented_dry_run_vs_execute() { }); // stimulate a dry run - let dry_run_result = Contracts::bare_instantiate( - RuntimeOrigin::signed(ALICE), - 0, - GAS_LIMIT, - DepositLimit::Balance(deposit_limit::()), - Code::Upload(wasm.clone()), - vec![], - None, // Use create1 - crate::NonceAlreadyIncremented::No, - ); + let dry_run_result = builder::bare_instantiate(Code::Upload(wasm.clone())) + .salt(None) + .nonce_already_incremented(crate::NonceAlreadyIncremented::No) + .build(); let dry_run_addr = dry_run_result.result.unwrap().addr; @@ -4604,16 +4598,7 @@ fn nonce_incremented_dry_run_vs_execute() { }); // stimulate an actual execution - let exec_result = Contracts::bare_instantiate( - RuntimeOrigin::signed(ALICE), - 0, - GAS_LIMIT, - DepositLimit::Balance(deposit_limit::()), - Code::Upload(wasm.clone()), - vec![], - None, - crate::NonceAlreadyIncremented::Yes, - ); + let exec_result = builder::bare_instantiate(Code::Upload(wasm.clone())).salt(None).build(); let exec_addr = exec_result.result.unwrap().addr; From f9291742e8d3dc5d93b44ca786d155e8869ee1cc Mon Sep 17 00:00:00 2001 From: castillax Date: Tue, 20 May 2025 11:21:57 +0100 Subject: [PATCH 16/17] salt defaults to None, no need to set it --- substrate/frame/revive/src/tests.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 4f3c6b00fac54..f815082e0b445 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4581,7 +4581,6 @@ fn nonce_incremented_dry_run_vs_execute() { // stimulate a dry run let dry_run_result = builder::bare_instantiate(Code::Upload(wasm.clone())) - .salt(None) .nonce_already_incremented(crate::NonceAlreadyIncremented::No) .build(); @@ -4598,7 +4597,7 @@ fn nonce_incremented_dry_run_vs_execute() { }); // stimulate an actual execution - let exec_result = builder::bare_instantiate(Code::Upload(wasm.clone())).salt(None).build(); + let exec_result = builder::bare_instantiate(Code::Upload(wasm.clone())).build(); let exec_addr = exec_result.result.unwrap().addr; From 7f124d2ee2b7db35ab5796c55dce5794ac9918d6 Mon Sep 17 00:00:00 2001 From: castillax Date: Tue, 20 May 2025 14:40:04 +0100 Subject: [PATCH 17/17] setting salt to Some() will lead to create2 being used --- substrate/frame/revive/src/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index f815082e0b445..12c7a6becab03 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4582,6 +4582,7 @@ fn nonce_incremented_dry_run_vs_execute() { // 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; @@ -4597,7 +4598,7 @@ fn nonce_incremented_dry_run_vs_execute() { }); // stimulate an actual execution - let exec_result = builder::bare_instantiate(Code::Upload(wasm.clone())).build(); + let exec_result = builder::bare_instantiate(Code::Upload(wasm.clone())).salt(None).build(); let exec_addr = exec_result.result.unwrap().addr;