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.

Sign up for GitHub

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

Jump to bottom

Remove dependency on rand #871

Closed
LLFourn opened this issue Feb 21, 2023 · 4 comments · Fixed by #1395
Closed

Remove dependency on rand #871

LLFourn opened this issue Feb 21, 2023 · 4 comments · Fixed by #1395
Assignees
@rustaceanrob
Labels
api A breaking API change module-blockchain module-wallet
Milestone
1.0.0-alpha

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Feb 21, 2023

Because we invoke thread_rng directly we have to also depend on getrandom in order to set certain feature flags to get things to compile on wasm.

Can we just depend on rand_core and let users pass in anything implement RngCore? This would simplify dependencies a bit. The place where this will be most tricky is in tx building but I hope it can be done.

We should try and attempt this before v1.
@LLFourn LLFourn added this to the v1 milestone Feb 21, 2023
@DanGould DanGould mentioned this issue Mar 2, 2023
Merged
@notmandatory notmandatory moved this to Todo in BDK Maintenance Mar 2, 2023
@notmandatory notmandatory removed this from the 1.0.0-alpha.0 milestone Mar 3, 2023
@notmandatory notmandatory added this to BDK May 4, 2023
@DanGould DanGould mentioned this issue May 21, 2023
Open
@notmandatory notmandatory moved this to Todo in BDK Jan 2, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Mar 17, 2024
@rustaceanrob
Copy link
Contributor

Would this imply TxBuilder would be initiated with impl Into<RngCore>? My gut feeling is that would cause problems for the UniFFI bindings.

@LLFourn
Copy link
Contributor Author

LLFourn commented Mar 28, 2024

Ok but the FFI bindings can depend on rand and pass in ThreadRng if they want to?

@rustaceanrob
Copy link
Contributor

Good point. I will try working on this.

@rustaceanrob rustaceanrob mentioned this issue Apr 3, 2024
Merged
8 tasks
@ValuedMammal
Copy link
Contributor

We could consider making TxParams generic over R and adding a field rng: Option<R>.

@notmandatory notmandatory moved this from Todo to In Progress in BDK Apr 12, 2024
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Jun 18, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Jun 23, 2024
LagginTimes pushed a commit to LagginTimes/bdk that referenced this issue Jun 25, 2024
@notmandatory @LagginTimes
Merge bitcoindevkit#1395: Remove rand dependency from bdk
ad0a6f0 
4bddb0d feat(wallet): add back TxBuilder finish() and sort_tx() with thread_rng() (Steve Myers)
45c0cae fix(bdk): remove rand dependency (rustaceanrob)

Pull request description:

  ### Description

  WIP towards removing `rand` fixes bitcoindevkit#871

  The `rand` dependency was imported explicitly, but `rand` is also implicitly used through the `rand-std` feature flag on `bitcoin`.

  ### Notes to he reviewers

  **Updated:**

  `rand` was used primarily in two parts of `bdk`. Particularly in signing and in building a transaction.

  Signing:
  - Used implicitly in [`sign_schnorr`](https://docs.rs/bitcoin/latest/bitcoin/key/struct.Secp256k1.html#method.sign_schnorr), but nowhere else within `signer`.

  Transaction ordering:
  - Used to shuffle the inputs and outputs of a transaction, the default
  - Used in the single random draw __as a fallback__ to branch and bound during coin selection. Branch and bound is the default coin selection option.

  See conversation for proposed solutions.

  ### Changelog notice

  - Remove the `rand` dependency from `bdk`

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  ValuedMammal:
    ACK 4bddb0d
  notmandatory:
    ACK 4bddb0d

Tree-SHA512: 662d9bcb1e02f8195d73df16789b8c2aba8ccd7b37ba713ebb0bfd19c66163acbcb6f266b64f88347cbb1f96b88c8a150581012cbf818d1dc8b4437b3e53fc62
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rustaceanrob rustaceanrob

Labels
api A breaking API change module-blockchain module-wallet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging a pull request may close this issue.

feat(wallet): Allow user provided RNG, make rand an optional dependency rustaceanrob/bdk
4 participants
@LLFourn @notmandatory @ValuedMammal @rustaceanrob