Better no-std support by evanlinjin · Pull Request #894 · bitcoindevkit/bdk

evanlinjin · 2023-03-16T04:42:48Z

This replaces #893

Description

Carrying over relevant maintenance changes from release 0.27.2 to bdk master.

Use default-features = false for miniscript and bitcoin
Introduce std features for bdk, bdk_chain and bdk_esplora

Notes to the reviewers

The default-features = falsefor bitcoin and miniscript is to let bdk be unbiased for things like no-std.

Changelog notice

Set default-features = false for rust-bitcoin and rust-miniscript Better no-std support #894
Introduce std features for bdk, bdk_chain and bdk_esplora

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

LLFourn · 2023-03-17T09:53:09Z

 [features]
 default = ["std", "miniscript"]
-std = []
+std = ["bitcoin/std", "miniscript/std"]


❌ I don't think we need miniscript/std for anything. This will unnecessarily add miniscript as a dependency if someone enables the std feature.

LLFourn · 2023-03-17T10:04:09Z

 default = ["std", "miniscript"]
-std = []
+std = ["bitcoin/std", "miniscript/std"]
+no-std = ["hashbrown", "bitcoin/no-std", "miniscript/no-std"]


Why does rust bitcoin and rust miniscript make no-std a feature :S

I am looking at the source of rust miniscript and rust bitcoin and it looks like the no-std feature doesn't do anyhing (absence of std makes it no_std)`

Let's not add this feature if this is the case.

Since there's no way to enable the no-std dependencies due to the absence of std I don't see another way. But maybe we could add these only to the crates/bdk project, and just document what dependencies need to be manually added for anyone using the bdk_chain in a no std project other than with bdk?

Since there's no way to enable the no-std dependencies due to the absence of std I don't see another way.

Ok so I see that hashbrown needs to be enabled to have HashMap. I think what we do in bdk_chain is the right way to solve this specific problem: Use BTreeMap instead unless hashbrown feature is enabled which makes it faster by using an actual hashbrown hashmap. If that's too offensive then just make hashbrown a mandatory dependency. That seems way less harmful than this feature flag.

I think adding this feature just to enable features in another crate is not a good solution:

Crates depending on bdk will have to do the same thing and so the no-std thing spreads everywhere.

It means --all-features enabling std and no-std which just seems awful.

I think just telling people that they have to manually depend on bitcoin and or miniscript and enable their no-std feature manually is probably the right way to go for now. I hope we can remove no-std upstream.

Ok, I'm on-board with not adding a no-std feature and just documenting what needs to be manually enabled by downstream projects who are building for that environment. I think no-std is going to be uncommon/advance user thing anyway and certainly a lower support priority relative to the rest of the 1.0.0 changes going on. I'll add an issue to document how to use bdk in a no std environment and then this PR can be just the changes for std and bumping esplora-client, how does that sound?

@tcharding gave us some good advice:

we can just always enable the no-std feature and optionally enable std.

[edit] this solves the problem for rust-bitcoin only. I wonder if we can do the same thing for rust-miniscript too.

I had a quick look and I rekon it should work for miniscript also.

evanlinjin · 2023-03-22T03:25:34Z

@notmandatory Do you want to take over this ticket? I think you are allowed to directly push to my branch.

notmandatory · 2023-03-26T22:47:05Z

A good explanation of how to verify your crate works with no_std here: https://github.com/KodrAus/rust-no-std

I'm expanding the scope of this a bit tackle #920 also.

notmandatory · 2023-03-27T01:38:42Z

I ran into a couple issues with getting no_std to work with bdk_chain.

alloc::collections::BTreeMap<K,V> requires K to be Sized. So in SpkTxOutIndex I changed K to a Box<Script>
I don't know what downstream issues this will cause, any concerns using a Box<Script>?
in bitcoin 0.29.x sync needs to be changed to alloc::sync, this looks like it was fixed in bitcoin 0.30.0.
I opened Update bitcoin and miniscript dependencies #929.

notmandatory · 2023-03-27T01:42:07Z

BTW, I'm testing no_std building like this:

% cargo build -p bdk_chain --no-default-features --features bitcoin/no-std,hashbrown --target thumbv6m-none-eabi

tcharding · 2023-03-27T22:24:55Z

I ran into a couple issues with getting no_std to work with bdk_chain.

alloc::collections::BTreeMap<K,V> requires K to be Sized. So in SpkTxOutIndex I changed K to a Box<Script>

In case its useful, as of rust-bitcoin 0.30.0 we have ScriptBuf which is Sized.

LLFourn · 2023-03-28T04:01:07Z

@tcharding so my understanding is that Script is renamed to ScriptBuf and Script is now way to operate on a slice representing a script. I wasn't paying enough attention at the time but if I was I would have suggested leaving Script and making ScriptSlice for the new type to avoid breaking everything?

In any case we need to s/Script/ScriptBuf/g across the project.

tcharding · 2023-03-28T21:42:55Z

Correct. We used Script/ScriptBuf to mimic types in the stdlib (Path/PathBuf), out of interest do types exist named as you suggest Foo/FooSlice?

benthecarman · 2023-04-03T18:30:04Z

@@ -13,7 +13,7 @@ readme = "README.md"

 [dependencies]
 bdk_chain = { path = "../chain", version = "0.3.1", features = ["serde", "miniscript"] }


You need default features off on bdk_chain here too

benthecarman · 2023-04-03T19:26:12Z

Not sure if this is from an issue on master or not but seems to break if you do

$ cargo check --target wasm32-unknown-unknown --no-default-features --features no-std --features dev-getrandom-wasm

I get this error

    Checking bdk_chain v0.3.1 (/home/ben/projects/bdk/crates/chain)
error[E0277]: `RefCell<core::option::Option<Arc<TaprootSpendInfo>>>` cannot be shared between threads safely
   --> crates/chain/src/keychain/txout_index.rs:566:6
    |
566 | ) -> impl Iterator<Item = (u32, Script)> + Clone + Send + 'a
    |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `RefCell<core::option::Option<Arc<TaprootSpendInfo>>>` cannot be shared between threads safely
    |
    = help: within `Descriptor<DescriptorPublicKey>`, the trait `Sync` is not implemented for `RefCell<core::option::Option<Arc<TaprootSpendInfo>>>`
    = note: required because it appears within the type `Mutex<Option<Arc<TaprootSpendInfo>>>`
    = note: required because it appears within the type `Tr<DescriptorPublicKey>`
    = note: required because it appears within the type `Descriptor<DescriptorPublicKey>`
    = note: required for `&Descriptor<DescriptorPublicKey>` to implement `Send`
    = note: required because it appears within the type `Cow<'_, Descriptor<DescriptorPublicKey>>`
note: required because it's used within this closure
   --> crates/chain/src/keychain/txout_index.rs:579:13
    |
579 |             move |index| -> Result<_, miniscript::descriptor::ConversionError> {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: required because it appears within the type `Map<TakeWhile<TakeWhile<R, [closure@txout_index.rs:575:21]>, [closure@txout_index.rs:577:21]>, [closure@txout_index.rs:579:13]>`
    = note: required because it appears within the type `TakeWhile<Map<TakeWhile<TakeWhile<R, [closure@txout_index.rs:575:21]>, [closure@txout_index.rs:577:21]>, [closure@txout_index.rs:579:13]>, fn(&Result<(u32, Script), ConversionError>) -> bool {Result::<(u32, Script), ConversionError>::is_ok}>`
    = note: required because it appears within the type `Map<TakeWhile<Map<TakeWhile<TakeWhile<R, [closure@txout_index.rs:575:21]>, [closure@txout_index.rs:577:21]>, [closure@txout_index.rs:579:13]>, fn(&Result<(u32, Script), ConversionError>) -> bool {Result::<(u32, Script), ConversionError>::is_ok}>, fn(Result<(u32, Script), ConversionError>) -> (u32, Script) {Result::<(u32, Script), ConversionError>::unwrap}>`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `bdk_chain` (lib) due to previous error

notmandatory · 2023-04-04T00:22:19Z

I'm stumped too. I get the same error as above (error[E0277]: RefCell<core::option::Option<Arc<TaprootSpendInfo>>> cannot be shared between threads safely --> crates/chain/src/keychain/txout_index.rs:566:6) with the build command:

cargo build -p bdk_chain --no-default-features --features miniscript/no-std

But the same build command with std enabled works fine:

cargo build -p bdk_chain --no-default-features --features miniscript/std

I don't understand why the TaprootSpendInfo type becomes Send + Sync when std is enabled but not with no-std.

LLFourn · 2023-04-04T04:18:01Z

I'm stumped too. I get the same error as above (error[E0277]: RefCell<core::option::Option<Arc<TaprootSpendInfo>>> cannot be shared between threads safely --> crates/chain/src/keychain/txout_index.rs:566:6) with the build command:
cargo build -p bdk_chain --no-default-features --features miniscript/no-std
But the same build command with std enabled works fine:
cargo build -p bdk_chain --no-default-features --features miniscript/std
I don't understand why the TaprootSpendInfo type becomes Send + Sync when std is enabled but not with no-std.

Probably they use a refcell when they don't have access to std::sync::Mutex. RefCell will not be Sync. This shouldn't be a problem and #927 might fix this (or at least it would be a first step).

notmandatory · 2023-04-04T16:12:18Z

Probably they use a refcell when they don't have access to std::sync::Mutex. RefCell will not be Sync. This shouldn't be a problem and #927 might fix this (or at least it would be a first step).

Ok, you are 100% right, in rust-miniscript I found where they swap in RefCell for Mutex when not(feature="std"). https://github.com/rust-bitcoin/rust-miniscript/blob/4a3ba11c2fd5063be960741d557f3f7a28041e1f/src/lib.rs#L795

tcharding · 2023-04-04T21:42:04Z

cc @sanket1729 in case he wants to chime in.

notmandatory · 2023-04-05T00:02:29Z

I have a work-around for the Mutex -> RefCell issue, I removed the Send trait requirements of the range_descriptor_spks() function. I'll need @danielabrozzoni to let me know how badly this butchers her code 😬.

https://github.com/bitcoindevkit/bdk/pull/894/files#diff-4f6395be13ef1a3eabfcbd5303acdd487fa20097ab838581778076dd8df29f53

LLFourn · 2023-04-09T21:41:39Z

I think range_descriptor_spks simply needs to be replaced with #927 for this to work. Or at least I think from there you can make this RefCell <-> Mutex thing to only make the iterator !Sync but it would let it be Send which is all we need here.

benthecarman · 2023-04-21T20:19:15Z

Looks like this will eventually need to add impl Ord for Condition or you will get this error:

error[E0277]: the trait bound `Condition: Ord` is not satisfied
    --> crates/bdk/src/descriptor/policy.rs:251:40
     |
251  |         map.serialize_entry(&k_string, v)?;
     |             ---------------            ^ the trait `Ord` is not implemented for `Condition`
     |             |
     |             required by a bound introduced by this call
     |
     = help: the trait `serde::Serialize` is implemented for `BTreeSet<T>`
     = note: required for `BTreeSet<Condition>` to implement `serde::Serialize`

I have this in a branch here if you want: benthecarman@d7459e9

notmandatory · 2023-05-23T04:49:00Z

Should esplora-client be bumped to v0.5.0?

notmandatory · 2023-06-08T13:45:41Z

Per discord conversation added this to alpha.1 milestone. @evanlinjin would you rather we get this in before or after #976? I can take it over if it needs any rebasing.

- Use `default-features = false` for `miniscript`,`bitcoin`,and `bdk_chain` - Introduce `bdk_chain/std` feature - Add GitHub workflow `check-no-std` (not yet completly working) - Update GitHub workflow `check-wasm` to disable default `std` features

notmandatory

ACK 7ab84be

notmandatory · 2023-06-20T21:34:26Z

I've rebased this PR and removed from the commit message and PR description the part about updating the esplora-client version since that ended up being done in a different PR.

evanlinjin added this to the 1.0.0-alpha.0 milestone Mar 16, 2023

evanlinjin mentioned this pull request Mar 16, 2023

Set bitcoin, miniscript default-features = false, bump esplora-client version to 0.4 #893

Closed

3 tasks

evanlinjin self-assigned this Mar 16, 2023

LLFourn reviewed Mar 17, 2023

View reviewed changes

notmandatory modified the milestones: 1.0.0-alpha.0, 1.0.0-alpha.1 Mar 20, 2023

notmandatory mentioned this pull request Apr 7, 2025

Document no std support bitcoindevkit/bdk_wallet#162

Open

notmandatory self-assigned this Mar 23, 2023

notmandatory added the BDK 1.0.0 label Mar 23, 2023

notmandatory mentioned this pull request Mar 27, 2023

Update bitcoin and miniscript dependencies #929

Closed

notmandatory mentioned this pull request Mar 27, 2023

W13 BDK Lib Team Call bitcoindevkit/.github#50

Closed

22 tasks

benthecarman reviewed Apr 3, 2023

View reviewed changes

notmandatory force-pushed the disable_default_features branch 2 times, most recently from ece4a21 to 72b5f96 Compare April 3, 2023 21:29

notmandatory force-pushed the disable_default_features branch 2 times, most recently from 544e013 to 82cc5db Compare April 4, 2023 23:54

benthecarman mentioned this pull request Apr 10, 2023

Custom spk iterator #927

Merged

notmandatory removed this from the 1.0.0-alpha.1 milestone Apr 11, 2023

notmandatory removed the BDK 1.0.0 label May 6, 2023

notmandatory added this to the 1.0.0-alpha.1 milestone Jun 8, 2023

Better no-std support

7ab84be

- Use `default-features = false` for `miniscript`,`bitcoin`,and `bdk_chain` - Introduce `bdk_chain/std` feature - Add GitHub workflow `check-no-std` (not yet completly working) - Update GitHub workflow `check-wasm` to disable default `std` features

notmandatory force-pushed the disable_default_features branch from 82cc5db to 7ab84be Compare June 20, 2023 21:08

notmandatory changed the title ~~Better no-std support and bump esplora-client to 0.4~~ Better no-std support Jun 20, 2023

notmandatory approved these changes Jun 20, 2023

View reviewed changes

notmandatory merged commit 5d1f922 into bitcoindevkit:master Jun 20, 2023

benthecarman mentioned this pull request Jun 22, 2023

Impl Ord for Condition #1014

Closed

notmandatory mentioned this pull request Jul 4, 2023

Release 1.0.0-alpha.1 #1025

Closed

24 tasks

		@@ -13,7 +13,7 @@ readme = "README.md"

		[dependencies]
		bdk_chain = { path = "../chain", version = "0.3.1", features = ["serde", "miniscript"] }

Conversation

evanlinjin commented Mar 16, 2023 • edited by notmandatory Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

Uh oh!

LLFourn Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

LLFourn Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

notmandatory Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

notmandatory Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

LLFourn Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Mar 22, 2023

Uh oh!

notmandatory commented Mar 26, 2023

Uh oh!

notmandatory commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Mar 27, 2023

Uh oh!

tcharding commented Mar 27, 2023

Uh oh!

LLFourn commented Mar 28, 2023

Uh oh!

tcharding commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benthecarman commented Apr 3, 2023

Uh oh!

notmandatory commented Apr 4, 2023

Uh oh!

LLFourn commented Apr 4, 2023

Uh oh!

notmandatory commented Apr 4, 2023

Uh oh!

tcharding commented Apr 4, 2023

Uh oh!

notmandatory commented Apr 5, 2023

Uh oh!

LLFourn commented Apr 9, 2023

Uh oh!

benthecarman commented Apr 21, 2023

Uh oh!

notmandatory commented May 23, 2023

Uh oh!

notmandatory commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

evanlinjin commented Mar 16, 2023 •

edited by notmandatory

Loading

notmandatory Mar 20, 2023 •

edited

Loading

LLFourn Mar 21, 2023 •

edited

Loading

notmandatory commented Mar 27, 2023 •

edited

Loading

tcharding commented Mar 28, 2023 •

edited

Loading

benthecarman Apr 3, 2023 •

edited

Loading

notmandatory commented Jun 8, 2023 •

edited

Loading