-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SignextLowering
pass to wasm-opt
#1280
Conversation
// This pass will then remove any `signext` instructions in order that the resulting | ||
// Wasm binary is compatible with older versions of `pallet-contracts` which do not | ||
// support the `signext` instruction. | ||
.add_pass(Pass::SignextLowering) |
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.
Since paritytech/substrate#14565 we do allow sign_extension in smart contracts on the pallet side. Shouldn't we make this pass optional for the cargo-contract so that the users of the newer versions of pallet-contracts could still opt in to have signext instructions in their contracts?
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 am aware, and I did consider making this optional. I decided against it because removing them by default means that both old and new versions of pallet_contracts
will be supported with this, so will make users and our lives easier if the resulting wasm is compatible with as wide a range of target nodes as possible.
I assume the majority of node implementations currently are using an older version of pallet_contracts
which does not support signext
.
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 see. Could adding smth like --allow-signext
to the cargo-contract contract build
so that wasm_opt
runs without the pass, be an option then?
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.
Yes, it would be preferable to do that if the user really does want signext
instructions.
It would be interesting to do some analysis to the benefits or otherwise of contracts with or without the signext
lowering pass. Could check for differences in contract sizes and gas efficiency.
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!
* Add `SignextLowering` pass to `wasm-opt` (#1280) * Add `SignextLowering` pass to `wasm-opt` * Remove check for Rust version (cherry picked from commit 756aa38) * Update wasm-opt * Cargo.lock * Enable Wasm sign_ext (#1189) * Enable Wasm sign_ext * Bump substrate crates * Add warning when building with new rustc * Stable toolchain * Revert "Stable toolchain" This reverts commit aa2a2e6. --------- Co-authored-by: Andrew Jones <ascjones@gmail.com> * Cargo.lock * Remove version check * WIP updating subxt * Updating subxt --------- Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Fixes #1239.
As suggested here: #1239 (comment), this will allow a newer toolchain >
1.69
to be used to deploy to older versions ofpallet-contracts
which do not yet supportsignext
.It does so by adding an extra
wasm-opt
pass which "lowers" anysignext
instructions to mvp only features.Tested that a contract built with Rust >=
1.70
can be uploaded to a node which does not supportsignext
.If it does work we can then consider either backporting this to a
3.2.0
release, or just going ahead and doing a4.0.0
release.