Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add client side check of withdraw-ability #1546

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
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
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
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
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
Loading