-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: Remove weight pre charging #8976
Conversation
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-fix-precharging Benchmark: Benchmark Runtime Pallet cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs ResultsPallet: "pallet_contracts", Extrinsic: "on_initialize", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
6c4110a
to
dfa9e83
Compare
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-fix-precharging Benchmark: Benchmark Runtime Pallet cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs ResultsPallet: "pallet_contracts", Extrinsic: "on_initialize", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
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.
One nitpick and one note, but otherwise looks good to me
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Sorry for pinging you all. I messed up the merge. Will fix. |
c4c2295
to
8ddca0f
Compare
bot merge |
Trying merge. |
Follow-up to paritytech/substrate#8976.
Fixes #8693
Please see the linked issue for an explanation of the rationale. I merely describe the three chunks of work this fix consists of that also correspond to the initial commits:
Remove pre-charging for code size
#8086 added a mechanism that charges weight when accessing a wasm blob based off its size. See this PR for a detailed explanation. The way this was done is by pre charging a maximum weight (based on the maximum allowed size for a wasm blob) and refund it after the blob was loaded and the size was determined. This approach was chosen to keep the methodical approach of charging weight on entry (extrinsinc, contract API) where the code size is unknown rather than spreading weight charging all over the code.
However, this pre charging was problematic for all the reasons already explained in #8693. As a result we move to a model of charging the weight much deeper into the execution where we can easily access the size of the wasm blob without loading it from storage. This gets rid of the pre charging and also removes the need for passing around the wasm size.
Remove pre-charging when reading values of fixed size
Reading values of a fixed type like
T::Balance
from a contract places us in a difficult position. Those have a fixed size in SCALE encoded form but the latter is unknown at compile time. This means when reading those values from contract memory we still need the untrusted contract to supply the size of encoded value it is passing. This requires us to account for the case where the contract passes a too large size for the passed value inducing processing costs for copying the data before decoding it. This is awkward to benchmark because in reality the value has a fixed size. In order to prevent the creation of many basically useless benchmarks we opted for charging some generic costs based off the supplied size and refund that after a successful decoding. Refunding is performed because the costs of reading fixed size types are included by any benchmark making use of that value. It is only to defend against an attack where a too large input size if supplied for a given type (which will fail because ofdecode_all
).With the introduction of
MaxEncodeLen
we finally gain the ability to determine the fixed size at compile time. We use this trait to ignore the length passed by the contract and use the length exposed by the trait instead. This makes pre charging no longer needed because we don't use any untrusted length parameters anymore.Add new versions of API functions that leave out parameters
With the addition of
MaxEncodeLen
we can remove all length parameters belonging to fixed sized types. We added a new version for each API where this is the case. The unstable newcall
API is als modified to have those parameters removed ( cc @cmichi ).