Use #![no_std] for ibc-proto crate

[soareschen]

Partially Closes: #1158

Description

This is a follow up on @romac's comment in #1156

c) Were the changes in the ibc-proto crate done manually? If so, can we automate that somehow? eg. via a prost flag/feature or within the proto-compiler crate as a post-processing step?

I just realized ibc-proto crate do not use much of std features, and we can enable #![no_std] for the crate rather easily without modifying the generated code. This PR does the fix separately from #1156.

One thing I notice: even though the ibc-proto crate successfully compiles with #![no_std], it is in fact not fully no-std-compatible as at least one of its transitive dependencies, socket2, does not work in no_std. I could not find easy way to verify that other than building ibc-proto with the target wasm32-unknown-unknown:

$ cargo build --target wasm32-unknown-unknown
error[E0432]: unresolved import `crate::sys`
 --> /home/ubuntu/.cargo/registry/src/github.com-1ecc6299db9ec823/socket2-0.4.0/src/sockaddr.rs:5:12
  |
5 | use crate::sys::{
  |            ^^^ could not find `sys` in the crate root
...

The crate socket2 is imported via hyper, which is used by tonic. When inspecting the tonic and socket2 crate, I also find various uses of std::io and std::net, which are not available in no_std.

This shows that supporting no_std is more tricky than just enabling the #![no_std] flag in our own crate. The no_std flag does not check on the compliance on dependencies, and I cannot find a good way to check on that other than building with the target wasm32-unknown-unknown. Other than tonic, it is also not clear how many other dependencies we have are unsupported in the real no_std environment.

@DaviRain-Su do you have any experience in how to verify if a crate truly support no-std environment?

---

For contributor use:

• Updated the Unreleased section of CHANGELOG.md with the issue.
• If applicable: Unit tests written, added test to CI.
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/) and code comments.
• Re-reviewed Files changed in the Github PR explorer.

[romac]

Nice!

Regarding the dependency problem, I guess this means that we need add a std feature, and only depend on tonic and enable the gRPC service code generated by tonic when the std feature is enabled. Does that make sense?

[DaviRain-Su]

I also think it is possible that it is not necessary to change all the code to a type that is only possible under no_std, here toinc is doing rpc I think it is possible to use std directly here, at the time I was compiling and testing my branch was not changed here is also possible, because proto's other structure types are already supported by no_std.
Relate pr: #1109

[en]

@DaviRain-Su do you have any experience in how to verify if a crate truly support no-std environment?

Hi Soares,

AFAIK, the only way is to build the whole project then check for errors.

We're following parity's best practices:

1. Add this section to Cargo.toml

[features]
default = ["std"]
std = []

2. Add this line to the top-level lib.rs:

#![cfg_attr(not(feature = "std"), no_std)]

3. Then check no-std support using the following command:

$ cargo check --no-default-features

For a crate that uses the same rule, we can add it to std = [] to inherit the feature settings.

[soareschen]

Regarding the dependency problem, I guess this means that we need add a std feature, and only depend on tonic and enable the gRPC service code generated by tonic when the std feature is enabled. Does that make sense?

I think that is doable, however that would still means that we would need to patch the generated prost files to switch between std and no-std mode. We could also consider submitting patches for tonic to support no-std upstream. From what I see, prost already have no-std support, including the generated files, but tonic don't.

@DaviRain-Su do you mean that the build in #1109 successfully compiles in Substrate environment? I am not familiar with Substrate, so is there any tools or steps to verify that a Rust library works in Substrate? My understanding is that the requirement for no-std is due to Substrate using WebAssembly. So what I currently know is to build it with the wasm32-unknown-unknown target to verify the support under WebAssembly.

@en I'm not sure if the approach you suggested checks for true no-std support. FYIW, the PR here makes ibc-proto always activate no_std regardless of whether it is used in std or no-std mode. But its dependency tonic still requires std. Also note that tonic and socket2 do not have an std feature flag for us to disable through Cargo.toml.

[DaviRain-Su]

@DaviRain-Su do you mean that the build in #1109 successfully compiles in Substrate environment? I am not familiar with Substrate, so is there any tools or steps to verify that a Rust library works in Substrate? My understanding is that the requirement for no-std is due to Substrate using WebAssembly. So what I currently know is to build it with the wasm32-unknown-unknown target to verify the support under WebAssembly.

You said subsrate and I thought of it here because the proto didn't compile over at the time. Because of the socket2 problem. The same problem you are having here.

Also note that tonic and socket2 do not have an std feature flag for us to disable through Cargo.toml.

I agree.

[hu55a1n1]

do you have any experience in how to verify if a crate truly support no-std environment?

@soareschen You should also be able to use cross-rs to build for a no_std target such as wasm32-unknown-emscripten to test for no_std support.

[soareschen]

I have added a no-std-check crate that checks for no-std compliance based on the guide available here: https://blog.dbrgn.ch/2019/12/24/testing-for-no-std-compatibility.

The no-std-check crate has a lib.rs file that imports a list of modules that we want to check. It also registers a panic handler, which will cause a compile error if the crate is linked with std, which also provides a panic handler.

With ibc-proto added to it, we get the following error:

$ cargo build --manifest-path no-std-check/Cargo.toml 
   Compiling no-std-check v0.1.0 (/data/development/informal/ibc-rs/no-std-check)
error[E0152]: found duplicate lang item `panic_impl`
  --> src/lib.rs:31:1
   |
31 | fn panic(_info: &PanicInfo) -> ! {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: the lang item is first defined in crate `std` (which `prost` depends on)
   = note: first definition in `std` loaded from /home/ubuntu/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-b6b48477bfa8c673.rlib
   = note: second definition in the local crate (`no_std_check`)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0152`.
error: could not compile `no-std-check`

Which shows that the check fails because prost depends on std. On the other hand if we remove the import of ibc-proto and import only flex-error with default features off, the crate builds successfully, showing that flex-error passes the no-std check.

I have also added a GitHub Actions workflow that runs the no-std check. Here is an example run. Obviously the CI is failing now because ibc-proto is not no-std-compliant. Before merging this, we will disable the check on ibc-proto so that the no-std-check test can pass. We can then add ibc-proto back to the list again when it becomes fully no-std compliant.

[soareschen]

I just try out a new approach which is to use the build-std feature in Cargo nightly to build and core and alloc crates explicitly. More details are in the rendered readme. Here is an example run of errors raised because the bytes crate is trying to use std.

[soareschen]

Let's wait for #1166 to merge first before we merge this.

[romac]

Let's wait for #1166 to merge first before we merge this.

It's not clear when we will be able to merge it, since there appears to be multiple regressions in prost 0.8 which are also affecting tendermint-rs.

So let's not wait on #1166 before merging this :)

[soareschen]

Alright then. I have removed the GitHub Actions workflow for now so that the CI can pass. I'm leaving the no-std-check crate there so that we can check for no-std compliance in future development.

[DaviRain-Su]

I think that is doable, however that would still means that we would need to patch the generated prost files to switch between std and no-std mode.

I think the code generated by prost here can be just under no_std, the standard library (std library) in rust is a superset of the core library (under no_std, e.g. alloc, core). Here I think it is OK to save only the no_std ones. For the compilation here still can not pass my understanding or tonic does not support std reason. Is it possible to change a library that can support no_std mode to provide grpc?

[romac]

Is it possible to change a library that can support no_std mode to provide grpc?

This is likely not necessary. We can probably keep using tonic by only adding a std feature to ibc-proto and only enabling tonic's transport feature when the std feature is enabled.

[DaviRain-Su]

Is it possible to change a library that can support no_std mode to provide grpc?

This is likely not necessary. We can probably keep using tonic by only adding a std feature to ibc-proto and only enabling tonic's transport feature when the std feature is enabled.

It's also a good way to end

[DaviRain-Su]

I wonder if it is possible to decouple proto and grpc here, so that proto can support NO-STD, I am trying to modify it.

[soareschen]

@romac should we merge this first and then do the separation of generated code in another PR? I have filed cosmos/ibc-proto-rs#7 as a follow up.

[romac]

@romac should we merge this first and then do the separation of generated code in another PR? I have filed cosmos/ibc-proto-rs#7 as a follow up.

Agreed. Let's merge this once CI is green.

[tac0turtle]

SWEEEEEEETTT!!!