cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
202 stars 79 forks source link

do not use f64/f32/usize as it fails some wasm runtimes (cosmwasm) #800

Closed dzmitry-lahoda closed 1 year ago

dzmitry-lahoda commented 1 year ago

Feature Summary

CosmWasm(wasmd), and hence IBC WASM-08 fail to upload wasm file if it has code with f32/f64. Unfortunately some things have usize which sometimes handled with floats (crazy but fact https://medium.com/cosmwasm/debugging-floating-point-generation-in-rust-wasm-smart-contract-f47d833b5fba ).

Given these things and that ibc-rs to support many runtime (near, solana, aptos, ic) which also may dislike these floats (as constraint and blockchain enviroments in general) preventing usage of this with linter is good.

Usize, except for some reason some crates use floats to handle it (now sure why), is of variable size, which may lead to bugs, e.g. when target would be wasm64.

Proposal

Remove all f64/f32/usize usage and lint that from repo.

LEGACY

Proposal

I used ibc-rs, and in latest version I get

12:54AM DBG invalid tx code=2 log="failed to execute message; message index: 0: Error calling the VM: Error compiling Wasm: Could not compile: WebAssembly translation error: Error in middleware Gatekeeper: Float operator detected: F64Load { memarg: MemoryImmediate { align: 3, offset: 0, memory: 0 } }. The use of floats is not supported.: create wasm contract failed" module=state
12:54AM INF executed block height=159 module=state num_invalid_txs=1 num_valid_txs=0

when it is part of contract.

I see old Cargo.lock

 "tendermint 0.31.1",
 "tendermint-light-client-verifier 0.31.1",
 "tendermint-proto 0.31.1",
 "dyn-clone",
 "erased-serde",
 "ibc-proto 0.30.0",
 "ics23 0.9.0",
 "ibc 0.41.0",

new

 "tendermint 0.32.2",
 "tendermint-light-client-verifier 0.32.2",
 "tendermint-proto 0.32.2",
 "ics23 0.10.1",
 "ibc-derive 0.1.0",
 "ibc-proto 0.32.0",
 "ics23 0.10.1",
informalsystems-pbjson

Also I found f64 usage in this repo when not needed.

So I do not use any things changed directly, and yet get issues.

Run https://github.com/CosmWasm/cosmwasm/blob/main/packages/check/README.md in CI.

I did not yet found root cause so.

Thing happend from 0.41 release inclusive and before 0.43. I was able to upload contract.

I think not only cw wasmd will be such, but wasm-08 too.

dzmitry-lahoda commented 1 year ago

I am trying to strip contract, if that works. UPDATE: did not helped

dzmitry-lahoda commented 1 year ago

Okey, it was PrefixedDenom

dzmitry-lahoda commented 1 year ago
pub(crate) fn calculate_block_delay(
    delay_period_time: &Duration,
    max_expected_time_per_block: &Duration,
) -> u64 {
    if max_expected_time_per_block.is_zero() {
        return 0;
    }

    FloatCore::ceil(delay_period_time.as_secs_f64() / max_expected_time_per_block.as_secs_f64())
        as u64
}

So such code if it runs in ics wasm-08 likely will not work.

dzmitry-lahoda commented 1 year ago

I stably reproduce on my contact, but failed to repro on small contract. When I copy paste PrefixedDenom instead of ref, check passes.


All contracts (1) passed checks!
bash-5.1$ nix run github:informalsystems/cosmos.nix#cosmwasm-check /home/dz/github.com/ComposableFi/composable/code/target/wasm32-unknown-unknown/cosmwasm-contracts/cw_xc_gateway.wasm
Available capabilities: {"stargate", "cosmwasm_1_1", "iterator", "staking", "cosmwasm_1_2"}

/home/dz/github.com/ComposableFi/composable/code/target/wasm32-unknown-unknown/cosmwasm-contracts/cw_xc_gateway.wasm: failure
Error compiling Wasm: Could not compile: WebAssembly translation error: Error in middleware Gatekeeper: Float operator detected: F64Load { memarg: MemoryImmediate { align: 3, offset: 8, memory: 0 } }. The use of floats is not supported.

Passes: 0, failures: 1
bash-5.1$ nix run github:informalsystems/cosmos.nix#cosmwasm-check /home/dz/github.com/ComposableFi/composable/code/target/wasm32-unknown-unknown/cosmwasm-contracts/cw_xc_gateway.wasm
Available capabilities: {"stargate", "cosmwasm_1_2", "iterator", "staking", "cosmwasm_1_1"}

/home/dz/github.com/ComposableFi/composable/code/target/wasm32-unknown-unknown/cosmwasm-contracts/cw_xc_gateway.wasm: pass

All contracts (1) passed checks!

The only differnece, I copied PrefixedDenom to our code. We use Channel/Port/Connect-Id, but when adding PrefixedDenom ref it breaks wasm.... Magic.

dzmitry-lahoda commented 1 year ago

Added cw-check example https://github.com/dzmitry-lahoda-forks/ibc-rs/tree/dz/9/ci/cw-check . It still may be usefull to do this to ensure it can run in wasm hosts with no floats support (blockchains).

dzmitry-lahoda commented 1 year ago

https://medium.com/cosmwasm/debugging-floating-point-generation-in-rust-wasm-smart-contract-f47d833b5fba

so ideally ibc-rs and its deps not to use usize

plafer commented 1 year ago

Thank you for bringing this to our attention! Very subtle...

dzmitry-lahoda commented 1 year ago

I fixed thing, it works for me now.

dzmitry-lahoda commented 1 year ago

this so works only on wasm https://github.com/chipshort/wasm-float-transpiler, not riskv or bpf or native float free. but it may be alfa version, and having yet another layer of compilation can be buggy.

Farhad-Shabani commented 1 year ago

Fixed in #894