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

nymius · 2024-06-24T18:54:08Z

Description

Resolves #534.

Resumes from the work in #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 #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 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
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

rustaceanrob

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.

rustaceanrob · 2024-06-24T19:40:15Z

crates/wallet/src/wallet/tx_builder.rs

+}
+
+impl std::fmt::Debug for TxOrdering {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {


core::fmt

rustaceanrob · 2024-06-24T19:40:39Z

crates/wallet/src/wallet/mod.rs

@@ -1555,7 +1555,7 @@ impl Wallet {
        };

        // sort input/outputs according to the chosen algorithm
-        params.ordering.sort_tx_with_aux_rand(&mut tx, rng);
+        params.ordering.clone().sort_tx_with_aux_rand(&mut tx, rng);



Forgive my ignorance but why do we need clone here

For the reasons above we need to use some kind of smart pointer inside TxOrdering. Smart pointers manage data outside of their size. On the other hand, and I'm quoting a Stack Overflow answer here:

The Copy trait represents values that can be safely duplicated via memcpy

The idea here is, if you have to manage data outside of your size you cannot copy it with memcpy, so you cannot have smart pointers implementing Copy.

Before my changes to TxOrdering, because of the sort_tx_with_aux_rand signature (sort_tx_with_aux_rand(self..., notice self is owned by the function), and as params is later used in create_tx, Rust[1] performed a copy to avoid the partial move.

That's not possible anymore as TxOrdering cannot implement Copy with a smart pointer in its fields, so the new way to avoid the partial move is with clone().

[1] Here I'm guessing based on the suggestion from clippy.

Great thanks for clarifying

🔧 You should be able to remove this .clone() by changing the TxOrdering functions to borrow &self instead of taking ownership of self.

impl TxOrdering { pub fn sort_tx(&self, tx: &mut Transaction) ... pub fn sort_tx_with_aux_rand(&self, tx: &mut Transaction, rng: &mut impl RngCore) ... }

@nymius what do you think about removing this .clone() ?

Great! I thought there were stronger reasons to take ownership, but If that's not the case, I prefer the borrow.

crates/wallet/src/wallet/tx_builder.rs

notmandatory · 2024-06-25T20:50:10Z

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

I have just a couple small comments, this one looks almost ready.

notmandatory · 2024-06-26T02:07:14Z

crates/wallet/src/wallet/tx_builder.rs

@@ -42,6 +42,9 @@ use alloc::{boxed::Box, rc::Rc, string::String, vec::Vec};
 use core::cell::RefCell;
 use core::fmt;

+use std::sync::Arc;


🔧 Need to change the Arc type to be compatible with no-std (and WASM).

Suggested change

use std::sync::Arc;

use alloc::sync::Arc;

notmandatory · 2024-06-26T02:20:35Z

crates/wallet/src/wallet/tx_builder.rs

+            secret_digest_from_txout(tx_a).cmp(&secret_digest_from_txout(tx_b))
+        });
+
+        let custom_bip69_ordering_1 = TxOrdering::Custom {


⛏️ 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.

Totally, probably leftovers of the Bip69Lexicographic test I use as template.

crates/wallet/src/wallet/tx_builder.rs

notmandatory · 2024-06-26T02:29:50Z

crates/wallet/src/wallet/tx_builder.rs

@@ -763,16 +766,33 @@ impl fmt::Display for AddForeignUtxoError {
 #[cfg(feature = "std")]
 impl std::error::Error for AddForeignUtxoError {}

+type TxSort<T> = dyn Fn(&T, &T) -> std::cmp::Ordering;


🔧 Need to change the Ordering type to be compatible with no-std (and WASM).

Suggested change

type TxSort<T> = dyn Fn(&T, &T) -> std::cmp::Ordering;

type TxSort<T> = dyn Fn(&T, &T) -> core::cmp::Ordering;

nymius · 2024-06-27T15:29:08Z

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.

crates/wallet/tests/wallet.rs

notmandatory · 2024-06-27T17:05:02Z

When you're ready for a final review be sure to rebase and squash the commits into one or two.

nymius · 2024-06-28T23:36:45Z

Rebased and updated. There is something extra you would like to see in the docs?

rustaceanrob · 2024-07-01T23:05:03Z

LGTM

notmandatory

ACK 3e1fc40

notmandatory · 2024-07-05T18:29:23Z

@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 · 2024-07-05T20:19:52Z

I signed the commits for @nymius .

notmandatory assigned nymius Jun 24, 2024

notmandatory added api A breaking API change module-wallet labels Jun 24, 2024

notmandatory added this to the 1.0.0-alpha milestone Jun 24, 2024

rustaceanrob reviewed Jun 24, 2024

View reviewed changes

notmandatory reviewed Jun 26, 2024

View reviewed changes

crates/wallet/src/wallet/tx_builder.rs Show resolved Hide resolved

notmandatory reviewed Jun 26, 2024

View reviewed changes

notmandatory reviewed Jun 27, 2024

View reviewed changes

crates/wallet/tests/wallet.rs Show resolved Hide resolved

nymius force-pushed the I534/add-custom-sort-and-deprecate-bip69 branch from 9d93a4e to 1348dbe Compare June 28, 2024 23:33

nymius force-pushed the I534/add-custom-sort-and-deprecate-bip69 branch from 1348dbe to 3e1fc40 Compare July 1, 2024 22:54

notmandatory approved these changes Jul 3, 2024

View reviewed changes

Yureien and others added 2 commits July 5, 2024 15:03

notmandatory force-pushed the I534/add-custom-sort-and-deprecate-bip69 branch from 3e1fc40 to 3bee563 Compare July 5, 2024 20:18

notmandatory merged commit 139eec7 into bitcoindevkit:master Jul 5, 2024
12 checks passed

notmandatory mentioned this pull request Jul 20, 2024

Release 1.0.0-beta.1 #1520

Closed

30 tasks

notmandatory changed the title ~~Add support for custom sorting and deprecate BIP69~~ refactor(wallet)!: Add support for custom sorting and deprecate BIP69 Jul 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nymius commented Jun 24, 2024

rustaceanrob left a comment

rustaceanrob Jun 24, 2024

rustaceanrob Jun 24, 2024

nymius Jun 25, 2024 •

edited

Loading

rustaceanrob Jun 25, 2024

notmandatory Jun 27, 2024 •

edited

Loading

notmandatory Jul 1, 2024

nymius Jul 1, 2024

notmandatory commented Jun 25, 2024

notmandatory left a comment

notmandatory Jun 26, 2024

notmandatory Jun 26, 2024 •

edited

Loading

nymius Jun 26, 2024

notmandatory Jun 26, 2024

nymius commented Jun 27, 2024

notmandatory commented Jun 27, 2024

nymius commented Jun 28, 2024

rustaceanrob commented Jul 1, 2024

notmandatory left a comment

notmandatory commented Jul 5, 2024

notmandatory commented Jul 5, 2024

	type TxSort<T> = dyn Fn(&T, &T) -> std::cmp::Ordering;
	type TxSort<T> = dyn Fn(&T, &T) -> core::cmp::Ordering;

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

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

Conversation

nymius commented Jun 24, 2024

Description

Notes to the reviewers

Why use smart pointers?

Why Arc or Rc instead of Box?

Why Arc instead of Rc?

Why using a type alias like TxVecSort?

Why Fn and not FnMut?

Why not &'a dyn FnMut?

Changelog notice

Checklists

All Submissions:

New Features:

rustaceanrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nymius Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmandatory Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmandatory commented Jun 25, 2024

notmandatory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmandatory Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nymius commented Jun 27, 2024

notmandatory commented Jun 27, 2024

nymius commented Jun 28, 2024

rustaceanrob commented Jul 1, 2024

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory commented Jul 5, 2024

notmandatory commented Jul 5, 2024

Why `Arc` or `Rc` instead of `Box`?

Why `Arc` instead of `Rc`?

Why using a type alias like `TxVecSort`?

Why `Fn` and not `FnMut`?

Why not `&'a dyn FnMut`?

nymius Jun 25, 2024 •

edited

Loading

notmandatory Jun 27, 2024 •

edited

Loading

notmandatory Jun 26, 2024 •

edited

Loading