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

Uploading wasm file appear function non-existent error #364

Closed
jeremymj opened this issue Mar 22, 2020 · 30 comments · Fixed by #1049
Closed

Uploading wasm file appear function non-existent error #364

jeremymj opened this issue Mar 22, 2020 · 30 comments · Fixed by #1049
Labels
C-question Further information is requested E-blocked The task is blocked on some other task to be finished.

Comments

@jeremymj
Copy link

I want to use the signature verification method in my contract,and add dependency in cargo.toml as fellow:
schnorrkel = { version = "0.9.1",features = ["preaudit_deprecated", "u64_backend"], default-features = false}
using cargo contract build command can generate wasm file normally and uploading the wasm file to node, got some error message:

2020-03-22 22:13:36.028 tokio-blocking-driver DEBUG runtime  DispatchError
2020-03-22 22:13:36.028 tokio-blocking-driver DEBUG runtime  module imports a non-existent function

using cargo-contract version is 0.5.0 ;
node of substrate version is: version 2.0.0-alpha.4-0b3020796-x86_64-linux-gnu
more about reproduce code, please go to my repository example

@Robbepop
Copy link
Collaborator

Can you please insert your .wasm file and tell us how you generate the .wasm file?
Did you use cargo-contract? If yes, what version? Please inspect with cargo contract --version.
Also preaudit_deprecated looks like you are using a deprecated feature.

@jeremymj
Copy link
Author

yes,I use cargo-contract generate wasm file and cargo-contract version is 0.5.0.
Based on your suggestions, I updated the test code and removed preaudit_deprecated, but the problem persists.This is the wasm file I generated.

@Robbepop
Copy link
Collaborator

Robbepop commented Mar 23, 2020

I don't know why it is there but I inspected your .wasm file and found the import that is causing this:

(import "env" "_ZN4core9panicking5panic17h9ff647c44ac14387E" (func (;6;) (type 1)))

To me it looks like a Rust (or C++) mangled name and probably has something to do with panicking since "panic" is mangled in its name.
So some dependency maybe is importing this?

@jeremymj
Copy link
Author

I don't understand the meaning of this expression _ZN4core9panicking5panic17h9ff647c44ac14387E,
This dependency is estimated to be introduced in schnorrkel. You can use the github address I provided to check whether the dependency is introduced in the cargo.lock file.

@Robbepop Robbepop added the C-question Further information is requested label Mar 23, 2020
@athei
Copy link
Contributor

athei commented Mar 25, 2020

This is rust name mangeling. Something inserted a call to core::panicking::panic: https://doc.rust-lang.org/stable/core/panicking

It is an experimental API where a no_std crate can panic but then requires a another crate to actually implement the panic function. Nobody does that in your case therefore it is just an unresolved imported function. However, I cannot see schnorrkeel making use of this API. Maybe schnorrkel is a red herring.

It would be interesting which function calls panic. However, the contract is stripped of all function names. The only thing I can see is that the imported function is actually called multiple times.

@jeremymj
Copy link
Author

@athei I used schnorrkel library from the sp-core module dependency in the substrate project, and they should also compile using no_std. If there is a problem with schnorrkel, they probably have encountered similar errors like this,

@athei
Copy link
Contributor

athei commented Mar 26, 2020

This imported call to panic is only there when I use cargo contract for building the contract. And it is not introduced by the post processing steps but by the actual build. I tried to mimic cargo contract build as closely as possible by manual cargo commands. But I never get that panic call introduced into my wasm file.

I added this to your Cargo.toml:

[package.metadata.cargo-xbuild]
memcpy = false
sysroot_path = "target/sysroot"
panic_immediate_abort = true

And then ran

 RUSTFLAGS="-C link-arg=-z -C link-arg=stack-size=65536 -C link-arg=--import-memory" cargo +nightly xbuild --target wasm32-unknown-unknown --no-default-features --release

Which should be exactly what cargo contract is doing. However, the resulting wasm file is correct (without the panic function).

I am not sure what is happening there in cargo contract.

I then looked at the broken wasm file produced by cargo contract (I modified it to remove post processing) to look for the callers of the panic function. There are two:

(func $_ZN51_$LT$u64$u20$as$u20$core..ops..arith..AddAssign$GT$10add_assign17h1e01cef87efc54f2E (type 12) (param i32 i64)

(func $_ZN51_$LT$i64$u20$as$u20$core..ops..arith..AddAssign$GT$10add_assign17h99e945e958616f28E (type 12) (param i32 i64)

I guess these are implementations of core::ops::AddAssign. Just by which concrete type?

@athei
Copy link
Contributor

athei commented Mar 27, 2020

After some discussion with my coworkers we found out that this problem is triggered when we stop passing rlib as additional crate-type to the compiler. cargo contract does exactly this in order to reduce the size of the resulting wasm file. When building with cargo manually the rlib is passed to the compiler. This explains the difference. What is unclear is why this is happening.

@jeremymj
Copy link
Author

Oh, I see. Thank you for your reply to this issue. Since this problem is triggered when we stop passing rlib as additional crate-type to the compiler, is there a way to fix it?

@ascjones
Copy link
Collaborator

ascjones commented Mar 27, 2020

☝️ Quickly added an option to disable manifest modifications for troubleshooting. However this (with rlib as a crate-type) will likely result in a significant increase in binary size: e.g. for incrementer 18.8k vs 5.1k (optimized)

@Robbepop
Copy link
Collaborator

Robbepop commented Mar 27, 2020

point_up Quickly added an option to disable manifest modifications for troubleshooting. However this (with rlib as a crate-type) will likely result in a significant increase in binary size: e.g. for incrementer 18.8k vs 5.1k (optimized)

Usage of this command is generally not recommended for use cases other than debugging.
My guess so far is that the library you are using is non compatible with contracts for some reason and we should find out why and not work around that problem by fiddling around with some configs here and there.
Our current findings is that the rlib target causes trouble if it is being removed. However, the reality is that this rlib option only exists because of our metadata generation process and should ideally not even be there at all. Not removing something that shouldn't exists in the first place is a clear indicator that something else is broken.

@ascjones
Copy link
Collaborator

Yes to be clear I was not suggesting that this was a fix for the actual problem, rather an option to allow troubleshooting using cargo contract build.

@jeremymj
Copy link
Author

Well, I understand what you said, thanks again.

@athei
Copy link
Contributor

athei commented Apr 23, 2020

Removing rlib enables link time optimization because currently rust silently disables lto when one of the crate types do not support them (rust-lang/rust#51009). This is also the reason why the contract gets smaller with rlib removed.

I bet that the bug also vanishes when you keep rlib but disable lto.

@Robbepop
Copy link
Collaborator

Does the issue still exist?

@athei
Copy link
Contributor

athei commented May 20, 2020

I could still reproduce it when writing the last commit. And I could confirm my hunch that the reason is lto and not the rlib. Basically a link time optimization bug.

@Robbepop
Copy link
Collaborator

I could still reproduce it when writing the last commit. And I could confirm my hunch that the reason is lto and not the rlib. Basically a link time optimization bug.

Then it might be connected to the bug that has been worked around by the newest cargo-contract version 0.6.1?

@Robbepop Robbepop changed the title uploading wasm file appear function non-existent error Uploading wasm file appear function non-existent error May 20, 2020
@ascjones
Copy link
Collaborator

@athei can you try again with 0.6.1, and if that fails then the latest master of cargo-contract?

@athei
Copy link
Contributor

athei commented May 20, 2020

No dice. With both version this bad boy is still there:

(import "env" "_ZN4core9panicking5panic17h00ea5caca74bc649E" (func (;6;) (type 1)))

It is like with lto it does not find the #[panic_handler] which is supplied by one of the ink! crates.

@Robbepop
Copy link
Collaborator

Triage: Can anyone (@jeremymj ) please confirm that this bug still persists with the current ink! 3.0-rc1 and recent cargo-contract versions? If not I am going to close this issue the next time I come across it.

@jeremymj
Copy link
Author

I updated the test code referring to the example in the latest version of ink, but the following error occurred:
Screenshot from 2020-10-22 09-22-02
When I removed schnorrkel, the contract can be compiled and passed normally, so I cannot continue to verify it. I also tried to solve this error, but failed.The cargo contract version I am currently using is 0.7.0

@athei
Copy link
Contributor

athei commented Oct 29, 2020

I can reproduce the error with this repo and cargo-contract 0.7.1:
https://github.com/athei/test_verify

The only known workaround when you encounter this bug is adding this to your Cargo.toml:

[profile.release]
overflow-checks = false

@athei
Copy link
Contributor

athei commented Nov 4, 2020

I was able to strip down this issue to its cause: rust-lang/rust#78744

@Robbepop Robbepop added the E-blocked The task is blocked on some other task to be finished. label Jan 14, 2021
@Robbepop
Copy link
Collaborator

Marked with blocked since it depends on a bug fix for a Rust compiler problem.

@cmichi
Copy link
Collaborator

cmichi commented Aug 5, 2021

I just tried to reproduce this bug with our latest versions of ink!, canvas-node, cargo-contract and rustc 1.56.0-nightly (2827db2b1 2021-08-01). This is no longer possible, the contract can be deployed just fine. I assume the bug disappeared at some point and am closing this issue therefore.

@cmichi cmichi closed this as completed Aug 5, 2021
@athei
Copy link
Contributor

athei commented Aug 5, 2021

I have heard that you work on automated regression tests. Maybe this could be a candidate for it. So we can detect when it reappears on a newer nightly.

@cmichi
Copy link
Collaborator

cmichi commented Aug 5, 2021

@athei Yes, for contract size regressions: paritytech/ink-waterfall#4.

It's a good idea to add a contract which provoked this bug to the waterfall and see if it still deploys fine in the future, I'll add it.

@athei
Copy link
Contributor

athei commented Nov 19, 2021

😱

It appears that this is back. What a tragedy. My minimal reproducer does not trigger it anymore. This is why it passed our regression test in rustc I assume. In order to reproduce:

cargo-contract 0.15.0-unknown-x86_64-macos
rustc 1.58.0-nightly (cc946fcd3 2021-11-18)
cargo +nightly contract new flipper
cd flipper
cargo +nightly contract build

ERROR: Validation of the Wasm failed.

...

Please note that the error does not appear in nightly-2021-11-17. So nightly-2021-11-18 introduces this regression.

@athei
Copy link
Contributor

athei commented Nov 23, 2021

Updated the reproducer and opened a new bug report: rust-lang/rust#91158

@athei
Copy link
Contributor

athei commented Nov 24, 2021

Also filed an issue with cargo that could resolve our problems when implemented: rust-lang/cargo#10118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question Further information is requested E-blocked The task is blocked on some other task to be finished.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants