Skip to content

Commit

Permalink
Skip balance and gas price checks on non-transactional calls (#57)
Browse files Browse the repository at this point in the history
* Skip balance and gas price checks on non-transactional calls

(cherry picked from commit b7081b4)

* Update comment

(cherry picked from commit caf6566)

* Ensure account nonce first

(cherry picked from commit 81abadd)
  • Loading branch information
tgmichel authored Apr 18, 2022
1 parent 8ccf41f commit 79ed3f2
Show file tree
Hide file tree
Showing 9 changed files with 202 additions and 18 deletions.
12 changes: 12 additions & 0 deletions client/rpc/src/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,12 @@ fn fee_details(
match (request_gas_price, request_max_fee, request_priority) {
(gas_price, None, None) => {
// Legacy request, all default to gas price.
// A zero-set gas price is None.
let gas_price = if gas_price.unwrap_or_default().is_zero() {
None
} else {
gas_price
};
Ok(FeeDetails {
gas_price,
max_fee_per_gas: gas_price,
Expand All @@ -476,6 +482,12 @@ fn fee_details(
}
(_, max_fee, max_priority) => {
// eip-1559
// A zero-set max fee is None.
let max_fee = if max_fee.unwrap_or_default().is_zero() {
None
} else {
max_fee
};
// Ensure `max_priority_fee_per_gas` is less or equal to `max_fee_per_gas`.
if let Some(max_priority) = max_priority {
let max_fee = max_fee.unwrap_or_default();
Expand Down
3 changes: 3 additions & 0 deletions frame/ethereum/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,7 @@ impl<T: Config> Pallet<T> {
}
};

let is_transactional = true;
match action {
ethereum::TransactionAction::Call(target) => {
let res = T::Runner::call(
Expand All @@ -808,6 +809,7 @@ impl<T: Config> Pallet<T> {
max_priority_fee_per_gas,
nonce,
access_list,
is_transactional,
config.as_ref().unwrap_or(T::config()),
)
.map_err(Into::into)?;
Expand All @@ -824,6 +826,7 @@ impl<T: Config> Pallet<T> {
max_priority_fee_per_gas,
nonce,
access_list,
is_transactional,
config.as_ref().unwrap_or(T::config()),
)
.map_err(Into::into)?;
Expand Down
4 changes: 4 additions & 0 deletions frame/evm/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ benchmarks! {

let value = U256::default();
let gas_limit_create: u64 = 1_250_000 * 1_000_000_000;
let is_transactional = true;
let create_runner_results = T::Runner::create(
caller,
contract_bytecode,
Expand All @@ -85,6 +86,7 @@ benchmarks! {
None,
Some(nonce_as_u256),
Vec::new(),
is_transactional,
T::config(),
);
assert_eq!(create_runner_results.is_ok(), true, "create() failed");
Expand All @@ -106,6 +108,7 @@ benchmarks! {
nonce = nonce + 1;
let nonce_as_u256: U256 = nonce.into();

let is_transactional = true;
let call_runner_results = T::Runner::call(
caller,
contract_address,
Expand All @@ -116,6 +119,7 @@ benchmarks! {
None,
Some(nonce_as_u256),
Vec::new(),
is_transactional,
T::config(),
);
assert_eq!(call_runner_results.is_ok(), true, "call() failed");
Expand Down
9 changes: 9 additions & 0 deletions frame/evm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
T::CallOrigin::ensure_address_origin(&source, origin)?;

let is_transactional = true;
let info = T::Runner::call(
source,
target,
Expand All @@ -202,6 +203,7 @@ pub mod pallet {
max_priority_fee_per_gas,
nonce,
access_list,
is_transactional,
T::config(),
)?;

Expand Down Expand Up @@ -238,6 +240,7 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
T::CallOrigin::ensure_address_origin(&source, origin)?;

let is_transactional = true;
let info = T::Runner::create(
source,
init,
Expand All @@ -247,6 +250,7 @@ pub mod pallet {
max_priority_fee_per_gas,
nonce,
access_list,
is_transactional,
T::config(),
)?;

Expand Down Expand Up @@ -291,6 +295,7 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
T::CallOrigin::ensure_address_origin(&source, origin)?;

let is_transactional = true;
let info = T::Runner::create2(
source,
init,
Expand All @@ -301,6 +306,7 @@ pub mod pallet {
max_priority_fee_per_gas,
nonce,
access_list,
is_transactional,
T::config(),
)?;

Expand Down Expand Up @@ -734,6 +740,9 @@ where
type LiquidityInfo = Option<NegativeImbalanceOf<C, T>>;

fn withdraw_fee(who: &H160, fee: U256) -> Result<Self::LiquidityInfo, Error<T>> {
if fee.is_zero() {
return Ok(None);
}
let account_id = T::AddressMapping::into_account_id(*who);
let imbalance = C::withdraw(
&account_id,
Expand Down
3 changes: 3 additions & 0 deletions frame/evm/src/runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pub trait Runner<T: Config> {
max_priority_fee_per_gas: Option<U256>,
nonce: Option<U256>,
access_list: Vec<(H160, Vec<H256>)>,
is_transactional: bool,
config: &evm::Config,
) -> Result<CallInfo, Self::Error>;

Expand All @@ -47,6 +48,7 @@ pub trait Runner<T: Config> {
max_priority_fee_per_gas: Option<U256>,
nonce: Option<U256>,
access_list: Vec<(H160, Vec<H256>)>,
is_transactional: bool,
config: &evm::Config,
) -> Result<CreateInfo, Self::Error>;

Expand All @@ -60,6 +62,7 @@ pub trait Runner<T: Config> {
max_priority_fee_per_gas: Option<U256>,
nonce: Option<U256>,
access_list: Vec<(H160, Vec<H256>)>,
is_transactional: bool,
config: &evm::Config,
) -> Result<CreateInfo, Self::Error>;
}
54 changes: 36 additions & 18 deletions frame/evm/src/runner/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ impl<T: Config> Runner<T> {
nonce: Option<U256>,
config: &'config evm::Config,
precompiles: &'precompiles T::PrecompilesType,
is_transactional: bool,
f: F,
) -> Result<ExecutionInfo<R>, Error<T>>
where
Expand All @@ -65,21 +66,24 @@ impl<T: Config> Runner<T> {
) -> (ExitReason, R),
{
let base_fee = T::FeeCalculator::min_gas_price();

let max_fee_per_gas = match (max_fee_per_gas, max_priority_fee_per_gas) {
(Some(max_fee_per_gas), Some(max_priority_fee_per_gas)) => {
ensure!(max_fee_per_gas >= base_fee, Error::<T>::GasPriceTooLow);
ensure!(max_fee_per_gas >= max_priority_fee_per_gas, Error::<T>::GasPriceTooLow);
max_fee_per_gas
},
(Some(max_fee_per_gas), None) => {
let max_fee_per_gas = match (max_fee_per_gas, is_transactional) {
(Some(max_fee_per_gas), _) => {
ensure!(max_fee_per_gas >= base_fee, Error::<T>::GasPriceTooLow);
max_fee_per_gas
},
// Gas price check is skipped when performing a gas estimation.
_ => Default::default()
}
// Gas price check is skipped for non-transactional calls that don't
// define a `max_fee_per_gas` input.
(None, false) => Default::default(),
_ => return Err(Error::<T>::GasPriceTooLow),
};

if let Some(max_priority_fee) = max_priority_fee_per_gas {
ensure!(
max_fee_per_gas >= max_priority_fee,
Error::<T>::GasPriceTooLow
);
}

// After eip-1559 we make sure the account can pay both the evm execution and priority fees.
let total_fee = max_fee_per_gas
.checked_mul(U256::from(gas_limit))
Expand All @@ -89,15 +93,22 @@ impl<T: Config> Runner<T> {
.checked_add(total_fee)
.ok_or(Error::<T>::PaymentOverflow)?;
let source_account = Pallet::<T>::account_basic(&source);
ensure!(
source_account.balance >= total_payment,
Error::<T>::BalanceLow
);
// Account balance check is skipped if fee is Zero.
// This case is previously verified to only happen on either:
// - Non-transactional calls.
// - BaseFee is configured to be Zero.
if total_fee > U256::zero() {
ensure!(
source_account.balance >= total_payment,
Error::<T>::BalanceLow
);
}

if let Some(nonce) = nonce {
ensure!(source_account.nonce == nonce, Error::<T>::InvalidNonce);
}
// Deduct fee from the `source` account.

// Deduct fee from the `source` account. Returns `None` if `total_fee` is Zero.
let fee = T::OnChargeTransaction::withdraw_fee(&source, total_fee)?;

// Execute the EVM call.
Expand Down Expand Up @@ -131,12 +142,13 @@ impl<T: Config> Runner<T> {
};
log::debug!(
target: "evm",
"Execution {:?} [source: {:?}, value: {}, gas_limit: {}, actual_fee: {}]",
"Execution {:?} [source: {:?}, value: {}, gas_limit: {}, actual_fee: {}, is_transactional: {}]",
reason,
source,
value,
gas_limit,
actual_fee
actual_fee,
is_transactional
);
// The difference between initially withdrawn and the actual cost is refunded.
//
Expand Down Expand Up @@ -214,6 +226,7 @@ impl<T: Config> RunnerT<T> for Runner<T> {
max_priority_fee_per_gas: Option<U256>,
nonce: Option<U256>,
access_list: Vec<(H160, Vec<H256>)>,
is_transactional: bool,
config: &evm::Config,
) -> Result<CallInfo, Self::Error> {
let precompiles = T::PrecompilesValue::get();
Expand All @@ -226,6 +239,7 @@ impl<T: Config> RunnerT<T> for Runner<T> {
nonce,
config,
&precompiles,
is_transactional,
|executor| executor.transact_call(source, target, value, input, gas_limit, access_list),
)
}
Expand All @@ -239,6 +253,7 @@ impl<T: Config> RunnerT<T> for Runner<T> {
max_priority_fee_per_gas: Option<U256>,
nonce: Option<U256>,
access_list: Vec<(H160, Vec<H256>)>,
is_transactional: bool,
config: &evm::Config,
) -> Result<CreateInfo, Self::Error> {
let precompiles = T::PrecompilesValue::get();
Expand All @@ -251,6 +266,7 @@ impl<T: Config> RunnerT<T> for Runner<T> {
nonce,
config,
&precompiles,
is_transactional,
|executor| {
let address = executor.create_address(evm::CreateScheme::Legacy { caller: source });
(
Expand All @@ -271,6 +287,7 @@ impl<T: Config> RunnerT<T> for Runner<T> {
max_priority_fee_per_gas: Option<U256>,
nonce: Option<U256>,
access_list: Vec<(H160, Vec<H256>)>,
is_transactional: bool,
config: &evm::Config,
) -> Result<CreateInfo, Self::Error> {
let precompiles = T::PrecompilesValue::get();
Expand All @@ -284,6 +301,7 @@ impl<T: Config> RunnerT<T> for Runner<T> {
nonce,
config,
&precompiles,
is_transactional,
|executor| {
let address = executor.create_address(evm::CreateScheme::Create2 {
caller: source,
Expand Down
Loading

0 comments on commit 79ed3f2

Please sign in to comment.