Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions frame/ethereum/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ impl<T: Config> Pallet<T> {
let (base_fee, _) = T::FeeCalculator::min_gas_price();
let (who, _) = pallet_evm::Pallet::<T>::account_basic(&origin);

let _ = CheckEvmTransaction::<InvalidTransactionWrapper>::new(
let check_transaction = CheckEvmTransaction::<InvalidTransactionWrapper>::new(
CheckEvmTransactionConfig {
evm_config: T::config(),
block_gas_limit: T::BlockGasLimit::get(),
Expand All @@ -536,12 +536,18 @@ impl<T: Config> Pallet<T> {
transaction_data.clone().into(),
weight_limit,
proof_size_base_cost,
)
.validate_in_pool_for(&who)
.and_then(|v| v.with_chain_id())
.and_then(|v| v.with_base_fee())
.and_then(|v| v.with_balance_for(&who))
.map_err(|e| e.0)?;
);
check_transaction
.validate_in_pool_for(&who)
.and_then(|v| v.with_chain_id())
.and_then(|v| v.with_base_fee())
.and_then(|v| v.with_balance_for(&who))
.map_err(|e| e.0)?;

use pallet_evm::OnChargeEVMTransaction;
let max_withdraw = check_transaction.max_withdraw_amount().map_err(|e| e.0)?;
<T as pallet_evm::Config>::OnChargeTransaction::can_withdraw(&origin, max_withdraw)
.map_err(|_| InvalidTransaction::Payment)?;

// EIP-3607: https://eips.ethereum.org/EIPS/eip-3607
// Do not allow transactions for which `tx.sender` has any code deployed.
Expand Down Expand Up @@ -893,7 +899,7 @@ impl<T: Config> Pallet<T> {
let (base_fee, _) = T::FeeCalculator::min_gas_price();
let (who, _) = pallet_evm::Pallet::<T>::account_basic(&origin);

let _ = CheckEvmTransaction::<InvalidTransactionWrapper>::new(
let check_transaction = CheckEvmTransaction::<InvalidTransactionWrapper>::new(
CheckEvmTransactionConfig {
evm_config: T::config(),
block_gas_limit: T::BlockGasLimit::get(),
Expand All @@ -904,12 +910,18 @@ impl<T: Config> Pallet<T> {
transaction_data.into(),
weight_limit,
proof_size_base_cost,
)
.validate_in_block_for(&who)
.and_then(|v| v.with_chain_id())
.and_then(|v| v.with_base_fee())
.and_then(|v| v.with_balance_for(&who))
.map_err(|e| TransactionValidityError::Invalid(e.0))?;
);
check_transaction
.validate_in_block_for(&who)
.and_then(|v| v.with_chain_id())
.and_then(|v| v.with_base_fee())
.and_then(|v| v.with_balance_for(&who))
.map_err(|e| TransactionValidityError::Invalid(e.0))?;

use pallet_evm::OnChargeEVMTransaction;
let max_withdraw = check_transaction.max_withdraw_amount().map_err(|e| e.0)?;
<T as pallet_evm::Config>::OnChargeTransaction::can_withdraw(&origin, max_withdraw)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What additional validation does this offer compared to the with_balance_for mentioned above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm rechecking this change we have at Moonbeam's frontier fork to see if the issue was fixed with paritytech/polkadot-sdk#2292. Basically it meant to make a consistent check for the withdraw amount to prevent a corner case where the TX would be included in the pool even if it did not have enough balance to pay the fees.

.map_err(|_| InvalidTransaction::Payment)?;

Ok(())
}
Expand Down
30 changes: 30 additions & 0 deletions frame/evm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ use impl_trait_for_tuples::impl_for_tuples;
use scale_codec::{Decode, Encode, MaxEncodedLen};
use scale_info::TypeInfo;
// Substrate
use frame_support::traits::tokens::WithdrawConsequence;
use frame_support::{
dispatch::{DispatchResultWithPostInfo, Pays, PostDispatchInfo},
storage::{child::KillStorageResult, KeyPrefixIterator},
Expand Down Expand Up @@ -1043,6 +1044,8 @@ pub trait OnChargeEVMTransaction<T: Config> {
/// need to be secured.
fn withdraw_fee(who: &H160, fee: U256) -> Result<Self::LiquidityInfo, Error<T>>;

fn can_withdraw(who: &H160, amount: U256) -> Result<(), Error<T>>;

/// After the transaction was executed the actual fee can be calculated.
/// This function should refund any overpaid fees and optionally deposit
/// the corrected amount, and handles the base fee rationing using the provided
Expand Down Expand Up @@ -1094,6 +1097,20 @@ where
Ok(Some(imbalance))
}

fn can_withdraw(who: &H160, amount: U256) -> Result<(), Error<T>> {
let account_id = T::AddressMapping::into_account_id(*who);
let amount = amount.unique_saturated_into();
let new_free = C::free_balance(&account_id).saturating_sub(amount);
C::ensure_can_withdraw(
&account_id,
amount,
WithdrawReasons::FEE, // note that this is ignored in ensure_can_withdraw()
new_free,
)
.map_err(|_| Error::<T>::BalanceLow)?;
Ok(())
}

fn correct_and_deposit_fee(
who: &H160,
corrected_fee: U256,
Expand Down Expand Up @@ -1187,6 +1204,15 @@ where
Ok(Some(imbalance))
}

fn can_withdraw(who: &H160, amount: U256) -> Result<(), Error<T>> {
let account_id = T::AddressMapping::into_account_id(*who);
let amount = amount.unique_saturated_into();
if let WithdrawConsequence::Success = F::can_withdraw(&account_id, amount) {
return Ok(());
}
Err(Error::<T>::BalanceLow)
}

fn correct_and_deposit_fee(
who: &H160,
corrected_fee: U256,
Expand Down Expand Up @@ -1258,6 +1284,10 @@ where
fn pay_priority_fee(tip: Self::LiquidityInfo) {
<EVMFungibleAdapter<T::Currency, ()> as OnChargeEVMTransaction<T>>::pay_priority_fee(tip);
}

fn can_withdraw(who: &H160, amount: U256) -> Result<(), Error<T>> {
EVMFungibleAdapter::<T::Currency, ()>::can_withdraw(who, amount)
}
}

pub trait OnCreate<T> {
Expand Down
8 changes: 8 additions & 0 deletions primitives/evm/src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,14 @@ impl<'config, E: From<TransactionValidationError>> CheckEvmTransaction<'config,
}
}

pub fn max_withdraw_amount(&self) -> Result<U256, E> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does with_balance_for in CheckEvmTransaction meet your needs?

Ok(self
.transaction_fee_input()?
.0 // max_fee_per_gas
.saturating_mul(self.transaction.gas_limit)
.saturating_add(self.transaction.value))
}

pub fn validate_common(&self) -> Result<&Self, E> {
if self.config.is_transactional {
// Try to subtract the proof_size_base_cost from the Weight proof_size limit or fail.
Expand Down