Initial support for no_std platforms

[GeneFerneau]

First attempt to provide alternatives for std libraries in rust-lightning/lightning, allowing for use on no_std platforms.

Some blockers remain for making rust-lightning/lightning a fully no_std library, like std::sync::{Arc, Mutex}, std::error, std::io::*, and more.

[TheBlueMatt]

This is awesome. Thanks for working on it. A few notes:

• no, we don't need to bother with no_std in fuzzing, I think, I'm not aware of any fuzzers that care. That said, does hashbrown by default not randomize? If so it's probably good to use that in fuzzing anyway since it triggers false-positive branch coverage.
• can we use the trick from rust-bitcoin for core? Instead of conditional-compilation in every file's use statements, use core as std at the root of the crate and let files import from that (I think maybe you could create a dummy module at the root that pulls in the bits of std we still use).

[GeneFerneau]

This is awesome. Thanks for working on it. A few notes:

Thanks, glad you like :)

* no, we don't need to bother with no_std in fuzzing, I think, I'm not aware of any fuzzers that care. That said, does hashbrown by default not randomize? If so it's probably good to use that in fuzzing anyway since it triggers false-positive branch coverage.

Yeah, it caused a CI fail, too. Removed those changes for now.

* can we use the trick from rust-bitcoin for core? Instead of conditional-compilation in every file's use statements, use core as std at the root of the crate and let files import from that (I think maybe you could create a dummy module at the root that pulls in the bits of std we still use).

I can try something like that. One issue I see is that some of the replacements don't come from core (like hashbrown and alloc stuff).

[devrandom]

What is the downside to always using core for imports, and skipping the conditionals?

[GeneFerneau]

What is the downside to always using core for imports, and skipping the conditionals?

There really isn't one, but I wanted to put up the changes for discussion before just replacing.

The only minor things are the extra extern crate core, extern crate alloc, and extern crate hashbrown. Plus the hashbrown dependency.

[TheBlueMatt]

I'm also not sure if core meets our MSRV, at least some parts of what we use likely aren't in it at MSRV, so we need to condition it somehow. Also agree we'd ideally not take a hashbrown dependency for std builds, though it's less critical given std's hashmap is just a frozen version of hashbrown.

[codecov]

Codecov Report

Merging #842 (dd9f430) into main (e0986de) will increase coverage by 0.19%.
The diff coverage is 94.44%.

❗ Current head dd9f430 differs from pull request most recent head 7687a34. Consider uploading reports for the commit 7687a34 to get more accurate results

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
+ Coverage   90.44%   90.63%   +0.19%     
==========================================
  Files          59       50       -9     
  Lines       29785    26843    -2942     
==========================================
- Hits        26938    24329    -2609     
+ Misses       2847     2514     -333     

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	93.05% <ø> (-2.47%)	⬇️
lightning/src/chain/mod.rs	100.00% <ø> (ø)
lightning/src/ln/chan_utils.rs	97.34% <ø> (-0.01%)	⬇️
lightning/src/ln/channelmanager.rs	83.36% <ø> (-0.14%)	⬇️
lightning/src/ln/features.rs	98.91% <ø> (+0.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.02% <ø> (-0.05%)	⬇️
lightning/src/ln/functional_tests.rs	96.86% <ø> (+0.06%)	⬆️
lightning/src/ln/msgs.rs	88.62% <ø> (+0.33%)	⬆️
lightning/src/ln/onion_route_tests.rs	96.85% <ø> (+0.04%)	⬆️
lightning/src/ln/onion_utils.rs	94.90% <ø> (ø)
... and 65 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0986de...7687a34. Read the comment docs.

[GeneFerneau]

I'm also not sure if core meets our MSRV, at least some parts of what we use likely aren't in it at MSRV, so we need to condition it somehow.

I'm not sure either. What is the MSRV for rust-lightning? I think a feature flag is a good idea, regardless. There is ongoing work for making std::{error,io} no_std compatible, which obviously won't be available in stable Rust for a while.

Also agree we'd ideally not take a hashbrown dependency for std builds, though it's less critical given std's hashmap is just a frozen version of hashbrown.

Definitely, unless switching over to a full no_std library, that makes the most sense.

[TheBlueMatt]

I'm not sure either. What is the MSRV for rust-lightning?

We support 1.30+, but for no_std its fine if we have a different MSRV if we need to. Ideally only as much as we need to, though.

[TheBlueMatt]

Note that we also need no_std support in rust-bitcoin, which should be much easier (no need to provide stubs for Mutex like here). See rust-bitcoin/rust-bitcoin#409 and the PR at rust-bitcoin/rust-bitcoin#528

[GeneFerneau]

Note that we also need no_std support in rust-bitcoin, which should be much easier (no need to provide stubs for Mutex like here). See rust-bitcoin/rust-bitcoin#409 and the PR at rust-bitcoin/rust-bitcoin#528

Nice! Will read through the PR and issue, thanks.

core2 mentioned in rust-bitcoin/rust-bitcoin#409 looks like a good possibilty for std::{error, io} stuff. The MSRV bump is substantial.

I can play around with core2, and see how it goes. Maybe open a separate PR considering the MSRV is 1.47+ (maybe higher once const generics stabilize).

Edit: attempted using core2, but ran into issues. Still debugging to see if it is a viable future option, since it looks like a promising effort.

[GeneFerneau]

Cleaned up the conditional compilation by moving all the feature flag stuff into lightning/src/lib.rs. Also caught some things I missed previously.

Let me know what you think. If you like it, I can squash it into one commit.

[TheBlueMatt]

Nice! Looking much better. Sorry to do this to you, I'm not actually sure we can't just "use core::*" in most places - I was checking how bitcoin_hashes did it (rust-bitcoin/bitcoin_hashes#52) and it looks like that mostly worked even with the same MSRV. If not, can we keep the same module structure (ie instead of just std::HashMap, keep it as std::collections::HashMap) then the diff size should be really trivial/more automated.

[GeneFerneau]

Nice! Looking much better. Sorry to do this to you, I'm not actually sure we can't just "use core::*" in most places - I was checking how bitcoin_hashes did it (rust-bitcoin/bitcoin_hashes#52) and it looks like that mostly worked even with the same MSRV. If not, can we keep the same module structure (ie instead of just std::HashMap, keep it as std::collections::HashMap) then the diff size should be really trivial/more automated.

For the stuff that has a 1-to-1 between std and core, I just use core now. There are a number of things in std::collections that don't have a 1-to-1 with alloc::collections, and stuff like alloc::vec::Vec/alloc::boxed::Box that I just import where they are used.

I also did the #![cfg_attr(feature = "no_std", no_std] thing, so running cargo check --features no_std will spit out the errors for the remaining pieces of std left. Mostly io and sync. Some of the sync types look like they're only used for tests, so can probably move all of that to its own module, and put it behind a feature flag.

Will look at the remaining std usage to see what can be stubbed and/or refactored.

Marked as draft for now, until the remaining std stuff is resolved.

[TheBlueMatt]

Awesome! Just skimmed the diff but this looks great. I think sync is all either Mutex (which we'll need to provide a stub for that uses RefCell under the hood) or atomic (which I think is safe in no_std).

[TheBlueMatt]

We've bumped our MSRV so we should be able to just use alloc directly now. The rest of rust-bitcoin is likely to do the same in the coming months.

[GeneFerneau]

We've bumped our MSRV so we should be able to just use alloc directly now. The rest of rust-bitcoin is likely to do the same in the coming months.

Awesome! I've been focused on other work. Will likely be able to dedicate some time to updating this PR in the next couple weeks.

[devrandom]

I believe that cargo features are supposed to be additive. The standard seems to be for libraries to have a feature named std, which makes the library add std features, rather than going the other way.

If the concern is that hashbrown will be linked in unnecessarily for the std case, if there is no use statement that actually uses it, it will get optimized out.

[TheBlueMatt]

Hmm, that's an interesting question - yes, an std feature is standard (and used in other rust-bitcoin crates), but in this case it's also "additive" to switch to hashbrown (arguably you could use it in std as well because it has a different default hasher and route finding may be faster with it). I don't really want to rely on the optimizer removing a dependency, as it's easy to link in __attribute__((constructor)) code and bypass that, through I admit it's a somewhat weaker case here given we're still recommending some users use hashbrown. Mostly rust's feature system is about 1/10th as expressive as we generally need it to be :/. I don't think this is cut-and-dry, but let me know what you think.

[devrandom]

That argument cuts both ways - if we depend on a crate for the ! no_std case, then you'll have to rely on the optimizer to optimize it out for the no_std case. Admittedly, there isn't such a crate yet for now...

[TheBlueMatt]

Right - because the features interface is so limited we kinda have to take things on a case-by-case basis :(.

[GeneFerneau]

I had thought about this off and on over the past few weeks, and agree with @devrandom that renaming to std is a good idea.

Will include this in the fixups when updating the branch on the latest main.

[devrandom]

I wonder if it makes sense to have both a std feature and a no_std feature and insist that the user choose one if they turn off default. I tried it out in rust-bitcoin/rust-bitcoin#603

The advantage is that any dependency that only applies to one of them stays optional, which is what we want.

[devrandom]

Actually, applying to this case, an std feature would have no consequence, so we're back to what you proposed.

[devrandom]

Coming back to this conversation, in #1028 it ends up that we do need an std feature, to pull in std from dependencies. So looks like we'll end up with both std and no_std features.

[GeneFerneau]

spin crate looks like a good replacement for std::sync::{Mutex, RwLock}, though there is a warning about doing a direct drop-in.

There is also current work on a mutex trait (embedded-wg/RFC-0377) for supported embedded architectures. Maybe spin or bare-metal is a good placeholder until the mutex trait work is done?

[TheBlueMatt]

spin crate looks like a good replacement for std::sync::{Mutex, RwLock}, though there is a warning about doing a direct drop-in.

I don't think we should bother. I think we should simply not support multi-threading in a no_std environment, using RefCells instead. The following is basically what I was thinking:

use std::ops::DerefMut;
use std::sync::{Mutex, MutexGuard};
use std::cell::{RefMut, RefCell};

// Trait impls:

trait Lock<'a, T>: DerefMut<Target=T> + 'a {}
trait Locker<'a, T> {
    type Thing: Lock<'a, T>;
    fn lock_me(&'a self) -> Self::Thing;
}

impl<'a, T: 'a> Lock<'a, T> for MutexGuard<'a, T> {}
impl<'a, T: 'a> Locker<'a, T> for Mutex<T> {
    type Thing = MutexGuard<'a, T>;
    fn lock_me(&'a self) -> MutexGuard<'a, T> {
        self.lock().unwrap()
    }
}

impl<'a, T: 'a> Lock<'a, T> for RefMut<'a, T> {}
impl<'a, T: 'a> Locker<'a, T> for RefCell<T> {
    type Thing = RefMut<'a, T>;
    fn lock_me(&'a self) -> RefMut<'a, T> {
        self.borrow_mut()
    }
}

// Note that the demo code works with either mutex module
pub(crate) mod mutex {
    pub(crate) type MaybeMutex<T> = std::sync::Mutex<T>;
    pub(crate) type MaybeMutexGuard<'a, T> = std::sync::MutexGuard<'a, T>;
}
/*
pub(crate) mod mutex {
    pub(crate) type MaybeMutex<T> = std::cell::RefCell<T>;
    pub(crate) type MaybeMutexGuard<'a, T> = std::cell::RefMut<'a, T>;
}*/
use mutex::*;

// Demo code to show it works

struct B { b: u32 }
struct A {
    thing: MaybeMutex<B>,
}
impl A {
    fn set_val(&self, b: u32) {
        self.thing.lock_me().b = b;
    }
}

fn do_thing<'a>(mut guard: MaybeMutexGuard<'a, B>) {
    guard.b = 30;
}

fn do_another_thing(a: &A) {
    let mut lock = a.thing.lock_me();
    assert_eq!(lock.b, 30);
    lock.b = 42;
}

fn main() {
    let a = A{ thing: MaybeMutex::new(B {b: 1}) };
    let lock = a.thing.lock_me();
    do_thing(lock);
    do_another_thing(&a);
    assert_eq!(a.thing.lock_me().b, 42);
    a.set_val(44);
    assert_eq!(a.thing.lock_me().b, 44);
}

[GeneFerneau]

I don't think we should bother. I think we should simply not support multi-threading in a no_std environment, using RefCells instead.

This definitely makes sense for platforms like hardware wallets, and I like your approach to stubbing out the mutex functionality into a new struct.

I understand not supporting multithreading for some no_std platforms (single-core boards), but other platforms would greatly benefit from the performance boost.

What are your thoughts on adding another feature for multithreaded no_std support (for platforms like RaspberryPi and other SBC)? This could come in a later PR, and extend the MaybeMutex struct. We could even do the MaybeMutex as a trait, and impl for different feature-sets / platforms. Starts to go in the direction of the Mutex trait being worked on upstream by the embedded-wg, though, which is why I brought that up.

Maybe at the point the embedded-wg work is ready, all the feature-gated Mutex stuff in rust-lightning gets replaced? Either way, I think starting simple with something like the example you gave is a good idea. Can always extend in the future.

[TheBlueMatt]

for platforms like RaspberryPi and other SBC

RPi and other SBC generally run Linux, so don't need no_std support?

Starts to go in the direction of the Mutex trait being worked on upstream by the embedded-wg, though, which is why I brought that up.

Ah, wasn't aware of that. In any case, indeed, once upstream has support for what we want we can use that. In the mea time, I don't really think we need to bother supporting multi-core embedded hardware, its just too uncommon - mostly by the time you get to a fast enough CPU and multiple cores you are running a full OS.

[GeneFerneau]

Cleaned up the code, and removed a lot of conditional compilation stuff.

Didn't attempt the Arc, Mutex[Guard], RwLock[Guard], etc. stuff, yet. That's going to be a little more intensive. The stub solution for Mutex is simple enough, but would need to be repeated for the other std::sync stuff.

Continuing to review all the uses of the multithreading primitives to try to find a good solution that won't take a ton of effort. Maybe putting all of it in a stub crate, since there doesn't seem to be a single replacement crate that covers all of them.

Will use core_io for all the std::io uses in the next batch of updates.

[devrandom]

FYI, submitted rust-bitcoin/rust-secp256k1#300 to allow use of Secp256k1::new

[TheBlueMatt]

Will use core_io for all the std::io uses in the next batch of updates.

You may be interested in the discussion at rust-bitcoin/bitcoin_hashes#127 which is likely to include relevant traits we can use instead of std::io (or core2 directly).

[TheBlueMatt]

At @devrandom's suggestion, I think we could go ahead and land the big s/std/alloc/ rename on imports if we split that out into a standalone PR, which would make this PR much smaller.

[GeneFerneau]

At @devrandom's suggestion, I think we could go ahead and land the big s/std/alloc/ rename on imports if we split that out into a standalone PR, which would make this PR much smaller.

Opened a PR that splits out alloc changes in #924

Do we also want a separate PR for replacing HashMap and friends with hashbrown?

[TheBlueMatt]

Do we also want a separate PR for replacing HashMap and friends with hashbrown?

Lets at least do a PR with the mass rename of use std to use core. That will be the majority of the diff, I think, and good to get that out of the way.

[TheBlueMatt]

Any interest in splitting the locking stuff out into its own PR, maybe using the suggested locking traits above? It looks like rust-bitcoin is moving pretty quick towards no_std.

[TheBlueMatt]

Superseded by #1028, which is now merged!

[devrandom]

thank you @GeneFerneau for getting this going