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

wasm-override: Support checking spec_name #10380

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Nov 27, 2021

This adds support to the wasm-override feature to compare the spec_name. If the spec_name doesn't
match, a warning will be printed and the override will be ignored.

Closes: #9685

This adds support to the wasm-override feature to compare the spec_name. If the spec_name doesn't
match, a warning will be printed and the override will be ignored.
@bkchr bkchr 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. labels Nov 27, 2021
@bkchr bkchr requested a review from insipx November 27, 2021 15:45
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Nov 27, 2021
Copy link
Contributor

@insipx insipx left a comment

Choose a reason for hiding this comment

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

LGTM!

#[derive(Clone, Debug, PartialEq)]
/// The interval in that we will print a warning when a wasm blob `spec_name`
/// doesn't match with the on-chain `spec_name`.
const WARN_INTERVAL: Duration = Duration::from_secs(30);
Copy link
Contributor

@kianenigma kianenigma Nov 30, 2021

Choose a reason for hiding this comment

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

wouldn't it be much easier if we just abort the command if this didn't match? I would continuously print a warning if at least the wasm-override was applied, and now you want to warn the user. If it was not even applied, I would either exit, or warn once about it and move on.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would spare you most of the Arc/Mutex layers as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

wouldn't it be much easier if we just abort the command if this didn't match?

Which command?

I mean the best would be to do that at startup, but there we don't know the spec name and that is also legal to change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Printing just one warning, will probably not bring that much, as it can be overseen relative easily.

@bkchr bkchr merged commit 3d005a3 into master Dec 3, 2021
@bkchr bkchr deleted the bkchr-wasm-override-check-spec-name branch December 3, 2021 10:29
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
This adds support to the wasm-override feature to compare the spec_name. If the spec_name doesn't
match, a warning will be printed and the override will be ignored.
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
This adds support to the wasm-override feature to compare the spec_name. If the spec_name doesn't
match, a warning will be printed and the override will be ignored.
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--wasm-runtime-overrides does not check spec name
4 participants