use-ink / cargo-contract

Setup and deployment tool for developing Wasm based smart contracts via ink!
GNU General Public License v3.0
243 stars 119 forks source link

cargo-contract 3.1.0 emits sign_ext ops when using rustc 1.69. #1239

Closed kvinwang closed 1 year ago

kvinwang commented 1 year ago

Cargo-contract says "Build using a version lower than 1.70.0 to make sure your contract will deploy".

However, we have noticed that sign_ext operations appear in the wasm build with cargo-contract-3.1.0+rustc-1.69 and there are no such problems when building with cargo-contract-3.0.1+rustc-1.69.

This might be due to #1189 enabling SignExt for wasm-opt, leading to the emission of sign_ext instructions even if Rustc didn't originally produce them.

Here is the contract.

ascjones commented 1 year ago

This might be due to https://github.com/paritytech/cargo-contract/pull/1189 enabling SignExt for wasm-opt, leading to the emission of sign_ext instructions even if Rustc didn't originally produce them.

Interesting. Could you confirm this by cloning cargo-contract, removing the .enable_feature(Feature::SignExt) and then building using cargo run -- contract build --release --manifest-path=/path/to/your/contract

Also maybe @brson can confirm whether this is expected behaviour for wasm-opt.

kvinwang commented 1 year ago

Interesting. Could you confirm this by cloning cargo-contract, removing the .enable_feature(Feature::SignExt) and then building using cargo run -- contract build --release --manifest-path=/path/to/your/contract

Tried. There is no sign_ext ops when the .enable_feature(Feature::SignExt) was removed.

ascjones commented 1 year ago

Thanks @kvinwang this is a bug we need to address. Because of this and other issues related to the toolchain e.g. https://github.com/paritytech/cargo-contract/issues/1240, we decided to yank 3.1.0.

This will be replaced shortly with a 4.0.0-alpha release for people wanting to use Rust >= 1.70 immediately https://github.com/paritytech/cargo-contract/pull/1243.

So currently users who want to use toolchains < 1.70 they will have to use 3.0.1. And for users who want to use >= 1.70 they should use 4.0.0-alpha

We may want to make 4.0.0 compatible with both these versions of the toolchains, but that will require a bit more work, therefore the move back to prerelease alpha.

SkymanOne commented 1 year ago

cargo-contract compatibility with different rust toolchains

Rust <= 1.69 + pallet-contracts < polkadot-1.0.0 = cargo-contract 3.0.1 Rust > = 1.70 + pallet-contracts >= polkadot-1.0.0 = cargo-contract 4.0.0-alpha

dtr0x commented 1 year ago

I followed the instructions and switched to using Rust <= 1.69 + pallet-contracts < polkadot-1.0.0 = cargo-contract 3.0.1. This fixes the issue with the wasm build, but now I am getting the error referenced in #1259:

---- flipper::e2e_tests::it_works stdout ----
thread 'flipper::e2e_tests::it_works' panicked at 'We should find a port before the reader ends', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/ink_e2e-4.2.1/src/node_proc.rs:192:10

Therefore I'm not sure if this is related to the present issue or not, I installed the contracts substrate node at v0.27.0 but the e2e test doesn't seem to be able to spin the node up.

gcp-development commented 1 year ago

@dtr0x , the e2e test does not work after the 3.1.0 has been yanked. It needs to be updated also.

brson commented 1 year ago

This might be due to #1189 enabling SignExt for wasm-opt, leading to the emission of sign_ext instructions even if Rustc didn't originally produce them.

Interesting. Could you confirm this by cloning cargo-contract, removing the .enable_feature(Feature::SignExt) and then building using cargo run -- contract build --release --manifest-path=/path/to/your/contract

Also maybe @brson can confirm whether this is expected behaviour for wasm-opt.

@ascjones I have asked @kripken about this. I am not clear on what it means to enable / disable a feature. It might be the case that enabling a feature allows the optimizers to use that feature, which would result in sign extension ops where they didn't exist before. I do think I recall that binaryen features are not about "gating" or disallowing ops in the input modules, but I'm not sure.

kripken commented 1 year ago

This is expected behavior for wasm-opt. When a feature is enabled, the optimizer will both accept input modules using that feature and also allow itself to emit new instructions that depend on the feature. (For sign extension, it can do things like turn x << 24 >> 24 - two shifts - into a single sign-extend operation from 8 bits to 32.)

kripken commented 1 year ago

Note btw that wasm-opt has a pass called --signext-lowering:

   --signext-lowering       lower sign-ext operations to
                            wasm mvp and disable the sign
                            extension feature

I think that might solve your issues here? It would remove any signext stuff that rustc emits that you don't want, and avoid adding any more during optimizations.

To use it, enable the feature (so that a wasm with such instructions can load without failing validation) and then run the pass.

brson commented 1 year ago

Turning on signext lowering in the wasm_opt crate would look like:

        OptimizationOptions::from(self.optimization_level)
            .mvp_features_only()
            .add_pass(Pass::SignextLowering)
            .zero_filled_memory(true)
            .debug_info(self.keep_debug_symbols)
            .run(dest_wasm, &dest_optimized)?;

This would seemingly get cargo-contract back to a state where it could disable signext opts, as long as all contracts were sanitized through cargo-contract.

ascjones commented 1 year ago

Thanks for the suggestion @kripken and @brson. PR here https://github.com/paritytech/cargo-contract/pull/1280