Skip to content

Conversation

@pLabarta
Copy link
Contributor

This pull request introduces changes to the frame/ethereum and frame/evm modules to improve transaction validation and add a new method for checking the maximum withdrawable amount on the client side.

@pLabarta pLabarta requested a review from sorpaas as a code owner December 10, 2024 18:48
@pLabarta pLabarta marked this pull request as draft December 10, 2024 18:48
@pLabarta pLabarta marked this pull request as ready for review December 10, 2024 21:09
}
}

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?


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.

@librelois
Copy link
Collaborator

I spent some time evaluating whether this change is still necessary, and the answer is yes.
with_balance_for is not equivalent to the check introduced in this PR.

with_balance_for relies on the reducible_balance implementation in pallet-balances, which allows spending reserved amounts.
In contrast, our custom can_withdraw implementation does not allow spending reserved amounts.

Here are my notes comparing the behavior of both implementations:

Upstream (reducible_balance):

total_payment = max_fee_per_gas * gas_limit + value
Reducible balance >= total_payment

Untouchable = frozen - reserved
Reducible balance = free - untouchable
Reducible balance = free - (frozen - reserved)
Reducible balance = free - frozen + reserved

⇒ total_payment <= free - frozen + reserved

Our fork (can_withdraw):

max_withdraw_amount == total_payment
can_withdraw(max_withdraw_amount) must be true

free - max_withdraw_amount >= frozen
⇒ free >= max_withdraw_amount + frozen
⇒ max_withdraw_amount <= free - frozen

As you can see, the behavior and constraints differ significantly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants