-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix lazy batch contract removal #10728
Fix lazy batch contract removal #10728
Conversation
User @pmikolajczyk41, please sign the CLA 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.
Thanks for spotting this. You need to make the CI happy, though :)
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.
Thanks for catching this!
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.
Fix looks fine, but I have a question about the details behind actual contract termination
assert_ok!(Contracts::call( | ||
Origin::signed(ALICE), | ||
addr.clone(), | ||
0, | ||
GAS_LIMIT, | ||
None, | ||
vec![] | ||
)); |
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.
How does this end up terminating the contract?
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.
If you look at the code in fixtures/self_destruct.wat
then you should find the information, that by passing empty input to thte contract, it will call seal_terminate
. And as far as I can understand text wasm format it does so :)
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.
Correct. The contract terminates if called with an empty input.
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "piomiko/fix-batch-deletion" with command cargo run --quiet --profile=production --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 Toolchain: stable-x86_64-unknown-linux-gnu (default) Results
|
… piomiko/fix-batch-deletion
…--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
(31_915_000 as Weight) | ||
// Standard Error: 1_000 | ||
.saturating_add((98_000 as Weight).saturating_mul(q as Weight)) | ||
(0 as Weight) | ||
// Standard Error: 3_000 | ||
.saturating_add((2_285_000 as Weight).saturating_mul(q as 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.
This increase is expected as the buggy code before wasn't doing doing much per queue item.
bot merge |
Waiting for commit status. |
* Fix lazy batch contract removal * Apply suggestions * Qualify ChildInfo * Negligible change to restart pipeline * Revert "Negligible change to restart pipeline" This reverts commit b38abb6. * cargo run --quiet --profile=production --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 Co-authored-by: Parity Bot <[email protected]>
* Fix lazy batch contract removal * Apply suggestions * Qualify ChildInfo * Negligible change to restart pipeline * Revert "Negligible change to restart pipeline" This reverts commit b38abb6. * cargo run --quiet --profile=production --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 Co-authored-by: Parity Bot <[email protected]>
* Fix lazy batch contract removal * Apply suggestions * Qualify ChildInfo * Negligible change to restart pipeline * Revert "Negligible change to restart pipeline" This reverts commit b38abb6. * cargo run --quiet --profile=production --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 Co-authored-by: Parity Bot <[email protected]>
This PR fixes a minor bug in lazy removal of smart contracts.
A contract is removed in two steps. Firstly, it disappears from contract lookup (its info is gone). This phase is cheap and thus immediately performed. However, cleaning a storage may be an expensive operation, hence it is done lazily. We keep a (bounded) queue of the contracts to be removed and in the beginning of each block we try to remove as many as possible. The method
process_deletion_queue_batch
inframe/contracts/src/storage.rs
is supposed to do so. However, PR #9669 has changed semantics a little bit (while
->if
), which resulted in removing at most one contract at a time.