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

no_std support #528

Closed
wants to merge 5 commits into from
Closed

no_std support #528

wants to merge 5 commits into from

Conversation

justinmoon
Copy link

@justinmoon justinmoon commented Dec 1, 2020

Use the alloc create, which requires the user define a global allocator.

  • I ignored old rust versions thus far. More research required to determine MSRV implications.
  • Import from core and alloc instead of std
  • Create std feature
  • Create use-bare-io feature which adds a
    bare-io dependency to polyfill
    std::io features. This is an experimental feature and should be
    used with extreme caution. Is using a 3rd party crate acceptable for this?
  • Created an io module to actually do ^^
  • Replace HashMap and HashSet with BTreeMap and BTreeSet just to
    get something working. This needs to be fixed.
  • CI runs tests no_std code

Closes #409

We use the [`alloc`](https://doc.rust-lang.org/alloc/) create, which requires the user define a global allocator.

* Import from `core` and `alloc` instead of `std`
* Create `std` feature
* Create `use-bare-io` feature which adds a
  [bare-io](https://github.com/bbqsrc/bare-io) dependency to polyfill
  `std::io` features. This is an experimental feature and should be
  used with extreme caution.
* Created an `io` module to polyfill `std::io` with `bare-io` in
  `no_std`.
* Replace `HashMap` and `HashSet` with `BTreeMap` and `BTreeSet` just to
  get something working. This needs to be fixed.
* CI runs tests `no_std` code
Cargo.toml Outdated

[dependencies]
base64-compat = { version = "1.0.0", optional = true }
bech32 = "0.7.2"
bitcoin_hashes = "0.9.0"
bitcoin_hashes = { git = "https://github.com/justinmoon/bitcoin_hashes.git", branch = "more_no_std", features = [], default-features = false }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -308,12 +309,14 @@ impl fmt::Display for Bip34Error {
}
}

impl ::std::error::Error for Bip34Error {}
#[cfg(feature = "std")]
impl std::error::Error for Bip34Error {}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what a lack of an Error impl in `no_std would mean here ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too much. The whole Error concept is std-only usually. The Result type doesn't require the second generic parameter to be Error, so it has very few effects.

pub use bare_io::*;

#[cfg(feature = "std")]
pub use std::io::*;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas on a better way to polyfill std::io?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What things need to be polyfilled exactly?

I mean at some level we can just feature-gate all stuff that needs std things, but I suppose your concern is that the fraction that would remain is so small that a polyfill could make more things no-std-compatible?

How does a polyfill version of Vec work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F.e. if on-stack Vec's have a hard length requirement, that may cause various errors in different places as we assume that Vec's have infinite space. (F.e. when writing/encoding into them, we always unwrap errors so a length limit would cause these to panic.)

Copy link
Author

@justinmoon justinmoon Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The polyfill is currently only for std::io. We're using alloc::Vec, which is exactly the same as std::Vec.

@justinmoon justinmoon mentioned this pull request Dec 1, 2020
3 tasks
@justinmoon
Copy link
Author

justinmoon commented Dec 4, 2020

I added another commit updating dependencies to get this running on real microcontrollers, as demonstrated in this repo.

As for MSRV,

  • The alloc crate didn't stabilize until 1.36.0. If we want rust-bitcoin to run on microcontrollers I believe we need to upgrade MSRV to 1.36.0.
  • The bare-io crate uses a non_exhaustive attribute which didn't stabilize until 1.40.0. I bet we could find a way around this one either by patching bare-io or implementing the std::io polyfill ourselves.

`std::collections::HashMap` and `std::collections::HashSet` don't have
any analogue in `alloc` because they require a OS RNG.

[Recently no_std-compatible implementations have been
merged](rust-lang/rust#58623) but I think we'd
need to massively increase our MSRV.

So I opted to move any `HasMap` or `HasSet` usage in tests to `BTreeMap` or `BTreeSet`, and to gate the only non-test usage of `HashMap` or `HashSet` (in the merkle block code which won't be helpful on MCUs) behind a "std" feature flag. Public API doesn't change.
Upgraded bare-io to a branch which adds a feature to disable the
`#[non_exhaustive]` attribute, which wasn't stabilized until 1.40.0.
@justinmoon
Copy link
Author

Now this works on stable 1.36.0, which is where alloc stabilized.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making modifications to tests is not needed IMO, running tests without std doesn't make sense.

For cases where HashMap, HashSet are needed outside of tests a trait can be used to avoid hard dependency on std.

@@ -834,7 +834,7 @@ mod tests {

#[test]
fn str_roundtrip() {
let mut unique = HashSet::new();
let mut unique = BTreeSet::new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be needed in tests, there's no point in making tests no_std

@@ -19,7 +19,8 @@
//! single transaction
//!

use std::default::Default;
use core::default::Default;
use alloc::vec::Vec;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's possible to avoid bumping MSRV if use like this is conditional. Assuming it's fine to have diferent MSRV for different features (I think it's reasonable).

@@ -52,8 +52,12 @@
//! assert_eq!(1, index[0]);
//! ```

#[cfg(feature="std")]
use std::collections::HashSet;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed here but not anywhere below, how it's possible?

@TheBlueMatt
Copy link
Member

It looks like this needs a rather significant rebase?

@justinmoon
Copy link
Author

justinmoon commented Jan 14, 2021

@TheBlueMatt Yea I'm going to tackle this over next week.

@sgeisler
Copy link
Contributor

Before rebasing this it probably would be best to look which dependencies need PRs merged and updates published first, otherwise I see you doing the same again in a month or two once all the dependencies are updated to have a no_std option.

@TheBlueMatt
Copy link
Member

Replace HashMap and HashSet with BTreeMap and BTreeSet just to get something working. This needs to be fixed.

I think it makes sense to take a hashbrown dependency for this, as @GeneFerneau did at lightningdevkit/rust-lightning#842

@h4x3rotab
Copy link

Any updates or help wanted? This would be very helpful for us.

@@ -23,6 +23,8 @@
//! software.
//!

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

@RCasatta RCasatta Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as suggested also by @Kixunil there is no need to prevent use of std in tests

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

@devrandom devrandom mentioned this pull request May 5, 2021
2 tasks
@dr-orlovsky
Copy link
Collaborator

After #603 got merged I assume this one can be closed

@apoelstra apoelstra closed this Sep 24, 2021
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

Successfully merging this pull request may close these issues.

no_std support
9 participants