Use core and alloc crates for no_std compatibility

[soareschen]

Partially Closes: #1158

Description

In this PR I search for the use of std:: imports and change them to use core or alloc. The unsupported constructs are left to be imported from std and be fixed through other PRs for #1158.

A notable change in this PR is that we are now using BTreeMap instead of HashMap, and BTreeSet instead of HashSet. The reason is that the formers are not supported in the alloc crate, as they depends on random sources which are not available in no-std. I'm not sure if this change would break anything, so we might want to investigate further on this.

---

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]

What's the advantage of doing this rather than use something like no-std-compat?

[soareschen]

What's the advantage of doing this rather than use something like no-std-compat?

• core and alloc are officially maintained by Rust itself. So we can be confident that they will be supported in the long run.
• The author of no-std-compat has sadly passed away, and the library is likely no longer maintained.
• Std wrapper crates tend to get outdated quickly when new versions of the standard library is released. I have experienced similar issue in Haskell, where the base-noprelude package which wraps base got outdated when new version of GHC is released, and that caused a lot of pain to update / workaround all dependents to that wrapper package.
• Polyfill std libraries do not necessarily follow the exact behavior of the original std constructs. This means we cannot easily refer back to the vanilla Rust std documentation. On the other hand core and alloc guarantee the same implementation as in std, and drops the unsupported constructs otherwise.
• If we are using constructs unsupported by core and alloc such as std::io, it is likely that any polyfill crate would have custom implementation of the constructs. In such case we should use the polyfill crate directly whether in std or no-std mode, so that there is less maintenance burden of switching between two versions of the code.
  • The polyfill crate can still switch between std mode and polyfill mode behind the scene. But from the perspective of ibc-rs as the user of the library, that should be transparent.
  • Using the polyfill crate directly means that we are aware of its existence and always refer to the polyfill's documentation instead of the official Rust documentation. So in practice it is the same as us using a third party library instead of std for that functionality.

[romac]

Very good points, thanks @soareschen! Let's go with that then!

Some follow-up questions:

a) Can also add a workflow to the CI to build the ibc crate in no-std mode, to ensure we don't accidentally introduce dependencies on std and friends?
b) I am wondering why we are doing this changes in the ibc-relayer, ibc-relayer-cli, ibc-telemetry and ibc-proto-compiler crates when those crates are not meant to be compiled with no-std?
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?

[soareschen]

a) Can also add a workflow to the CI to build the ibc crate in no-std mode, to ensure we don't accidentally introduce dependencies on std and friends?

The current change do not support no-std just yet. There are still use of constructs from std such as std::error::Error. So we will need further PRs to fix that. The purpose of this PR is so that we can minimize the later changes required when no-std support is ready.

b) I am wondering why we are doing this changes in the ibc-relayer, ibc-relayer-cli, ibc-telemetry and ibc-proto-compiler crates when those crates are not meant to be compiled with no-std?

I think it is good to be consistent with the convention so that we do not need to switch between the conventions when developing on different crates. The use of core and alloc also makes it more clear of which features we are using are only supported in std, in case we want to support no-std in those crates later on.

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?

That's a good point! I have reverted the changes and created #1164 to address the issue separately.

[romac]

@soareschen What do you think about defining our own prelude which re-exports commonly used types such as Vec, String, FromStr, ToString, etc?

[soareschen]

What do you think about defining our own prelude which re-exports commonly used types such as Vec, String, FromStr, ToString, etc?

I can see that having such prelude would make it more convenient to use common constructs. However I would advise against building our own prelude library, as it can be tedious to keep the exports in sync with a proper "subset" of std. When there are inconsistencies, it can also cause confusion when developers are referring to the official Rust documentation.

What we could do instead is to create a minimal prelude that re-exports everything from core::prelude::v1 and alloc::prelude::v1. That way we are still delegating the management of what is being exported to core and alloc.

Note that the alloc prelude is currently nightly only. So I think it is fine to have custom re-exports for now that mirror what is available in alloc::prelude::v1. Looking at the definitions, it also seems like core::prelude::v1 is already made available in no_std. So the constructs from alloc::prelude::v1 are probably the only ones we need to add on top of it.

[romac]

What we could do instead is to create a minimal prelude that re-exports everything from core::prelude::v1 and alloc::prelude::v1. That way we are still delegating the management of what is being exported to core and alloc.

Note that the alloc prelude is currently nightly only. So I think it is fine to have custom re-exports for now that mirror what is available in alloc::prelude::v1. Looking at the definitions, it also seems like core::prelude::v1 is already made available in no_std. So the constructs from alloc::prelude::v1 are probably the only ones we need to add on top of it.

Sounds good to me!

[adizere]

What is the status of this PR? I think we should move forward with it.

Are there any implications of using BTreeMap and BTreeSet, performance, or correctness, that we should be aware of?

[romac]

Are there any implications of using BTreeMap and BTreeSet, performance, or correctness, that we should be aware of?

I don't see any implications on correctness as I don't believe we every relied on the iteration order of elements of a HashMap or HashSet (which would be dumb anyway as it is non-deterministic).

As for performance, most operations on BTree-based data-structures have worse asymptotic complexity than their Hash-based counterparts, see the table below.

I don't believe that's an issue at the moment, as we do not maintain any data structure large enough for it to matter (especially as most operations in Hermes are IO-bound anyway).

— source

[soareschen]

I have made a few more changes to this PR after merging with master:

#![no_std] with extern crate std

I have now activated #![no_std] in the ibc crate, with an extern crate std to explicitly re-import the std crate. This in effect remove the implicit imports from std::prelude, so that we do not accidentally use features that are implicitly exported by std prelude, such as println!.

Custom crate::prelude Crate

With #![no_std] activated, this means that std prelude constructs such as format!, vec!, String, and Vec are no longer implicitly imported in every Rust module. To compensate with this, I have added a private prelude crate that can be imported with use crate::prelude::* inside the ibc crate. The prelude essentially re-exports everything from core::prelude::v1 and alloc::prelude::v1, but excludes std-only prelude constructs such as println!.

Unfortunately, it seems like Rust still do not allow us to set a custom prelude inside a crate. (rust-lang/rfcs#890) So it is necessary to call use crate::prelude::* in every individual modules that need to use prelude constructs. Nevertheless, this should hopefully make the developers' life easier as compared to having to import from modules such as alloc::format, alloc::string::String, etc everywhere.

Remaining std Uses

With the two changes above, there are only a few places remaining in the ibc crate that still uses std:

• std::error::Error - This use can be removed once we upgrade to new version of tendermint-rs.
• std::thread::sleep and std::printlnt - These are only used in tests, so we can probably conditionally allow std in #[cfg(test)].
• std::time::SystemTime - This seems to be the only tricky bit left that is being used by the protobuf code generated by prost/tonic. We may want to investigate whether it is possible to generate no-std-compliant structs from prost that do not use SystemTime.

[romac]

Thanks for the update @soareschen! This all sounds good to me.

std::time::SystemTime - This seems to be the only tricky bit left that is being used by the protobuf code generated by prost/tonic. We may want to investigate whether it is possible to generate no-std-compliant structs from prost that do not use SystemTime.

This seems to be the main remaining issue. If there is no support for this directly in prost/tonic, do you see a way of working around that?

[soareschen]

This seems to be the main remaining issue. If there is no support for this directly in prost/tonic, do you see a way of working around that?

It seems like SystemTime is not directly used, rather as an intermediary conversion between tendermint::Time and prost_types::TimeStamp.

We will have to fix tendermint::Time, as it is actually a newtype wrapper for chrono::DateTime without providing access to the underlying value. (Reminder that if we are defining newtypes next time, please do expose the underlying values as public fields) Once that is done, we should be able to convert chrono::DateTime to the nanos and seconds values directly.

[soareschen]

@romac shall we merge this?

[romac]

Let's go, we just need an entry in the .changelog folder using unclog.

[soareschen]

we just need an entry in the .changelog folder using unclog.

Done.

outdated

[romac]

Awesome work @soareschen, thanks a lot!