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

Find a stable and convenient solution for floats implicit loading before cosmwasm 1.0 #458

Closed
KamiD opened this issue Jul 1, 2020 · 14 comments

Comments

@KamiD
Copy link

KamiD commented Jul 1, 2020

Summary of Bug

there is a problem about floating number type support differentiation in tinyGo and rust-wasm.

  • in cosmwasm, Floating number is disabled, so the function "parse_wasm_opcode" which in source file primitives.rs will denied all floating opcode, such as F32Load or any opcode about floating number,
  • when i using tinygo to build some code like json parser or make a custom marshal interface, since i used a package include float type, the F32Load OPCODE will complie into wasm file by tinyGo, so i have to check and recheck.

that's not a finally solution, I think there at least have two way to fix that:

  1. disable all float support in tinyGo, that's not very easy~
  2. allow wasm file which inluding floating opcode loaded,that's not cool for security of cosmwasm

Version

  • Cosmwasm v0.8
@webmaster128 webmaster128 added this to the 1.0.0 milestone Jul 1, 2020
@KamiD
Copy link
Author

KamiD commented Aug 3, 2020

hi guys, checking the unsupported symbol during compiling~ how do you think ?
I wrote a tool for wasm file check, it will translate wasm file to wat file, then analyze every single line to make sure no unsupported opcode include, it will generate calling map and relationship between functions, find out where is the unsupported opcode using~
Also it will check tables such as export\import\memory or anything else.
after some resolving, it will translated from wat file to wasm file, that's a way to determined wasm file is suitable with cosmwasm-vm(pass through some checkXXX in cosmwasm-vm) during compiling

@webmaster128
Copy link
Member

@KamiD such a tool seems very helpful for debugging. However, I would only check the Wasm and explain. Manipulating the Wasm can easily become dangerous when the tool does not 100 % work as intended.

Building a call tree for debugging is nice until you hit call_indirect, where the call tree suddenly depends on runtime variable values.


I was thinking about a different approach to rejecting Wasm ops codes: we can replace every float operation with Operator::Unreachable. This would allow us to accept floats as long as they are never used at runtime.

@ethanfrey
Copy link
Member

I was thinking about a different approach to rejecting Wasm ops codes: we can replace every float operation with Operator::Unreachable. This would allow us to accept floats as long as they are never used at runtime.

I did this in a preprocessing compile step, before uploading wasm blobs, when working on the TinyGo prototype. There were float operations that were never executed in runtime, but still not removed by the compiler/optimizer.

This is a bit dangerous, as it may cause contracts that really use floats to fail unexpectedly, but it can resolve the issue. I would not do this on the server, but require the user uploading contracts to do this as a pre-processing step, maybe with some warning if there were more floats removed than typical

@webmaster128
Copy link
Member

Removing from the 1.0 milestone as the state of this is too vague to block the release. We can open up floats later on. It would be consensus breaking but existing contracts would not be affected.

@webmaster128 webmaster128 removed this from the 1.0.0 milestone Jun 22, 2021
@ethanfrey
Copy link
Member

Agree

@webmaster128
Copy link
Member

NaN canonicalization is available for all Wasmer backends now. NEAR is using to overcome the only non-deterministic aspect of floats in Wasm and seems to be happy with it.

Should we just open up? That would allow all kinds of calculations to become possible in CosmWasm contracts that you cannot do in native Cosmos SDK because of the additional sandbox layer.

@ethanfrey
Copy link
Member

If we can guarantee they produce the same results on ARM (M1 and "Rasperry Pi"), Intel and AMD, then I am happy to allow them. It would make lots of things much easier. I just want to make sure this is safe.

@ethanfrey
Copy link
Member

Maybe you can show me the NEAR test vectors / fuzzing that I can use to convince myself this is safe.

@webmaster128
Copy link
Member

Singlepass v1 is x86_64 only and looking at wasmerio/wasmer#2463 this will not change anymore. So for a cross-chipset test you first need a cross-backend test. I wonder if that is desired? Why should e.g. cranelift and singlepass calculate deterministic results. Maybe it works though. There are tests in the Wasmer code base I can look into.

@ethanfrey
Copy link
Member

The issue for me is if floats are safe in x86 only singlepass v1, but no longer safe in multi-architecture singlepass v2, but we already enabled float support with v1, then we can never upgrade to v2.

The conservative alternative is to not allow them until v2 is out and we are sure this is deterministic cross-architecture. At that point we can remove the "no-float" rule with a minor release of CosmWasm, either together with v2 support or separate. But sure that we will not regret that step.

Personally, it would make PoE and bonding curves much easier to code. Just too much time at Tendermint having "floats are evil in blockchain" drilled into my head.

@AmitPr
Copy link
Contributor

AmitPr commented May 24, 2023

Hey all, any update on the state of support across various architectures that would allow this to move forward?

@betterclever
Copy link

Hey everyone!
Any updates on the support of Float ops? I encountered an issue while trying to use pbjson with prost-serde plugin. I think it uses FP32 internally somewhere.
Currently, trying to debug it using the tool mentioned in the thread.

@chipshort
Copy link
Collaborator

I just started working on float support, probably coming in CosmWasm 1.5.
Keep an eye on #1845 and any PRs related to that.

@webmaster128
Copy link
Member

Closing in favour of #1845

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

No branches or pull requests

6 participants