no_std support, keeping MSRV #603

devrandom · 2021-05-05T13:08:45Z

TODO:

needs release of bitcoin_hashes
needs release of rust-bech32

Based on the original work by Justin Moon in #528

MSRV unchanged from 1.29.0.

When std is off the no-std feature must be turned on, which turns on the alloc crate, which requires the user define a global allocator. It also pulls in the hashbrown crate and activates no_std and alloc support in a couple of dependencies.

Import from core and alloc instead of std
alloc only used if std is off
Create std feature
Create no-std feature which adds a core2 dependency to polyfill std::io features. This is an experimental feature and should be used with caution.
CI runs tests no_std code
MSRV of no_std is 1.51 or so

Fixes #409

TheBlueMatt · 2021-05-05T13:48:50Z

Cargo.toml


 base64-compat = { version = "1.0.0", optional = true }
 bitcoinconsensus = { version = "0.19.0-1", optional = true }
 serde = { version = "1", features = [ "derive" ], optional = true }
+hashbrown = "0.11.2"


Can we feature-guard hashbrown somehow? We can probably just not export chunks of the library that rely on a HashMap, most of the library is still useful without it.

I pushed a couple of commits to do this, let me know what you think of the approach

src/io.rs

devrandom · 2021-05-05T16:18:49Z

Need CI use approval.

elichai · 2021-05-05T17:25:26Z

Need CI use approval.

I Approved the CI to run

TheBlueMatt · 2021-05-05T20:31:39Z

I Approved the CI to run

We have to do it every time any new push happens. I have zero clue what this is even intended to solve, but its super obnoxious. Obnoxious enough that I'd start thinking about something other than GH actions at this point for CI.

devrandom · 2021-05-06T07:53:03Z

CI should be fixed with latest push. Sorry for the hassle.

devrandom · 2021-05-06T10:08:19Z

And one more...

devrandom · 2021-05-06T17:00:58Z

OK, looks like CI passes, but there is some strangeness in the display with three additional line items showing as "expected".

devrandom · 2021-05-07T08:39:22Z

What do we think about bumping the MSRV to edition 2018 / 1.31.0?

This would affect rust-lightning too, since it is at 1.30.0.

sgeisler · 2021-05-07T12:46:58Z

What do we think about bumping the MSRV to edition 2018 / 1.31.0?

I think it's planned in the next few months (#510 (comment)).

src/io.rs

TheBlueMatt

Generally looks good. I think we should wait for/work on rust-bitcoin/bitcoin_hashes#126 or rust-bitcoin/bitcoin_hashes#127 first and then use that here.

devrandom · 2021-05-15T15:15:01Z

In the meantime, I added an embedded test, similar to the one in bitcoin_hashes. 512KB minimum is required because of the amount of stack that secp256k1 requires to build a context, even in lowmemory mode.

devrandom · 2021-05-25T11:50:30Z

~~switch from take() to CappedRead as discussed in rust-bitcoin/bitcoin_hashes#127 (comment)~~

devrandom · 2021-05-27T15:11:59Z

So HashEngine from bitcoin_hashes doesn't implement core2::io::Write yet. I assume the plan is to implement that? Should I go ahead and open a PR?

TheBlueMatt

High level looks good. Can you squash up the commits and fix the FIXMEs? Lets land this!

fuzz/fuzz_targets/deserialize_address.rs

TheBlueMatt · 2021-05-27T15:29:23Z

So HashEngine from bitcoin_hashes doesn't implement core2::io::Write yet. I assume the plan is to implement that? Should I go ahead and open a PR?

Yea, probably go ahead and open a PR there or chat with @RCasatta about doing it. Maybe we can simplify this PR to export fewer things for now and land it quicker, then we can expand the things exposed in no_std over time?

devrandom · 2021-05-28T10:41:54Z

Opened rust-bitcoin/bitcoin_hashes#128 and adjusted this one to point to that

sgeisler · 2021-06-27T13:29:55Z

Ah, sorry for another remark, I was just going through my notifications and saw that there is a non-alpha release of core2 now (bbqsrc/core2#4) which we should use here imo.

devrandom · 2021-06-27T15:42:12Z

Ah, sorry for another remark, I was just going through my notifications and saw that there is a non-alpha release of core2 now (bbqsrc/core2#4) which we should use here imo.

Ah, great. But we'll have to upgrade and release bitcoin_hashes first, otherwise there's a semver version conflict.

TheBlueMatt · 2021-07-05T22:52:26Z

An updated version of bitcoin_hashes has been published. Can we use the latest core2 now?

devrandom · 2021-07-06T09:14:36Z

updated deps, rebased and squashed

TheBlueMatt

One question, two tiny comments, and I'm otherwise happy :)

TheBlueMatt · 2021-07-06T15:02:46Z

contrib/test.sh

-    alias cargo="cargo +$TOOLCHAIN"
-fi
+echo "********* Testing std *************"
+# Test without any features first.  std is required for tests


If we're just gonna require std for tests, I'm a bit confused why a bunch of #[cfg(test)] blocks have std/no-std feature checks. Is it just that we intend to support no_std tests but don't yet and its WIP to be done in a followup?

Comment was wrong, and we do indeed test with no-std below. Fixed.

src/util/merkleblock.rs

Cargo.toml

TheBlueMatt

LGTM 🎉

TheBlueMatt · 2021-07-14T20:21:47Z

Note that this now requires a major version bump as its "breaking" to update the bitcoin_hashes dependency. I think that's fine and don't really care about the merkle block deprecated functions, just wanted to flag it.

src/util/mod.rs

Based on the original work by Justin Moon. *MSRV unchanged from 1.29.0.* When `std` is off, `no-std` must be on, and we use the [`alloc`](https://doc.rust-lang.org/alloc/) and core2 crates. The `alloc` crate requires the user define a global allocator. * Import from `core` and `alloc` instead of `std` * `alloc` only used if `no-std` is on * Create `std` feature * Create `no-std` feature which adds a core2 dependency to polyfill `std::io` features. This is an experimental feature and should be used with caution. * CI runs tests `no-std` * MSRV for `no-std` is 1.51 or so

devrandom · 2021-07-15T07:09:19Z

Note that this now requires a major version bump as its "breaking" to update the bitcoin_hashes dependency. I think that's fine and don't really care about the merkle block deprecated functions, just wanted to flag it.

@TheBlueMatt to clarify, are you saying that we could remove the deprecated funcs but you don't feel strongly about it? (I don't feel strongly either)

TheBlueMatt

@TheBlueMatt to clarify, are you saying that we could remove the deprecated funcs but you don't feel strongly about it? (I don't feel strongly either)

Yes.

sgeisler

utACK bba57d7

RCasatta

I did a couple of tests limiting memory with the embedded test:

with 64KB of memory the process hangs forever with the CPU looping
with 128KB it panics with panic Some([libsecp256k1] illegal argument. rustsecp256k1_v0_4_0_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx))
with 256KB works correctly (secp buf size 66240 bytes)

embedded/scripts/env.sh

RCasatta · 2021-07-16T12:22:37Z

embedded/scripts/env.sh

@@ -0,0 +1,2 @@
+export RUSTFLAGS="-C link-arg=-Tlink.x"
+export CARGO_TARGET_THUMBV7M_NONE_EABI_RUNNER="qemu-system-arm -cpu cortex-m3 -machine mps2-an385 -m 1G -nographic -semihosting-config enable=on,target=native -kernel"


-m 1G on my host complain saying maximum memory for this machine is 16Mib, removing the parameter seems to work fine

qemu-system-arm: Invalid RAM size, should be 16 MiB

Curious what is your qemu and OS version? does it work with -m 16m?

I'm thinking of changing this to -m 1m, but it would be good to know the answer to the above to make sure it works in your env too.

yeah, sorry.

On macOS Catalina with QEMU emulator version 5.2.0 it works only with -m 16m or without the option.

It doesn't work with -m 1m or -m 1G

OK. Ubuntu 20.04 has qemu 4.2 and it also works without the flag. So I think I'll remove the flag if there are any other changes, or wait to a followup PR.

RCasatta · 2021-07-16T12:30:19Z

embedded/src/main.rs

+#[global_allocator]
+static ALLOCATOR: CortexMHeap = CortexMHeap::empty();
+
+const HEAP_SIZE: usize = 1024 * 512; // 512 KB


all memory available is 512Kb according to memory.x so the heap should be less, eg 256KB

Probably not worth dismissing approvals only for this

apoelstra

ack bba57d7

apoelstra · 2021-07-20T16:40:02Z

Gonna merge this. We can address @RCasatta comment in a folllowup PR (and maybe make other changes to work with tiny amounts of RAM)

devrandom changed the title ~~WIP: no_std support, keeping MSRV~~ Draft: no_std support, keeping MSRV May 5, 2021

devrandom marked this pull request as draft May 5, 2021 13:09

TheBlueMatt reviewed May 5, 2021

View reviewed changes

devrandom force-pushed the no_std_devrandom branch 2 times, most recently from 55e2a03 to 9f79ab9 Compare May 5, 2021 16:09

devrandom mentioned this pull request May 5, 2021

Initial support for no_std platforms lightningdevkit/rust-lightning#842

Closed

RCasatta mentioned this pull request May 6, 2021

new literacy::{Read, Write} traits to handle std/no_std rust-bitcoin/bitcoin_hashes#126

Closed

devrandom force-pushed the no_std_devrandom branch from dec4ab4 to ff18a01 Compare May 8, 2021 07:27

TheBlueMatt reviewed May 10, 2021

View reviewed changes

src/io.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 10, 2021

View reviewed changes

devrandom force-pushed the no_std_devrandom branch from 866016d to 1805550 Compare May 15, 2021 15:21

devrandom force-pushed the no_std_devrandom branch 3 times, most recently from 4e267b6 to c715916 Compare May 26, 2021 16:19

TheBlueMatt reviewed May 27, 2021

View reviewed changes

fuzz/fuzz_targets/deserialize_address.rs Outdated Show resolved Hide resolved

devrandom force-pushed the no_std_devrandom branch from e930a41 to 2e69c77 Compare May 28, 2021 11:52

devrandom mentioned this pull request May 28, 2021

Add core2 support rust-bitcoin/bitcoin_hashes#128

Merged

devrandom mentioned this pull request Jun 27, 2021

core2 0.3.0 was released rust-bitcoin/bitcoin_hashes#131

Merged

devrandom dismissed sgeisler’s stale review via 2d416dd June 27, 2021 17:06

devrandom force-pushed the no_std_devrandom branch from 2d416dd to c50a5f2 Compare July 6, 2021 08:49

TheBlueMatt reviewed Jul 6, 2021

View reviewed changes

devrandom force-pushed the no_std_devrandom branch from c50a5f2 to 8af260e Compare July 7, 2021 07:50

Kixunil reviewed Jul 7, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

devrandom force-pushed the no_std_devrandom branch from 8af260e to f4ec84c Compare July 7, 2021 18:47

TheBlueMatt previously approved these changes Jul 14, 2021

View reviewed changes

sgeisler reviewed Jul 14, 2021

View reviewed changes

src/util/mod.rs Outdated Show resolved Hide resolved

devrandom added 2 commits July 15, 2021 09:04

Embedded test via qemu

bba57d7

devrandom dismissed TheBlueMatt’s stale review via bba57d7 July 15, 2021 07:05

devrandom force-pushed the no_std_devrandom branch from f4ec84c to bba57d7 Compare July 15, 2021 07:05

TheBlueMatt approved these changes Jul 15, 2021

View reviewed changes

sgeisler approved these changes Jul 15, 2021

View reviewed changes

RCasatta reviewed Jul 16, 2021

View reviewed changes

sgeisler mentioned this pull request Jul 19, 2021

Adds Taproot BIP341 signature message and create a unified sighash cache for legacy, segwit and taproot inputs #628

Merged

apoelstra approved these changes Jul 20, 2021

View reviewed changes

apoelstra merged commit 5e6b56a into rust-bitcoin:master Jul 20, 2021

devrandom mentioned this pull request Aug 2, 2021

no-std cleanup #637

Merged

dr-orlovsky mentioned this pull request Aug 9, 2021

no_std support #528

Closed

mcroad mentioned this pull request Oct 13, 2021

no_std support bitcoindevkit/bdk#205

Closed

3 tasks

sanket1729 mentioned this pull request Oct 17, 2021

no_std support rust-bitcoin/rust-miniscript#201

Closed

		@@ -0,0 +1,2 @@
		export RUSTFLAGS="-C link-arg=-Tlink.x"
		export CARGO_TARGET_THUMBV7M_NONE_EABI_RUNNER="qemu-system-arm -cpu cortex-m3 -machine mps2-an385 -m 1G -nographic -semihosting-config enable=on,target=native -kernel"

no_std support, keeping MSRV #603

no_std support, keeping MSRV #603

Conversation

devrandom commented May 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom commented May 5, 2021

elichai commented May 5, 2021 • edited

TheBlueMatt commented May 5, 2021

devrandom commented May 6, 2021

devrandom commented May 6, 2021

devrandom commented May 6, 2021

devrandom commented May 7, 2021 • edited

sgeisler commented May 7, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

devrandom commented May 15, 2021

devrandom commented May 25, 2021 • edited

devrandom commented May 27, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 27, 2021

devrandom commented May 28, 2021

sgeisler commented Jun 27, 2021

devrandom commented Jun 27, 2021

TheBlueMatt commented Jul 5, 2021

devrandom commented Jul 6, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 14, 2021

devrandom commented Jul 15, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

RCasatta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCasatta Jul 18, 2021 • edited

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 20, 2021

devrandom commented May 5, 2021 •

edited

elichai commented May 5, 2021 •

edited

devrandom commented May 7, 2021 •

edited

devrandom commented May 25, 2021 •

edited

RCasatta Jul 18, 2021 •

edited