refactor(wallet)!: Add support for custom sorting and deprecate BIP69 #1487
Conversation
rustaceanrob
left a comment
rustaceanrob
left a comment
There was a problem hiding this comment.
Thanks for that detailed explanation. I think Arc is in line with what we are going for. I wonder how maintainers would feel to just remove BIP69 entirely since this allows for users to add it back if they must.
We discussed on the dev call today and the consensus is to remove bip69 support completely instead of just deprecating it. |
notmandatory
left a comment
notmandatory
left a comment
There was a problem hiding this comment.
I have just a couple small comments, this one looks almost ready.
| use core::cell::RefCell; | ||
| use core::fmt; | ||
|
|
||
| use std::sync::Arc; |
There was a problem hiding this comment.
🔧 Need to change the Arc type to be compatible with no-std (and WASM).
| use std::sync::Arc; | |
| use alloc::sync::Arc; |
| secret_digest_from_txout(tx_a).cmp(&secret_digest_from_txout(tx_b)) | ||
| }); | ||
|
|
||
| let custom_bip69_ordering_1 = TxOrdering::Custom { |
There was a problem hiding this comment.
⛏️ A nit, I wouldn't call these variables custom_bip69_ordering_? since bip69 is only for a specific deterministic lexicographical sort, and not this one with the hashing. Calling these custom_ordering_? would be better.
There was a problem hiding this comment.
Totally, probably leftovers of the Bip69Lexicographic test I use as template.
|
As you are the best to judge if your requests have been fulfilled, @notmandatory @rustaceanrob, please, consider resolving conversations as we move this forward. I plan to squash commits once we agree the work here is final. |
|
When you're ready for a final review be sure to rebase and squash the commits into one or two. |
nymius
force-pushed
the
I534/add-custom-sort-and-deprecate-bip69
branch
from
9d93a4e to
1348dbe
Compare
|
Rebased and updated. There is something extra you would like to see in the docs? |
nymius
force-pushed
the
I534/add-custom-sort-and-deprecate-bip69
branch
from
1348dbe to
3e1fc40
Compare
|
LGTM |
|
@nymius I you need to sign your commits before I can merge this PR. |
The deterministic sorting of transaction inputs and outputs proposed in BIP 69 doesn't improve the privacy of transactions as originally intended. In the search of not spreading bad practices but still provide flexibility for possible use cases depending on particular order of the inputs/outpus of a transaction, a new TxOrdering variant has been added to allow the implementation of these orders through the definition of comparison functions. Signed-off-by: Steve Myers <steve@notmandatory.org>
BIP 69 proposed a deterministic way to sort transaction inputs and outputs with the idea of enhancing privacy. It was later discovered there was no such enhancement but rather a decrement in privacy due to this sorting. To avoid the promotion of bad practices, the TxOrdering::Bip69Lexicographic variant which implemented this BIP for BDK is removed with this change. Notice that you can still produce a BIP 69 compliant transaction defining order functions for TxOrdering::Custom. Signed-off-by: Steve Myers <steve@notmandatory.org>
notmandatory
force-pushed
the
I534/add-custom-sort-and-deprecate-bip69
branch
from
3e1fc40 to
3bee563
Compare
|
I signed the commits for @nymius . |
notmandatory
changed the title
4 participants
Description
Resolves #534.
Resumes from the work in #556.
Add custom sorting function for inputs and outputs through
TxOrdering::Customand deprecatesTxOrdering::Bip69Lexicographic.Notes to the reviewers
I tried consider all discussions in #534 while implementing some changes to the original PR. I created a summary of the considerations I had while implementing this:
Why use smart pointers?
The size of enums and structs should be known at compilation time. A struct whose fields implements some kind of trait cannot be specified without using a smart pointer because the size of the implementations of the trait cannot be known beforehand.
Why
ArcorRcinstead ofBox?The majority of the useful smart pointers that I know (
Arc,Box,Rc) for this case implementDropwhich rules out the implementation ofCopy, making harder to manipulate a simple enum likeTxOrdering.Clonecan be used instead, implemented byArcandRc, but not implemented byBox.Why
Arcinstead ofRc?Multi threading I guess.
Why using a type alias like
TxVecSort?cargo-clippy was accusing a too complex type if using the whole type inlined in the struct inside the enum.
Why
Fnand notFnMut?FnMutis not allowed insideArc. I think this is due to the&mut selfocupies the first parameter of thecallmethod when desugared (https://rustyyato.github.io/rust/syntactic/sugar/2019/01/17/Closures-Magic-Functions.html), which doesn't respectsArclimitation of not having mutable references to data stored insideArc:Quoting the docs:
FnOnce>FnMut>Fn, where>stands for "is supertrait of", so,Fncan be used everywhereFnMutis expected.Why not
&'a dyn FnMut?It needs to include a lifetime parameter in
TxOrdering, which will force the addition of a lifetime parameter inTxParams, which will require the addition of a lifetime parameter in a lot of places more. Which one is preferable?Changelog notice
TxOrderingvariant:TxOrdering::Custom. A structure that stores the ordering functions to sort the inputs and outputs of a transaction.TxOrdering::Bip69Lexicographic.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: