-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: Move Schedule
from Storage to Config
#8773
Conversation
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-schedule-to-config 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
|
/benchmark runtime pallet pallet_contracts |
Finished benchmark for branch: at-schedule-to-config 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
|
da23bd1
to
e5ea458
Compare
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'm not aware of the debugging tools and necessity for contracts, maybe it would still be useful to be able to print message for executed contract on some dev chain, people could compile their specific debug runtime which enables every print message.
But maybe being able to get message on rpc execution is enough.
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.
apart from the migration which doesn't seem righ #8773 (comment),
and I am not sure about the debug necessity for contracts (like maybe we could keep enable_println
in schedule and user would compile their own debug runtime), but I feel the propose implementation is good too.
Otherwise implementation is good to me.
I really like to get rid of the complexity of having an additional storage item. Those would need to be queried even when doing on-chain execution. This PR saved one storage access by avoiding this. I think it would make sense to print the debug_buffer to the node console after every call as tracing message. This way one can enable those messages during compilation. |
Yes but if we keep |
Ahh yes you are right. I like to avoid additional tuning knobs for logging output, though. It adds to the list of things someone needs to know to start debugging. I think I just add a tracing output (on RPC execution) so we can use the well known rust log mechanism: |
Just the outstanding question about potential repeated debug messages
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
This is a great idea, and really helps with debugging/developing contracts. |
bot merge |
Trying merge. |
* Move `Schedule` from Storage to Config * Updated CHANGELOG * Fix nits from review * Fix migration * Print the debug buffer as tracing message * Use `debug` instead of `trace` and update README * Add additional assert to test * Rename `schedule_version` to `instruction_weights_version` * Fixed typo * Added more comments to wat fixtures * Add clarification for the `debug_message` field
As a followup to #8359 this moves the
Schedule
data structure from being in-storage to be supplied asGet<Schedule>
through theConfig
trait. The added complexity of having it in storage is not really justified: Changes to weights without any other changes that would require a runtime upgrade anyways are a bit far fetched.One challenge that arises from this change is that we loose the ability to influence values in the
Schedule
through a genesis config. This means we cannot have different values for different chain specs. Previously, we enabled the ability for contracts to print log messages by setting a value in the schedule when running when running a--dev
chain. This is no longer possible now.Instead, we came up with the following solution implemented in this PR: A contract is able to print log messages through
seal_debug_message
(renamed fromseal_println
) if it is called through an RPC (opposed to being called through an extrinsic). This means debug message printing is disabled when executing on-chain and therefore does not add overhead to the path we care about. Additionally, those messages are not printed to the node console but collected into a debug buffer that is then passed on to the RPC client:substrate/frame/contracts/common/src/lib.rs
Lines 37 to 54 in 3d955c5
It can then decide how to present those messages to the user. This can be seen as a precursor to implementing #5239 where we will enrich the returned
debug_message
with further information (on error) like for example a stack trace.@jacogr Can we make the
debug_message
available to the user when it is non-empty?@Robbepop @seanyoung New contract callable function
seal_debug_message
. It is documented here:substrate/frame/contracts/src/wasm/runtime.rs
Lines 1393 to 1420 in 964c0e0