-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Integrate pallet_contracts gas with the weight system #5712
Conversation
It looks like @athei signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
@@ -713,7 +700,6 @@ pub type SignedExtra = ( | |||
frame_system::CheckNonce<Runtime>, | |||
frame_system::CheckWeight<Runtime>, | |||
pallet_transaction_payment::ChargeTransactionPayment<Runtime>, | |||
pallet_contracts::CheckBlockGasLimit<Runtime>, |
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.
As a curious note, this could have been a breaking change.
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.
It brakes chains which use the contracts module as they will be referencing the now gone signed extension.
What are you saying here?
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.
Hm, what do you mean by "referencing"? I was thinking that this could have been a breaking change if CheckBlockGasLimit
required including some data into the extrinsics extras (but AFAIK it didn't).
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.
Okay then you need to define "braking". Because every chain node compiled against an older version of substrate that uses the contracts pallet will no longer compile after this change. That is what I call braking change.
And yes you are right. It did only validation. No inclusion of additional data.
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.
By "breaking" i meant that the format of the extrinsics would have been incompatible.
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.
What would be the implication of that? I mean you would import the old extrinsics with a compatible runtime then upgrade the runtime and expect other extrinsics.
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.
Looks reasonable.
// them efficiently. | ||
pub type Gas = u64; | ||
// Gas is essentially the same as weight. It is a 1 to 1 correspondence. | ||
pub type Gas = frame_support::weights::Weight; |
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 would consider making a wrapper type to have type safety and make sure we actually do convert one to another and not blindly pass weight to wherever gas is expected.
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.
Eventually I want to remove Gas
as a unit altogether as there is no difference from Weight
anymore. So in a sense it is OK to "blindly pass weight to wherever gas is expected". I just decided to leave Gas as is here as changing it would added too many changes to this PR.
c96184a
to
5b509ee
Compare
Resolved the TODO by scaling the Ready for review now. |
Co-Authored-By: Tomasz Drwięga <[email protected]>
25139f8
to
39ef81d
Compare
Looks good. The only thing that I'd change is the comment on So a comment stating that it is just a ballpark and that we need to perform proper benchmarks would be good |
Co-Authored-By: Sergei Pepyakin <[email protected]>
I agree. Added the comment. |
Related Items
#4147 This was a previous attempt which didn't had a working weight refund to build upon.
#5446 Ground work that rescaled the weights o a higher resolution so they can be used as gas without additional scaling
#5458 Allows our dispatchables to return the actual weight used in order to refund unspent gas.
Overview
This greatly simplifies the gas system in pallet_contracts by integrating it with Substrate's weight system. Additionally it makes contract execution contribute to the weight of a block which is not the case right now. It also fixes the problem of
ext_dispatch_call
calls not being billed based on their actual weight.It effectively unifies gas and weight:
1 weight = 1 gas = 1 ps
. ps is one pico second of execution on our reference system. However, in order to reduce to code churn of this PR I did not rename all occurrences of the word gas to weight. This could be done in a followup PR. From this PR forward these words are used interchangeably.Changes
Gas price is gone
The contracts pallet no longer deals with buying and refunding gas. The
gas_price
is gone. The dispatchables are simply annotated to weigh their supplied gas limit and are billed accordingly by theChargeTransactionPayment
signed extension:Using the new weight refund system we can refund unused gas through the
ChargeTransactionPayment
signed extension.CheckBlockGasLimit
is goneBecause, unlike previously, the gas now contributes to the regular weight of a block we no longer need our own signed extension that checks that a block does not use too much gas. This is now checked by
frame_system::CheckWeight
as for any other extrinsic.Remove the gas limit argument from
put_code
The weight costs of
put_code
are known pre dispatch as they only depend on the length of the supplied code which is not executed at that time. Therefore we removed the argument and changed the weight annotation so that it calculates the weight from the code length.Fees are removed from the config trait
All costs are now specified on the dynamically changeable (by root)
Schedule
and are specified in Weight / Gas instead of some fixed Balance.ext_dispatch_call
bills based on the real weightBecause it is now possible to refund deducted weight post dispatch we do exactly this on
ext_dispatch_call
: Remove the weight (+ base cost) from theGasMeter
and refund the unspent weight post dispatch.Tuning the gas schedule
Since we are no longer using a separate currency of gas but effectively weight to formulate the costs of contract actions we need to retune the default
Schedule
that contains these costs. It looks currently like this (unmodified values):Given that one unit here constitutes 1 weight = 1 picosecond of execution these values are way to low now. These are the new values:
This is just a ballpark over estimation that scales the old defaults which where also just a ballpark estimation. Benchmarking and properly tuning the gas costs is not in scope here.