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

Tracking issue for no-std support #1158

Closed
20 of 25 tasks
Tracked by #1401 ...
soareschen opened this issue Jul 7, 2021 · 22 comments · Fixed by #1156 or #1164
Closed
20 of 25 tasks
Tracked by #1401 ...

Tracking issue for no-std support #1158

soareschen opened this issue Jul 7, 2021 · 22 comments · Fixed by #1156 or #1164
Assignees
Labels
E: no-std External: support for no_std compatibility
Milestone

Comments

@soareschen
Copy link
Contributor

soareschen commented Jul 7, 2021

This is a tracking issue to track the progress of no-std support for ibc-rs to close #29. The change set for PR #1109 is pretty broad, and as a result it is difficult to review and merge the PR all in one go. I would suggest that we split the changes into multiple smaller PRs, so that the changes can be incremental with the goal of eventually supporting no-std.

Based on the changes in PR #1109, I'd suggest we have separate PRs for the following changes:


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@DaviRain-Su
Copy link
Contributor

Hi, I'm glad to see you've written this in such detail, and I agree with what you've written when you list this. After all the skimming through the third one listed, this one I think can go for it. For the penultimate one, I don't know who is in charge of the tendermint-rs. over there. I would like someone to review my pr to merge the code in and fix this issue.

@soareschen
Copy link
Contributor Author

For the penultimate one, I don't know who is in charge of the tendermint-rs. over there. I would like someone to review my pr to merge the code in and fix this issue.

I am also responsible to having no_std supported in tendemint-rs. I am still looking at informalsystems/tendermint-rs#890, but got side tracked by something else, sorry for haven't giving any feedback yet.

That being said, I think the work need to be done on tendermint-rs side should be similar with what's mentioned in this issue, and I will create a separate tracking issue for that. I also think that the two developments can be done in parallel, and we'd have a separate PR to update tendermint-rs in ibc-rs once the changes have been merged on tendermint-rs side.

@soareschen
Copy link
Contributor Author

soareschen commented Jul 12, 2021

I found this issue on substrate (paritytech/substrate#4043), which mentions that the std crate is in fact partially supported in WebAssembly. However Substrate decides to not support std because the std implementation is not stable. This means that the need for no-std is more of a Substrate-specific issue, rather than a WebAssembly-specific issue.

Because of this, I think we should refine the acceptance criteria of what we are aiming for here:

  • Crate-local no-std compliance: our own crates can be built with #![no_std] enabled, but the dependencies may still be using std. Example: Use #![no_std] for ibc-proto crate #1164 enables no-std on ibc-proto, but its dependent prost is still using std.
  • Full no-std compliance: Our own crates and all dependencies can be built with #![no_std] enabled. This can be verified following this guide.
  • WebAssembly compliance: Our crates can be built with WebAssembly targets such as wasm32-unknown-unknown and wasm32-wasi. Since std is still partially available there, either our crates or dependencies may use some of the std features available.
  • Substrate compliance: Our crates can be built and run on the Substrate platform. The exact step to verify this needs to be clarified.

The first three acceptance criteria are rather straightforward to verify. However I have not been able to find good source of verifying for no-std compliance on Substrate. The only information available is on the guide which shows how to configure Cargo.toml feature flags to switch between std and no_std modes. It is important to note that feature flags alone cannot verify that crates compiled without the std features are truly no-std compliance, and the Substrate documentation do not explain further on how to verify on this.

What I found instead is that Substrate crates like sp-io seems to also register panic handlers when the std feature flag is absent, which conflicts with the panic handler in std. So the question becomes: Is the acceptance criteria for Substrate compliance such that it can import crates like sp-io without the std feature flags?

@soareschen
Copy link
Contributor Author

Related issue on checking for no_std support: rust-embedded/wg#64

@romac
Copy link
Member

romac commented Jul 12, 2021

Thanks for the write-up, @soareschen!

My understanding is that at the moment, we want both WebAssembly (wasm32-unknown-unknown ) and Substrate compliance (the latter seems to require the former).

The first three acceptance criteria are rather straightforward to verify. However I have not been able to find good source of verifying for no-std compliance on Substrate. The only information available is on the guide which shows how to configure Cargo.toml feature flags to switch between std and no_std modes. It is important to note that feature flags alone cannot verify that crates compiled without the std features are truly no-std compliance, and the Substrate documentation do not explain further on how to verify on this.

What I found instead is that Substrate crates like sp-io seems to also register panic handlers when the std feature flag is absent, which conflicts with the panic handler in std. So the question becomes: Is the acceptance criteria for Substrate compliance such that it can import crates like sp-io without the std feature flags?

@DaviRain-Su Can you please chime in here to provide with more details on this? Especially as to how we can test that the ibc and ibc-proto crates are compatible with Substrate?

@DaviRain-Su
Copy link
Contributor

DaviRain-Su commented Jul 17, 2021

Substrate compliance: Our crates can be built and run on the Substrate platform. The exact step to verify this needs to be clarified.
@DaviRain-Su Can you please chime in here to provide with more details on this? Especially as to how we can test that the ibc and ibc-proto crates are compatible with Substrate?

Regarding the validation on substrate, we use ibc to implement pallet (a term used in substrate) and the use of pallet is registered to the runtime, the final compilation output in substrate is native binary, and wasm files, these pallet files will be compiled into wasm files. files will also be compiled into wasm files. So the conclusion here is that the verification result on substrate is to set the compilation target to the WebAssembly target.

relate toutials: https://substrate.dev/docs/en/tutorials/add-a-pallet/import-a-pallet

his is important to enable the Substrate runtime to compile to both native binary, which supports Rust std, and Wasm binary, which do not (see: no_std).
The use of Wasm runtime binaries is one of Substrate's defining features. It allows the runtime code to become a part of a blockchain's evolving state; it also means that the definition of the runtime itself is subject to the cryptographic consensus mechanisms that ensure the security of the blockchain network. The usage of Wasm runtimes enables one of Substrate's most innovative features: forkless runtime upgrades, which means that Substrate blockchain nodes can stay up-to-date and even acquire new features without needing to be replaced with an updated application binary.

This means that the substrate platform can also be applicable as long as the requirements proposed in the previous three are suitable.

@adizere
Copy link
Member

adizere commented Jul 29, 2021

@DaviRain-Su many thanks for your support with no_std so far!
I just wanted to check with you on the timeline for no_std support in ibc-rs: If we're targeting end of September to close this issue, would that align with Octopus Network plans?

@DaviRain-Su
Copy link
Contributor

@DaviRain-Su many thanks for your support with no_std so far!
I just wanted to check with you on the timeline for no_std support in ibc-rs: If we're targeting end of September to close this issue, would that align with Octopus Network plans?

Oh, thanks, this time is in line with our expectations

@adizere adizere added the E: no-std External: support for no_std compatibility label Aug 16, 2021
@DaviRain-Su
Copy link
Contributor

@soareschen Hi, I would like to know how this issue is progressing now.

@soareschen
Copy link
Contributor Author

@DaviRain-Su I have some time to work on this for the next few weeks. Many changes have been merged and moving us closer to no_std support. However there are still quite a number of blockers, in particular on the no_std support from the dependencies. I have identified some of the main issues that still need to be addressed and updated the list in the description.

At the moment it is not clear how much time is needed to resolve the current issues, and how many new issues will still be uncovered.

@DaviRain-Su
Copy link
Contributor

@DaviRain-Su I have some time to work on this for the next few weeks. Many changes have been merged and moving us closer to no_std support. However there are still quite a number of blockers, in particular on the no_std support from the dependencies. I have identified some of the main issues that still need to be addressed and updated the list in the description.

At the moment it is not clear how much time is needed to resolve the current issues, and how many new issues will still be uncovered.

For substrate-ibc that depends on ibc-rs now compiles through in substrate, I think it can be merged into the main branch.

@DaviRain-Su
Copy link
Contributor

Hi @soareschen , I encountered some errors when integrating substrate-ibc to substrate-parachain-template , can you help me to take a look at it, my previous node-template on substrate was tested and passed, the difference between the two is the version of substrate. The version of substrate-parachain-template template is polkadot-v0.9.12.

This is error:

    Checking pallet-vesting v4.0.0-dev (https://github.com/paritytech/substrate?branch=polkadot-v0.9.12#d76f3999)
error[E0277]: the trait bound `ics23::ProofSpec: informalsystems_prost::Message` is not satisfied
  --> /Users/davirain/.cargo/git/checkouts/ibc-rs-b37d925b86d85d59/8da1b06/modules/src/ics23_commitment/specs.rs:40:36
   |
40 |             prost::Message::encode(ds, &mut encoded).unwrap();
   |             ---------------------- ^^ the trait `informalsystems_prost::Message` is not implemented for `ics23::ProofSpec`
   |             |
   |             required by a bound introduced by this call
   |
note: required by `informalsystems_prost::Message::encode`
  --> /Users/davirain/.cargo/registry/src/github.com-1ecc6299db9ec823/informalsystems-prost-0.8.1/src/message.rs:47:5
   |
47 | /     fn encode<B>(&self, buf: &mut B) -> Result<(), EncodeError>
48 | |     where
49 | |         B: BufMut,
50 | |         Self: Sized,
   | |____________________^

substrate-ibc: https://github.com/octopus-network/substrate-ibc.git and branch is dv-ibc-dev-0.9.12
substrate-parachain-template: https://github.com/octopus-network/substrate-parachain-template.git and branch is master

@mzabaluev
Copy link
Contributor

@DaviRain-Su Looks to me like informalsystems-prost version used to build the component does not match the version that is used in the generated impls. Try checking with cargo tree.

@DaviRain-Su
Copy link
Contributor

Looks to me like informalsystems-prost version used to build the component does not match the version that is used in the generated impls. Try checking with cargo tree.

Thx,I'll give it a try

@soareschen
Copy link
Contributor Author

Yes, I am updating this branch so that it will be using the same prost version as master.

@DaviRain-Su
Copy link
Contributor

Yes, I am updating this branch so that it will be using the same prost version as master.

Okay, when you fix it, you can @DaviRain-Su

@DaviRain-Su
Copy link
Contributor

@DaviRain-Su Looks to me like informalsystems-prost version used to build the component does not match the version that is used in the generated impls. Try checking with cargo tree.

After I run this command, I see that all the versions of this library(informalsystems-prost)us
ed are the same.

@soareschen
Copy link
Contributor Author

@DaviRain-Su the crate informalsystems-prost was a temporary fork we used to patch up bugs in prost. Since then new versions of prost have been released, and we have switched back to directly use prost instead. You'll need to remove the use of package = "informal-systems-prost" in Cargo.toml and update prost and tonic to the latest version.

I have updated #1390 to keep up to date to master. But now the no_std support is broken, with various new issues such as tendermint-rs/issues/1027 and #1612.

@soareschen
Copy link
Contributor Author

@DaviRain-Su please use the branch in #1613, which I have updated the original branch with new versions of prost and tonic.

As mentions, the support for no_std #1390 is currently broken. It may take a while before we can fix everything again. Just be aware that #1613 has been lagging behind master since October, and there have been many breaking changes made on master since then.

@DaviRain-Su
Copy link
Contributor

@DaviRain-Su please use the branch in #1613, which I have updated the original branch with new versions of prost and tonic.

As mentions, the support for no_std #1390 is currently broken. It may take a while before we can fix everything again. Just be aware that #1613 has been lagging behind master since October, and there have been many breaking changes made on master since then.

Yes because of this problem, I found out for a long time a few days ago because of this problem that the upper dependency of tendemint-rs has been modified, no-std has not compiled over, and now I have fixed tendermint-rs to a rev that can be run at that time, as a temporary development now.

@soareschen
Copy link
Contributor Author

Update: the full support for no_std with the latest version of ibc-rs is now available at #1846. There are just some minor fixes need to be upstreamed to ics23 and safe-regex. Once that is done we can get full support for no_std on master.

@soareschen
Copy link
Contributor Author

The master branch now fully supports no_std, albeit with some patches needed for the dependencies. This issue can now be closed with #1858 tracking the last bit of requirements for full support of no_std without requiring the patches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: no-std External: support for no_std compatibility
Projects
No open projects
Status: Needs PR
5 participants