-
Notifications
You must be signed in to change notification settings - Fork 479
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
Align GasWeightMapping
with Substrate do_pre_dispatch
logic
#865
Align GasWeightMapping
with Substrate do_pre_dispatch
logic
#865
Conversation
This is a good fix given how involved it would be to fix through substrate, but there is one quite minor issue left: the txpool will avoid including txns which would just barely fit. In other words, when a txn would fit by a margin smaller than ...quite minor... :) |
That also applies to any extrinsic no? If a weight annotation hints 99 weight, the weight left in the block is 100, and the base extrinsic weight is 2, then that extrinsic overall weight is 101 and it would exhaust resources. |
You mean a normal Substrate txn? Yes, it would apply to those as well, but they would end up charging for that weight (instead of refunding it later like we're trying to do here). Edit: elaborate: Take the following example, let's say we have two similar txns, one Substrate-based and one Ethereum-based.
The normal Substrate txn will work something like this as far as txpool is concerned:
The Ethereum txn will work a bit differently:
|
Sorry but I'm not sure I totally understand, or more specifically how is that related to this change. Including (or not) the transaction in a block based on the weight annotation overestimation - gas limit VS. gas used - is what ethereum does. If the user is hinting he might spend up to 6000, and the txpool has 5000 space left, the transaction must not be included, independently on what the real usage was after exiting the evm. |
The user is hinting at 5000 weight (from In my example, the user hints at 5000 weight, and by the time we are done executing (with the changes in this PR), it will correctly account for 5000. But in |
You can ignore the above concerns of mine, I was confused about this approach. I see that it will result in the desired behavior. (cc @sorpaas) However, I do think we should have a separate fn in |
Instead a separate function I added the |
…adot-evm#865) * Align `GasWeightMapping` with Substrate `do_pre_dispatch` logic * Add `without_base_extrinsic` param * fmt (cherry picked from commit 2d802a1)
…adot-evm#865) * Align `GasWeightMapping` with Substrate `do_pre_dispatch` logic * Add `without_base_extrinsic` param * fmt
This is a followup PR for what was mentioned by @notlesh in #851 (comment)
This PR proposes:
pallet-evm
that treated Gas and Weight as the same unit.WeightPerGas
associated type topallet-evm
.GasWeightMapping
for an arbitrary struct inpallet-evm
in which'sgas_to_weight
implementation the base base extrinsic weight is substracted to the result.The goal is align with how
CheckWeight::do_pre_dispatch
works. In there, (pre-dispatch)DispatchInfoOf + base_extrinsic
is put inframe_system::BlockWeights
storage. After (post-dispatch),PostInfo
(the actual gas/weight used by the evm) is used to correct the accountedWeight
, but thebase_extrinsic
still over-accounted. By subtracting thebase_extrinsic
in thegas_to_weight
we fix that.Note
This proposal assumes:
21_000 * WeigthPerGas
.