Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement Clone and Default for Config #12397

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

This is a preparation stage for polkadot#4212. It does not affect any existing functionality.

What?

This PR contains two closely related but separate changes:

  1. Derive Clone for Config;
  2. Implement Default for Config which is returning not just an empty structure but some reasonable fallback defaults.

Why?

  1. sc_executor_wasmtime is designed to consume the config instead of borrowing it. I believe this is due to the possibility of replacement of instantiation strategy inside it in some cases. It works fine while config is set by a constant value. The task of execution environment parametrization introduces a need to hold that config as a part of abstract executor structure, owned by that structure. Option are either to rewrite sc_executor_wasmtime code to borrow the config instead of consuming it, or to make it possible to create config clones. By the principle of least impact, the latter approach was chosen.
  2. Right now execution environment conifg is set by an external component (in my case by the PVF executor interface) and that is totally correct solution. Nevertheless, it is pretty logical for any execution environment to provide some failsafe default which can be used out-of-the-box. First case where it could be useful is tests. You don't usually want to define a whole config for test unless you're testing with some specific parameters, you just want some safe defaults. Another case is migrations. After execution environment will have been paramertized, we'll get into the situation with older PVFs which haven't ever got a set of parameters bound to them but still need to be executed with some set of parameters. To provide a default set of parameters from the execution environment itself is just more convenient than to define them as a constant in execution environment interface. Config values used in this PR are nothing new by themselves, they are a verbatim copy of values used in PVF executor interface.

@s0me0ne-unkn0wn s0me0ne-unkn0wn added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Oct 2, 2022
@bkchr
Copy link
Member

bkchr commented Oct 2, 2022

2. Implement Default for Config which is returning not just an empty structure but some reasonable fallback defaults.

I don't see any reason for this and it should be reverted. You should also not forget that this is Substrate and not Polkadot. Meaning the executor is used for executing runtimes and there are currently different default values. TLDR, everything should just define its own Config as needed, this isn't really hard.

@s0me0ne-unkn0wn
Copy link
Contributor Author

@bkchr I'm totally aware it's Substrate and the executor may be used anywhere. I see your point, there's no good in introducing some Poltadot-related defaults to Substrate. I believe it is not about that but somewhat wider.

Executor is assigned to execute an arbitrary runtime. A user often wants a component just to do its job only falling back to modifying specific config settings when he has to. wasmtime itself provides a default engine configuration which you could modify if need be, but usually you don't have to. I just propose to introduce the same logic to Substrate executor.

@pepyakin what's your opinion?

@pepyakin
Copy link
Contributor

pepyakin commented Oct 5, 2022

Yeah, when I was reading the PR had the same thought as Basti.

Default configuration is a very elusive concept IMO. It might be OK, for a general purpose execution environment which is wasmtime, since it may try to serve a need of an average user so that it just works. Here, we deal with 2 (two) users with greatly varying needs. What is the default in this case?

I also think that configuration, esp. if it is important should live closer to where it's needed, in this case it's the Polkadot repo. I don't see any benefit of it living here.

Clone is fine though.

@s0me0ne-unkn0wn
Copy link
Contributor Author

s0me0ne-unkn0wn commented Oct 5, 2022

Okay, if you guys both tell that it shouldn't be there, then it shouldn't be there ;)
Removed default config, only Clone derivation left.

Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding only Clone is fine by me.

@bkchr
Copy link
Member

bkchr commented Oct 5, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 87224cf into master Oct 5, 2022
@paritytech-processbot paritytech-processbot bot deleted the s0me0ne/executor-config-fixes branch October 5, 2022 15:11
ordian added a commit that referenced this pull request Oct 5, 2022
* master:
  Implement `Clone` and `Default` for `Config` (#12397)
  Don't send back empty proofs if light request fails (#12372)
  MMR: impl `TypeInfo` for some structures (#12423)
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Implement `Clone` and `Default` for `Config`

* `cargo fmt`

* Remove default config implementation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants