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

Implement Wasm validation for known issues/markers #171

Merged
merged 19 commits into from
Feb 11, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Feb 5, 2021

Closes #163.

Output currently looks like this:

…
 [2/3] Post processing wasm file                                                                                                                               
ERROR: Validation of the Wasm failed.                                          
                                                                               
                                                                               
ERROR: An error was found while compiling the contract:                       
The ink! message with the selector `0xAD931DAA` contains an invalid trait call. 
                                                                               
Please check if the mutable parameter of the function to call is consistent                                                                                    
with the scope in which it is called.                                                                                                                          
                                                                               
                                                                                                                                                               
ERROR: An unexpected panic function import was found in the contract Wasm.                                                                                     
This typically goes back to a known bug in the Rust compiler:                                                                                                  
https://github.com/rust-lang/rust/issues/78744

As a workaround try to insert `overflow-checks = false` into your `Cargo.toml`. 
This will disable safe math operations, but unfortunately we are currently not  
aware of a better workaround until the bug in the compiler is fixed.

@cmichi cmichi marked this pull request as draft February 5, 2021 14:06
src/cmd/build.rs Outdated Show resolved Hide resolved
@cmichi cmichi marked this pull request as ready for review February 5, 2021 15:17
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

It would be possible to make it forward compatible if we wanted by adding these to an extra section in the generated metadata, though this would of course rely on the metadata being generated first.

@Robbepop
Copy link
Contributor

Robbepop commented Feb 8, 2021

It would be possible to make it forward compatible if we wanted by adding these to an extra section in the generated metadata, though this would of course rely on the metadata being generated first.

This protocol should not exist in the contract metadata since it has absolutely no value beyond cargo-contract. If it escapes cargo-contract's build it is already bad. Since it is a protocol I guess there is no proper way around providing an implementation on both sides: ink! serves and cargo-contract is a client. Please proof me wrong if I am.

@ascjones
Copy link
Collaborator

ascjones commented Feb 8, 2021

That makes sense.

There is still a way at least to provide some forward compatibility by detecting the common prefix __ink_enforce_error and enumerating the subsequent arguments even if we wouldn't be able to provide a nice error message.

Copy link
Contributor

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

nice! 🚀

src/validation/mod.rs Outdated Show resolved Hide resolved
src/validation/validate_wasm.rs Outdated Show resolved Hide resolved
src/validation/validate_wasm.rs Outdated Show resolved Hide resolved
src/validation/validate_wasm.rs Outdated Show resolved Hide resolved
src/validation/validate_wasm.rs Outdated Show resolved Hide resolved
src/validation/validate_wasm.rs Outdated Show resolved Hide resolved
src/validation/validate_wasm.rs Outdated Show resolved Hide resolved
.expect("error marker must exist as prefix");
let hex = serde_hex::from_hex(&encoded).expect("decoding hex failed");
let decoded =
<EnforcedErrors as codec::Decode>::decode(&mut &hex[..]).expect("decoding object failed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably still return a friendly string explaining that the EnforcedError could not be decoded, and the probable cause (a mismatch between the ink! version and the local definition).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could occur when e.g. ink! adds a new variant, and someone is running an outdated version of cargo-contract.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@cmichi cmichi merged commit 4f7356b into master Feb 11, 2021
@cmichi cmichi deleted the cmichi-validate-wasm-import-fns branch February 11, 2021 14:31
@ascjones ascjones mentioned this pull request Feb 22, 2021
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.

Ensure building/checking depends on Wasm import functions being correct
3 participants