Skip to content

Commit

Permalink
fix nonce (#2588)
Browse files Browse the repository at this point in the history 
* nonce issue test

* EVM::create does not discard nonce so we don't increment the nonce on failure

* fix nonce

---------

Co-authored-by: Ermal Kaleci <[email protected]>
  • Loading branch information
@xlc @ermalkaleci
xlc and ermalkaleci authored Aug 20, 2023
1 parent c55160a commit d8e0754
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 73 deletions.
29 changes: 10 additions & 19 deletions modules/evm/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -636,10 +636,11 @@ pub mod module {
T::config(),
);

Self::inc_nonce_if_needed(&source, &outcome);

match outcome {
Err(e) => {
// EVM state changes reverted, increase nonce by ourselves
Self::inc_nonce(&source);

Pallet::<T>::deposit_event(Event::<T>::ExecutedFailed {
from: source,
contract: target,
Expand Down Expand Up @@ -821,10 +822,11 @@ pub mod module {
T::config(),
);

Self::inc_nonce_if_needed(&source, &outcome);

match outcome {
Err(e) => {
// EVM state changes reverted, increase nonce by ourselves
Self::inc_nonce(&source);

Pallet::<T>::deposit_event(Event::<T>::CreatedFailed {
from: source,
contract: H160::default(),
Expand Down Expand Up @@ -901,10 +903,11 @@ pub mod module {
T::config(),
);

Self::inc_nonce_if_needed(&source, &outcome);

match outcome {
Err(e) => {
// EVM state changes reverted, increase nonce by ourselves
Self::inc_nonce(&source);

Pallet::<T>::deposit_event(Event::<T>::CreatedFailed {
from: source,
contract: H160::default(),
Expand Down Expand Up @@ -1887,19 +1890,7 @@ impl<T: Config> Pallet<T> {
Ok(())
}

fn inc_nonce_if_needed<Output>(origin: &H160, outcome: &Result<ExecutionInfo<Output>, DispatchError>) {
if matches!(
outcome,
Ok(ExecutionInfo {
exit_reason: ExitReason::Succeed(_),
..
})
) {
// nonce increased by EVM
return;
}

// EVM changes reverted, increase nonce by ourselves
fn inc_nonce(origin: &H160) {
Accounts::<T>::mutate(origin, |account| {
if let Some(info) = account.as_mut() {
info.nonce = info.nonce.saturating_add(T::Nonce::one());
Expand Down
15 changes: 3 additions & 12 deletions modules/evm/src/runner/stack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ use crate::{
state::{Accessed, CustomStackState, StackExecutor, StackState as StackStateT, StackSubstateMetadata},
Runner as RunnerT, RunnerExtended,
},
AccountInfo, AccountStorages, Accounts, BalanceOf, CallInfo, Config, CreateInfo, Error, ExecutionInfo, One, Pallet,
STORAGE_SIZE,
AccountStorages, BalanceOf, CallInfo, Config, CreateInfo, Error, ExecutionInfo, Pallet, STORAGE_SIZE,
};
use frame_support::{
dispatch::DispatchError,
Expand Down Expand Up @@ -657,7 +656,7 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity,

#[cfg(feature = "evm-tests")]
fn exists(&self, address: H160) -> bool {
Accounts::<T>::contains_key(&address) || self.substate.is_account_dirty(address)
crate::Accounts::<T>::contains_key(&address) || self.substate.is_account_dirty(address)
}

#[cfg(not(feature = "evm-tests"))]
Expand Down Expand Up @@ -729,15 +728,7 @@ impl<'vicinity, 'config, T: Config> StackStateT<'config> for SubstrateStackState
}

fn inc_nonce(&mut self, address: H160) {
Accounts::<T>::mutate(address, |maybe_account| {
if let Some(account) = maybe_account.as_mut() {
account.nonce += One::one()
} else {
let mut account_info = <AccountInfo<T::Nonce>>::new(Default::default(), None);
account_info.nonce += One::one();
*maybe_account = Some(account_info);
}
});
Pallet::<T>::inc_nonce(&address);
}

fn set_storage(&mut self, address: H160, index: H256, value: H256) {
Expand Down
8 changes: 4 additions & 4 deletions modules/evm/src/runner/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,8 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> StackExecu
gas_limit: u64,
access_list: Vec<(H160, Vec<H256>)>,
) -> (ExitReason, Vec<u8>) {
self.state.inc_nonce(caller);

event!(TransactCall {
caller,
address,
Expand All @@ -660,8 +662,6 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> StackExecu
self.initialize_with_access_list(access_list);
}

self.state.inc_nonce(caller);

let context = Context {
caller,
address,
Expand Down Expand Up @@ -815,6 +815,8 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> StackExecu
return Capture::Exit((ExitError::OutOfFund.into(), None, Vec::new()));
}

self.state.inc_nonce(caller);

let after_gas = if take_l64 && self.config.call_l64_after_gas {
if self.config.estimate {
let initial_after_gas = self.state.metadata().gasometer.gas();
Expand All @@ -833,8 +835,6 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> StackExecu
let gas_limit = min(after_gas, target_gas);
try_or_fail!(self.state.metadata_mut().gasometer.record_cost(gas_limit));

self.state.inc_nonce(caller);

self.enter_substate(gas_limit, false);

{
Expand Down
66 changes: 28 additions & 38 deletions modules/evm/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,44 +34,6 @@ use sp_core::{
use sp_runtime::{traits::BadOrigin, AccountId32};
use std::str::FromStr;

#[test]
fn inc_nonce_if_needed() {
new_test_ext().execute_with(|| {
assert_eq!(EVM::account_basic(&alice()).nonce, U256::from(1));

let mut call_info = CallInfo {
exit_reason: ExitReason::Succeed(ExitSucceed::Returned),
value: vec![],
used_gas: Default::default(),
used_storage: 0,
logs: vec![],
};

// succeed call won't inc nonce
Pallet::<Runtime>::inc_nonce_if_needed(&alice(), &Ok(call_info.clone()));
assert_eq!(EVM::account_basic(&alice()).nonce, U256::from(1));

call_info.exit_reason = ExitReason::Revert(ExitRevert::Reverted);
// revert call will inc nonce
Pallet::<Runtime>::inc_nonce_if_needed(&alice(), &Ok(call_info.clone()));
assert_eq!(EVM::account_basic(&alice()).nonce, U256::from(2));

call_info.exit_reason = ExitReason::Fatal(ExitFatal::NotSupported);
// fatal call will inc nonce
Pallet::<Runtime>::inc_nonce_if_needed(&alice(), &Ok(call_info.clone()));
assert_eq!(EVM::account_basic(&alice()).nonce, U256::from(3));

call_info.exit_reason = ExitReason::Error(ExitError::OutOfGas);
// error call will inc nonce
Pallet::<Runtime>::inc_nonce_if_needed(&alice(), &Ok(call_info.clone()));
assert_eq!(EVM::account_basic(&alice()).nonce, U256::from(4));

// dispatch error will inc nonce
Pallet::<Runtime>::inc_nonce_if_needed::<H160>(&alice(), &Err(Error::<Runtime>::InvalidDecimals.into()));
assert_eq!(EVM::account_basic(&alice()).nonce, U256::from(5));
});
}

#[test]
fn fail_call_return_ok_and_inc_nonce() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -109,6 +71,34 @@ fn fail_call_return_ok_and_inc_nonce() {
});
}

#[test]
fn inc_nonce_with_revert() {
// pragma solidity ^0.5.0;
//
// contract Foo {
// constructor() public {
// require(false, "error message");
// }
// }
let contract = from_hex(
"0x6080604052348015600f57600080fd5b5060006083576040517f08c379a000000000000000000000000000000000000000000000000000000000815260040180806020018281038252600d8152602001807f6572726f72206d6573736167650000000000000000000000000000000000000081525060200191505060405180910390fd5b603e8060906000396000f3fe6080604052600080fdfea265627a7a723158204741083d83bf4e3ee8099dd0b3471c81061237c2e8eccfcb513dfa4c04634b5b64736f6c63430005110032").unwrap();

new_test_ext().execute_with(|| {
let alice = alice();
let account = MockAddressMapping::get_account_id(&alice);
let origin = RuntimeOrigin::signed(account);

// alice starts with nonce 1
assert_eq!(EVM::account_basic(&alice).nonce, U256::from(1));

// revert call
assert_ok!(EVM::create(origin.clone(), contract, 0, 1000000, 0, vec![]));

// nonce inc by 1
assert_eq!(EVM::account_basic(&alice).nonce, U256::from(2));
});
}

#[test]
fn should_calculate_contract_address() {
new_test_ext().execute_with(|| {
Expand Down

0 comments on commit d8e0754

Please sign in to comment.