Skip to content
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

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Aug 17, 2023

Fixes #1239.

As suggested here: #1239 (comment), this will allow a newer toolchain > 1.69 to be used to deploy to older versions of pallet-contracts which do not yet support signext.

It does so by adding an extra wasm-opt pass which "lowers" any signext instructions to mvp only features.

Tested that a contract built with Rust >= 1.70 can be uploaded to a node which does not support signext.

If it does work we can then consider either backporting this to a 3.2.0 release, or just going ahead and doing a 4.0.0 release.

@ascjones ascjones marked this pull request as ready for review August 17, 2023 12:43
// 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)
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

@ascjones ascjones Aug 17, 2023

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.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Thanks!

@ascjones ascjones merged commit 756aa38 into master Aug 18, 2023
10 checks passed
@ascjones ascjones deleted the aj/wasm-opt-signext-lowering branch August 18, 2023 10:34
ascjones added a commit that referenced this pull request Aug 21, 2023
* Add `SignextLowering` pass to `wasm-opt`

* Remove check for Rust version

(cherry picked from commit 756aa38)
ascjones added a commit that referenced this pull request Aug 22, 2023
* 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>
@ascjones ascjones mentioned this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo-contract 3.1.0 emits sign_ext ops when using rustc 1.69.
3 participants