-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: Charge rent for code storage #7935
Conversation
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-rent 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
|
b40d5b3
to
17428ca
Compare
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-rent 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
|
e04da97
to
1604eeb
Compare
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-code-rent 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: Andrew Jones <ascjones@gmail.com>
Co-authored-by: Andrew Jones <ascjones@gmail.com>
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
apart from above comments, it looks good to me |
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 prefer to have some comment for:
- the
occupied_storage
which can return a wrong value as it can make use of the outdated refcount. - internal comment saying refcount usage is tedious and should be used with care
- maybe some internal comment saying that instantiate will pay rent without the increment of refcount.
- and usage
size_of::<PrefabWasmModule<T>>()
should be replaced with something accurate or having a strong coment saying that the inaccuracy is fine because negligeable in regards to code len and original code len.
all good to me now |
bot merge |
Trying merge. |
Fixes #6596 <- Check this out for an introduction to this topic.
@jacogr This replaces the
put_code
dispatchable withinstantiate_with_code
and therefore requires changes to the off-chain environment.This PR is separated into two commits which implemented the linked issue:
Implement refcounting for wasm code
Here we implement a
PrefabWasmModule::refcounter
for theCodeStorage
storage item that holds the instrumented code for every contract including some metadata like the said counter. Every time a contract is instantiated using a specific code the counter is incremented and when the contract is wilfully terminated or evicted the counter is decremented. When it reaches zero the code is removed from storage.As a consequence of this we can no longer allow to store code to the chain without deploying a contract at the same time (the refcounter for this code would be zero). This is why we also remove the
put_code
dispatchable and replace it with theinstantiate_with_code
dispatchable.This commit is so big because almost every test needed to be adapted and because some substantial refactoring was necessary for decoupling the code instrumentation from the code storage step.
Also charge rent for code storage
This will be a small PR because all the refactoring work was done in the previous commit. It merely changes the rent formula to also account for the storage that is occupied by the contracts code. These costs are shared between all contracts that use the same code.
The bulk of the commit is number tuning of the tests to account for the greatly increased rent incurred during testing.
Bonus commit: Fix dispatchables erroneously refunding base costs
When unifying weight and gas an error slipped through which caused dispatchables to always refund their base costs. This means only the gas costs where charged but not the overhead of the extrinsic itself. I noticed the error during test refactoring and fixed it.