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
Remove rand
dependency from bdk
#1395
base: master
Are you sure you want to change the base?
Conversation
Good question. I think we want to remove the dependency on the bitcoin feature too because otherwise we have the same problem with wasm. If we want randomness during signing (it's a good default), then we should make the caller pass in an Rng and call:
Yeah, I think it's worth it tbh if that's what it comes to. Annoying thing is that not every |
Thanks, will keep chipping at this |
After thinking about this I am a little confused at how the Another aside, to shuffle inputs and outputs, and I think in BnB, an extension trait on slices from |
Yes. See BIP340. The secret and the message are hashed together if the aux rand bytes are null so that' swhat
There are no implications in most scenarios. People making a Bitcoin HSM using BDK that needs to be robust against an attacker who can inject faults into the power supply or something should look for the option and add randomness. It'd be nice if we could easily make it the default or nudge people towards. We'll have an opportunity when we redesign transaction building. But for now making it optional is fine.
Definitely shouldn't add a dependency. I think rolling your own is the right thing to do in this case. I wonder why we would be shuffling things in BnB but hopefully that goes away soon. |
I vastly overestimated the difficulty. No issues on the shuffling algo.
I am thinking of a few ways to go about this. There can be an |
Updated: The PR now uses
I like the idea of having a user pass an On building transactions, using randomness depends on the |
TBH I'm thinking we should just drop randomness for the schnorr signing function for now (use the |
51f8f6e
to
7952332
Compare
I would prefer this. Ultimately I think this is a feature that can be handled outside of this PR. I thought longer about input/output shuffling and greater concept of From the docs:
I would argue we do care what the ordering is for BDK user transactions as there are privacy implications, and the user should hand select the Along the same vein, I took the opportunity to try to resolve #534 by deprecating I referenced the The pub enum TxOrdering {
/// Randomized
Shuffle(Box<dyn RngCore>),
/// Unchanged
Untouched,
/// BIP69 / Lexicographic
#[deprecated = "BIP69 does not improve privacy as was the intention of the BIP"]
Bip69Lexicographic,
/// A custom ordering of inputs and outputs
Custom {
/// The custom function to order the inputs of the transaction
input_ordering: Box<dyn Fn(&TxIn, &TxIn) -> core::cmp::Ordering>,
/// The custom function to order the outputs of the transaction
output_ordering: Box<dyn Fn(&TxOut, &TxOut) -> core::cmp::Ordering>,
},
} |
Here's how I think we can keep single random draw as a fallback to BNB: don't implement SRD on the type I was a bit skeptical of adding an argument to |
I am hesitant about
Agree with this. I looked at the cc @evanlinjin |
In that case, fallback to LargestFirst. maybe simpler to just require the rng in all cases, though. |
I exchanged a message with Murch on that. He said that would be a bad idea because they might end up grinding down their UTXO pool. |
Seeing that it's common for our upstream deps to make rand-std a cargo feature, the idea came up that BDK should do the same, but that still leaves the question of how to obtain randomness when the feature is not enabled. I think we agree that a discussion of the optimal coin-select strategy probably belongs in a separate PR. If we were to do nothing else but try and drop the dependency on Add the parameter
With a minimal approach, we simply require a user-provided rng everywhere. I don't think this interferes with the idea of custom tx ordering, nor does it create strange new error cases.
// Try BNB. If that fails, fallback to single random draw
let coin_selection = match coin_selection.coin_select(
required_utxos.clone(),
optional_utxos.clone(),
fee_rate,
outgoing + fee_amount,
&drain_script,
) {
Ok(res) => res,
Err(e) => match e {
coin_selection::Error::InsufficientFunds { .. } => {
return Err(CreateTxError::CoinSelection(e));
}
coin_selection::Error::BnBNoExactMatch
| coin_selection::Error::BnBTotalTriesExceeded => {
coin_selection::single_random_draw(
required_utxos,
optional_utxos,
fee_rate,
outgoing + fee_amount,
&drain_script,
rng,
)
}
},
}; |
The plan above sounds good. As an aside I'm going to work on replacing the coin selection code with |
Will prep this for tomorrows PR review club. There is still plenty to discuss but I think we are in agreement on how this should be implemented. Thanks for the input @ValuedMammal
I'm interested to see how this works. Admittedly coin selection is an area I need to study up on, but relaxing BnB to find solutions with change makes randomness a non-issue as you said. |
It's on Thursday no? |
Oops mixed it up with team meeting. Regardless of me rebasing and making these changes, we should have a discussion on feature flags for the review club |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK
e5b50f5
to
012aafe
Compare
Updated with the notes from PR review club. Will have a few follow up PRs after this, but this ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, remind me again what we decided on what to tackle in this PR yesterday?
What happens if someone doesn't have/bring his own PRNG?
He/She can't finish the transaction builder, since we only expose finish_with_aux_rand
and no more finish
(no PRNG)?
In a |
Yes, that was what I was recollecting. Great, agreed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 012aafe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 012aafe
Not sure what caused the code coverage job to stall, you could try pushing again.
d4ce09f
to
913eb85
Compare
Description
WIP towards removing
rand
#871The
rand
dependency was imported explicitly, butrand
is also implicitly used through therand-std
feature flag onbitcoin
.Notes to he reviewers
Updated:
rand
was used primarily in two parts ofbdk
. Particularly in signing and in building a transaction.Signing:
sign_schnorr
, but nowhere else withinsigner
.Transaction ordering:
See conversation for proposed solutions.
Changelog notice
rand
dependency frombdk
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: