Add support for custom sorting and deprecate BIP69 #556

Yureien · 2022-02-27T06:16:47Z

Description

Resolves #534.

This PR adds a way to add custom sorting functions for both inputs and outputs. BIP69 is permanently deprecated.

Notes to the reviewers

Fn was used instead of FnMut to reduce complexity, since it's inside an Arc. We can try supporting FnMut but that would also require us to use a Mutex and I'm not sure whether it's worth the added complexity.

Additionally, I cannot decide what kind of test will be good for this. Should I implement just a simple sort (like a.value.cmp(&b.value)) and test if it's in proper order?

All Submissions:

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

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

Bugfixes:

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

rajarshimaitra

Initial Concept ACK.. But I don't recall if we have decided to depreciate BIP69.. It feel its still useful to keep it around?? Use it just as another option aTxOrdering? Some user might want to use lexicographic..

On the test, what comes to mind is use the lexicoraphic function itself as custom fn, and compare against standard test vectors.. You should get test vecs in reference impls of bip69.. maybe rust-bitcoin have some.. Or else cook up your simple custom sort and test against some self generated vectors.. In that case its better to hardcode the derived vecs in the test code..

afilini · 2022-03-10T11:06:02Z

But I don't recall if we have decided to depreciate BIP69

Yes, the idea is to deprecate it, but never remove it. It's just to make the users aware of its issues, they will be shown a warning and they can decide if they want to keep using it or not.

afilini · 2022-03-10T11:07:25Z

src/wallet/tx_builder.rs

 pub enum TxOrdering {
    /// Randomized (default)
    Shuffle,
    /// Unchanged
    Untouched,
    /// BIP69 / Lexicographic
+    #[deprecated = "BIP69 does not improve privacy as was the intention of the BIP"]


I don't know if we can, but if possible I would add a link to the discussion here on github. Maybe you could make a multiline warning, first line stays the same, and in a new line something like "To know more read the discussion at: ...".

Similar to what the rust compiler does when you have an error.

@afilini Can you give me an example in the rust lang? I can just look at their source code and implement the same way.

I meant something like:

warning: the feature `native_link_modifiers` is incomplete and may not be safe to use and/or cause compiler crashes --> library/unwind/src/lib.rs:8:12 | 8 | #![feature(native_link_modifiers)] | ^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(incomplete_features)]` on by default = note: see issue #81490 <https://github.com/rust-lang/rust/issues/81490> for more information

I like that "note: see issue ..." at the end

afilini · 2022-03-10T11:08:49Z

src/wallet/tx_builder.rs

+    /// Provide custom comparison functions for sorting
+    Custom {
+        /// Transaction inputs sort function
+        input_sort: Arc<dyn Fn(&TxIn, &TxIn) -> Ordering>,


Very minor nit, but this confused me for a second: I would just import std::cmp and then here use cmp::Ordering.

For a second I confused Ordering with TxOrdering and I couldn't figure out what was going on.

afilini · 2022-03-10T11:22:15Z

src/wallet/tx_builder.rs

    Bip69Lexicographic,
+    /// Provide custom comparison functions for sorting
+    Custom {


I think we may have to add some sort of "state" here, something that the two functions could look at to know what to do essentially. It doesn't have to be mutable, that would mess a lot of this up.

If we don't wanna add generic types we could maybe add a Box<dyn std::any::Any>, which you can then downcast to whatever you need in the functions. Obviously you would also have to add that as an extra argument to your functions.

CC @LLFourn who proposed sorting with a shared secret. I think you would not be able to do that, unless you introduce this state.

I'm not sure what happens if you try borrowing from local variables in the two Fns, I'm guessing you are not allowed to do that because of lifetime issues. Maybe we should test this first, actually.

@fadedcoder can you write a doc comment or a test which maybe demos how to use a secret to hash the input/output to produce the key?

Given the awkward Arc thing it might be better to have a convenience method like set_custom_ordering(i_f: impl Fn(&Txin...), o_f: impl Fn(&TxOut,..)) which would wrap the Arcs around them.

I think we may have to add some sort of "state" here, something that the two functions could look at to know what to do essentially. It doesn't have to be mutable, that would mess a lot of this up.

Just move in what you need?

Just move in what you need?

For some reason I always instinctively think that move will only work with FnOnce because I feel like the value would be consumed at the first iteration.

So yeah, move probably works, but as you said it would be nice to have an example and more tests to verify this.

Yeah sure, I'll add some tests or examples soon!

LLFourn

Nice. LGTM but needs a test.

LLFourn · 2022-03-15T02:57:09Z

src/wallet/tx_builder.rs

+                output_sort,
+            } => {
+                tx.input.sort_unstable_by(|a, b| input_sort(a, b));
+                tx.output.sort_unstable_by(|a, b| output_sort(a, b));


💡 I think you want sort_by_cached_key here since you might do some expensive computation why computing the key e.g. hashing.

LLFourn · 2022-03-15T03:06:41Z

src/wallet/tx_builder.rs

    Bip69Lexicographic,
+    /// Provide custom comparison functions for sorting
+    Custom {


@fadedcoder can you write a doc comment or a test which maybe demos how to use a secret to hash the input/output to produce the key?

Given the awkward Arc thing it might be better to have a convenience method like set_custom_ordering(i_f: impl Fn(&Txin...), o_f: impl Fn(&TxOut,..)) which would wrap the Arcs around them.

I think we may have to add some sort of "state" here, something that the two functions could look at to know what to do essentially. It doesn't have to be mutable, that would mess a lot of this up.

Just move in what you need?

notmandatory · 2022-05-04T17:03:03Z

Hi, please rebase to pickup changes in #596. Thanks!

danielabrozzoni · 2022-07-27T09:41:04Z

Hey @Yureien, are you still working on this?

danielabrozzoni · 2022-11-14T14:14:04Z

I'm closing this PR as the author hasn't been working on it for a while. If that's not the case, and instead you wish to keep working on this, feel free to re-open it :)

…e BIP69 3bee563 refactor(wallet)!: remove TxOrdering::Bip69Lexicographic (nymius) e5cb7b2 feat(wallet): add TxOrdering::Custom (FadedCoder) Pull request description:  ### Description Resolves bitcoindevkit#534. Resumes from the work in bitcoindevkit#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 bitcoindevkit#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](https://doc.rust-lang.org/std/sync/struct.Arc.html): > 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 `TxOrdering` variant: `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](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 * [ ] I've added docs for the new feature Top commit has no ACKs. Tree-SHA512: 0d3e3ea9aee3a6c9e9d5e1ae93215be84bd1bd99907a319976517819aeda768a7166860a48a8d24abb30c516e0129decb6a6aebd8f24783ea2230143e6dcd72a

Yureien added 2 commits February 27, 2022 11:38

Add support for custom sorting and deprecate BIP69

3abdbf5

Updated CHANGELOG

946819f

rajarshimaitra reviewed Mar 4, 2022

View reviewed changes

afilini reviewed Mar 10, 2022

View reviewed changes

notmandatory mentioned this pull request Mar 14, 2022

Remove BIP 69 sorting #534

Closed

LLFourn requested changes Mar 15, 2022

View reviewed changes

notmandatory assigned Yureien Mar 15, 2022

notmandatory modified the milestone: Release 0.18.0 Feature Freeze Mar 17, 2022

notmandatory mentioned this pull request Jun 21, 2022

W25 BDK Library Team Call bitcoindevkit/.github#9

Closed

14 tasks

danielabrozzoni mentioned this pull request Aug 28, 2022

W35 BDK Library Team Call bitcoindevkit/.github#20

Closed

27 tasks

danielabrozzoni closed this Nov 14, 2022

nymius mentioned this pull request Jun 24, 2024

refactor(wallet)!: Add support for custom sorting and deprecate BIP69 #1487

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom sorting and deprecate BIP69 #556

Add support for custom sorting and deprecate BIP69 #556

Yureien commented Feb 27, 2022

rajarshimaitra left a comment •

edited

Loading

afilini commented Mar 10, 2022

afilini Mar 10, 2022

Yureien Mar 15, 2022

afilini Mar 15, 2022

afilini Mar 10, 2022

afilini Mar 10, 2022

afilini Mar 10, 2022

LLFourn Mar 15, 2022

afilini Mar 15, 2022

Yureien Mar 15, 2022

LLFourn left a comment

LLFourn Mar 15, 2022

LLFourn Mar 15, 2022

notmandatory commented May 4, 2022

danielabrozzoni commented Jul 27, 2022

danielabrozzoni commented Nov 14, 2022

Add support for custom sorting and deprecate BIP69 #556

Add support for custom sorting and deprecate BIP69 #556

Conversation

Yureien commented Feb 27, 2022

Description

Notes to the reviewers

All Submissions:

New Features:

Bugfixes:

rajarshimaitra left a comment • edited Loading

Choose a reason for hiding this comment

afilini commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LLFourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmandatory commented May 4, 2022

danielabrozzoni commented Jul 27, 2022

danielabrozzoni commented Nov 14, 2022

rajarshimaitra left a comment •

edited

Loading