bitcoindevkit
Add support for custom sorting and deprecate BIP69
Description
Resolves https://github.com/bitcoindevkit/bdk/issues/534.
Resumes from the work in https://github.com/bitcoindevkit/bdk/pull/556.
Add custom sorting function for inputs and outputs through TxOrdering::Custom and deprecates TxOrdering::Bip69Lexicographic.
Notes to the reviewers
I tried consider all discussions in https://github.com/bitcoindevkit/bdk/issues/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 Arc or Rc instead of Box?
The majority of the useful smart pointers that I know (Arc, Box, Rc) for this case implement Drop which rules out the implementation of Copy, making harder to manipulate a simple enum like TxOrdering. Clone can be used instead, implemented by Arc and Rc, but not implemented by Box.
Why Arc instead of Rc?
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 Fn and not FnMut?
FnMut is not allowed inside Arc. I think this is due to the &mut self ocupies the first parameter of the call method when desugared (https://rustyyato.github.io/rust/syntactic/sugar/2019/01/17/Closures-Magic-Functions.html), which doesn't respects Arc limitation of not having mutable references to data stored inside Arc:
Quoting the docs:
you cannot generally obtain a mutable reference to something inside an
Arc.
FnOnce > FnMut > Fn, where > stands for "is supertrait of", so, Fn can be used everywhere FnMut is expected.
Why not &'a dyn FnMut?
It needs to include a lifetime parameter in TxOrdering, which will force the addition of a lifetime parameter in TxParams, which will require the addition of a lifetime parameter in a lot of places more. Which one is preferable?
Changelog notice
- Adds new
TxOrderingvariant:TxOrdering::Custom. A structure that stores the ordering functions to sort the inputs and outputs of a transaction. - Deprecates
TxOrdering::Bip69Lexicographic.
Checklists
All Submissions:
- [ ] I've signed all my commits
- [x] I followed the contribution guidelines
- [x] I ran
cargo fmtandcargo clippybefore committing
New Features:
- [x] I've added tests for the new feature
- [ ] I've added docs for the new feature
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.
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.
Rebased and updated. There is something extra you would like to see in the docs?
