-
Notifications
You must be signed in to change notification settings - Fork 1k
pallet_revive: Change EVM call opcodes to respect the gas limit passed #10018
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
base: master
Are you sure you want to change the base?
Conversation
/cmd prdoc --audience runtime_dev |
I could validate that this resolves paritytech/contract-issues#119 for EVM execution (but not for PVM execution). |
Yes PVM is not fixed, yet. It is listed as follow ups in the PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we re-run the benchmarks as well?
I want to run them in a separate PR once all the open stuff is merged. Otherwise we are creating too many merge conflicts. |
All GitHub workflows were cancelled due to failure one of the required jobs. |
Will run the benchmarks here once #9418 is merged. |
Ok(Permill::from_percent(20).mul_ceil(gas_price)) | ||
// We do not support tips. Hence the recommended priority fee is | ||
// always zero. The effective gas price will always be the base price. | ||
Ok(Default::default()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any data points on wallets being confused by the fact that this is zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this (eth_maxPriorityFeePerGas) is not really used in practice, wallets and library like alloy will usually use eth_feeHistory
to propose the priority fee
Since we don't support this concept for now anyway we are better off with this change anyway
let weight_fee_available = | ||
T::FeeInfo::weight_to_fee(&frame.nested_gas.gas_left(), Combinator::Min); | ||
let available_balance = self | ||
let weight_fee_available = T::FeeInfo::weight_to_fee(&frame.nested_gas.gas_left()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we calculate weight_to_fee
for gas left instead of gas consumed, we would need to combine via min instead of via max again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally: weight_fee_available
needs to be multiplied be the next fee multiplier.
.origin | ||
.account_id() | ||
.map(|acc| { | ||
T::Currency::reducible_balance(acc, Preservation::Preserve, Fortitude::Polite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use the reducible_balance
of the origin here? This has no meaning if exec_config
is ethereum like (i.e., self.exec_config.collect_deposit_from_hold
is Some
).
Shouldn't we instead limit the initial storage deposit limit for all substrate like executions by initially checking that this limit is at most the origin's reducible balance – or better even, reduce the origin's balance initially by this deposit limit for substrate like transactions?
|
||
// the gas_limit is in unadjusted fee | ||
let deposit_limit = { | ||
let weight_fee = T::FeeInfo::weight_to_fee(&weight_limit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be a weight_to_fee
calculation where we take the minimum instead of maximum to be consistent with https://github.com/paritytech/polkadot-sdk/pull/10018/files#r2435295902
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that there is a general problematic inconsistency with the fact that the gas_left
is the minimum of
- the available imbalance in tx payment
- the sum of a) the available gas as determined by available weight and b) the available gas as determined by available storage deposit
But both are incompatible – Example: when the first frame immediately makes a call to another contract and does not explicitly limit gas, then in calc_limits
, gas_limit
equals available
.
Then weight_limit
is equal to nested_gas.gas_left()
. Since this is right at the beginning of the first frame, this is essentially the original weight limit as determined in try_into_checked_extrinsic
.
By definition FeeInfo::weight_to_fee
of this original weight limit is then almost equal to ((the tx payment imbalance amount) / (next fee multiplier)) [ignoring some small constant values like the FeeInfo::fixed_fee
].
But "((the tx payment imbalance amount) / (next fee multiplier))" is exactly what self.gas_left()
returns in calc_limits
because max_by_credit_hold
turns out to be (the tx payment imbalance amount) given that at this point essentially no weight or deposit has been consumed.
Thus, in calc_limits
:
weight_fee
- =
T::FeeInfo::weight_to_fee(&weight_limit)
- =
FeeInfo::weight_to_fee
of the original weight limit - = ((the tx payment imbalance amount) / (next fee multiplier))
- =
self.gas_left()
Thus, deposit_limit
as returned by calc_limits
will essentially be zero. Not what we want.
Proposal
I propose that we have completely different semantics for ethereum like or substrate like transactions.
For substrate like transactions both the initial weight and deposit limits can be used fully independently – keep the code as it is.
For ethereum like transactions the semantics is different: the gas limit can be either used for weight fees or for storage deposits – which one is unknown before. We should change:
- gas left = max(
FeeInfo::weight_to_fee
(limit of top frame's gas meter) * (next fee multiplier), (limit of top frame's deposit meter)) - (total consumed) where- total consumed =
FeeInfo::weight_to_fee
(consumed of top frame's gas meter) * (next fee multiplier) + (consumed of top frame's deposit meter)
- total consumed =
- in
calc_limit
- for the case
CallResources::Ethereum
- set
weight_left
to (limit of top frame's gas meter) -FeeInfo::fee_to_weight
(total consumed) / (next fee multiplier) - the returned deposit limit should be ((limit of top frame's deposit meter) - (total consumed)) *
ratio
- set
- for the case
CallResources::Precise
- don't return
(*weight, *deposit_limit)
directly but- limit the first entry by (limit of top frame's gas meter) -
FeeInfo::fee_to_weight
(total consumed) / (next fee multiplier) - limit the second entry by (limit of top frame's deposit meter) - (total consumed)
- limit the first entry by (limit of top frame's gas meter) -
- don't return
- for the case
Additionally, remove the calculation of max_by_credit_hold
in gas_left
, it is not required anymore.
So far the EVM family of call opcodes did ignore the
gas
argument passed to them. The consequence was that we were not able to limit the resource usage of sub contract calls. This PR changes that. Gas is now fully functional on the EVM backend.The resources of any sub contract call are now effectively limited. This is both true for
Weight
and storage deposit. The algorithm works in a way that if you passx%
of the currentGAS
the theCALL
opcode the sub call will havex%
of currently availableWeight
and storage deposit available. This allows the caller to always make sure to execute code after retuning from a sub call.Changes to the gas meter
I needed to change the gas meter to track
gas_consumed
instead ofgas_left
. Otherwise it is not possible to know the total amount of gas spent for a call stack that is not unwinded, yet.Followup
Weight
and storage deposit limit