-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: Consider contract size in weights #8086
Conversation
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-size-weights 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
|
480f056
to
db51db7
Compare
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-size-weights 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
|
70cadd4
to
e8217f7
Compare
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
e390b19
to
ef596b3
Compare
I fucked up the master merge. Needed to rebase and force push to get this history clean again. Sorry for your inconvenience. |
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-size-weights 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 Results
Downloading crates ...
|
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 good to me,
I didn't expect the code_len to propagate everywhere like this :-/ but at the end it should be ok
/// The maximum length of a contract code in bytes. This limit applies to the instrumented | ||
/// version of the code. Therefore `instantiate_with_code` can fail even when supplying | ||
/// a wasm binary below this maximum size. | ||
type MaxCodeSize: Get<u32>; |
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.
when code is reinstumented, could it happen that it goes beyond max code size ?
reading from the code it seems we don't check for it. But also it shouldn't be too much of an issue.
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.
Yes this could happen but we couldn't throw any error in this case because the contract was already deployed successfully and this would break existing contracts. However, it will be instrumented from the same original code and the contents of the schedule do not influence the size of the instrumented code. Without changes to the instrumentation code this should not be an issue.
@@ -549,10 +554,13 @@ decl_module! { | |||
let origin = ensure_signed(origin)?; |
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.
those early returns are still charged for the MaxCodeSize but I think it is fine.
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.
Yes. They are even charged for the whole gas limit in case of an early error. Those errors are all in the control of the caller to avoid. That said, I will try to eliminate overcharging in the future but for now I think this is fine.
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-size-weights 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
|
…/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
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.
LGTM
@@ -124,7 +130,7 @@ pub trait Ext { | |||
code_hash: CodeHash<Self::T>, | |||
rent_allowance: BalanceOf<Self::T>, | |||
delta: Vec<StorageKey>, | |||
) -> DispatchResult; | |||
) -> Result<(u32, u32), (DispatchError, u32, u32)>; |
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.
Can you add a comment explaining the fields in the error tuple (I assume matching (caller_code_len, tombstone_code_len)
). It's not immediately clear. Or could make a struct
instead of using a tuple.
Potentially applied to other methods here too.
bot merge |
Checks failed; merge aborted. |
For a contract to be called or instantiated its code must be pulled from storage first. Because contracts are usually allowed to grow in the kilobyte range the costs of reading or writing the code are substantial. However, up until now those costs were neglected and the weights only covered loading and storing of a minimum sized contract.
This PR changes the following entry points to consider the dynamic costs of loading and storing the contract code. Please note that the weight of changing the
refcount
(see #7935) also depends on the code size because they are bundled inside the same storage item:call
[Dispatchable] (Loads the contract code from storage in order to execute it)instantiate
[Dispatchable] (Loads the contract code from storage in order to execute it and increments the refcount)seal_call
(Same ascall
but as contract callable function)seal_instantiate
(Same asinstantiate
but as contract callable function)seal_terminate
(Decrements the refcount of the calling contracts code)seal_restore_to
(Decrements the refcount of the caller and increases the refcount of the tombstones code)All entry points pre-charge the weight for accessing a maximum size code and correct this weight after execution. This will require contracts to specify higher gas limits as what is actually necessary to execute them. Also some error paths do not correct the pre-charged weight. This can be improved in the future but right now I am concerned about safety and optimizing for the happy path.
In order to mitigate the impact that the maximum allowed code size has we tune down the default to 128KiB (down from 512KiB). We also move it from the schedule to the configuration trait because it is accessed in the weight annotation now.
@jacogr This changes the
Schedule
in-storage data structure.Why don't we "just" charge the weight as gas when loading and storing the contract instead of precharging the maximum?
Firstly, we do not know the size of the original code before loading the item because it is encoded inside the storage item. We could use the size of the whole storage item in bytes the calculate the weight but that would be imprecise because the weight depends on the original code size not the whole storage item.
Secondly, it would be a departure from the simple "weights are measured as a whole" paradigm. This means weights are measured for whole dispatchables and contract callable functions. Breaking them down into smaller elements is imprecise and comes with its own set of challenges.
Lastly, not all entry points that need to load the code execute the contract and have a gas meter. Right now this is only
claim_surcharge
. However, a special solution would have been needed for this dispatchable when trying to charge the load/store costs from a gas meter.Extra: Charge the weight needed for reinstrumentation
Whenever the current schedule changes (by governance) each contract needs to be re-instrumented with the current schedule. This happens on the first call after the schedule update for each contract. Previously, the weight needed for this reinstrumention was neglected. This PR charges the weight necessary from the gas meter of whoever is unlucky enough to call the contract for the first time after the schedule update.