Merge bitcoindevkit#1487: Add support for custom sorting and deprecate BIP69

3bee563 refactor(wallet)!: remove TxOrdering::Bip69Lexicographic (nymius)
e5cb7b2 feat(wallet): add TxOrdering::Custom (FadedCoder)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### 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`.

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### 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?**

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### 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`.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### 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

Author: notmandatory

crates/wallet/src/wallet/tx_builder.rs

@@ -42,10 +42,13 @@ use alloc::{boxed::Box, rc::Rc, string::String, vec::Vec};
 use core::cell::RefCell;
 use core::fmt;
 
+use alloc::sync::Arc;
+
 use bitcoin::psbt::{self, Psbt};
 use bitcoin::script::PushBytes;
 use bitcoin::{
-    absolute, Amount, FeeRate, OutPoint, ScriptBuf, Sequence, Transaction, Txid, Weight,
+    absolute, Amount, FeeRate, OutPoint, ScriptBuf, Sequence, Transaction, TxIn, TxOut, Txid,
+    Weight,
 };
 use rand_core::RngCore;
 
@@ -763,43 +766,60 @@ impl fmt::Display for AddForeignUtxoError {
 #[cfg(feature = "std")]
 impl std::error::Error for AddForeignUtxoError {}
 
+type TxSort<T> = dyn Fn(&T, &T) -> core::cmp::Ordering;
+
 /// Ordering of the transaction's inputs and outputs
-#[derive(Default, Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Clone, Copy)]
+#[derive(Clone, Default)]
 pub enum TxOrdering {
     /// Randomized (default)
     #[default]
     Shuffle,
     /// Unchanged
     Untouched,
-    /// BIP69 / Lexicographic
-    Bip69Lexicographic,
+    /// Provide custom comparison functions for sorting
+    Custom {
+        /// Transaction inputs sort function
+        input_sort: Arc<TxSort<TxIn>>,
+        /// Transaction outputs sort function
+        output_sort: Arc<TxSort<TxOut>>,
+    },
+}
+
+impl core::fmt::Debug for TxOrdering {
+    fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
+        match self {
+            TxOrdering::Shuffle => write!(f, "Shuffle"),
+            TxOrdering::Untouched => write!(f, "Untouched"),
+            TxOrdering::Custom { .. } => write!(f, "Custom"),
+        }
+    }
 }
 
 impl TxOrdering {
     /// Sort transaction inputs and outputs by [`TxOrdering`] variant.
     ///
     /// Uses the thread-local random number generator (rng).
     #[cfg(feature = "std")]
-    pub fn sort_tx(self, tx: &mut Transaction) {
+    pub fn sort_tx(&self, tx: &mut Transaction) {
         self.sort_tx_with_aux_rand(tx, &mut bitcoin::key::rand::thread_rng())
     }
 
     /// Sort transaction inputs and outputs by [`TxOrdering`] variant.
     ///
     /// Uses a provided random number generator (rng).
-    pub fn sort_tx_with_aux_rand(self, tx: &mut Transaction, rng: &mut impl RngCore) {
+    pub fn sort_tx_with_aux_rand(&self, tx: &mut Transaction, rng: &mut impl RngCore) {
         match self {
             TxOrdering::Untouched => {}
             TxOrdering::Shuffle => {
                 shuffle_slice(&mut tx.input, rng);
                 shuffle_slice(&mut tx.output, rng);
             }
-            TxOrdering::Bip69Lexicographic => {
-                tx.input.sort_unstable_by_key(|txin| {
-                    (txin.previous_output.txid, txin.previous_output.vout)
-                });
-                tx.output
-                    .sort_unstable_by_key(|txout| (txout.value, txout.script_pubkey.clone()));
+            TxOrdering::Custom {
+                input_sort,
+                output_sort,
+            } => {
+                tx.input.sort_unstable_by(|a, b| input_sort(a, b));
+                tx.output.sort_unstable_by(|a, b| output_sort(a, b));
             }
         }
     }
@@ -910,13 +930,28 @@ mod test {
     }
 
     #[test]
-    fn test_output_ordering_bip69() {
+    fn test_output_ordering_custom_but_bip69() {
         use core::str::FromStr;
 
         let original_tx = ordering_test_tx!();
         let mut tx = original_tx;
 
-        TxOrdering::Bip69Lexicographic.sort_tx(&mut tx);
+        let bip69_txin_cmp = |tx_a: &TxIn, tx_b: &TxIn| {
+            let project_outpoint = |t: &TxIn| (t.previous_output.txid, t.previous_output.vout);
+            project_outpoint(tx_a).cmp(&project_outpoint(tx_b))
+        };
+
+        let bip69_txout_cmp = |tx_a: &TxOut, tx_b: &TxOut| {
+            let project_utxo = |t: &TxOut| (t.value, t.script_pubkey.clone());
+            project_utxo(tx_a).cmp(&project_utxo(tx_b))
+        };
+
+        let custom_bip69_ordering = TxOrdering::Custom {
+            input_sort: Arc::new(bip69_txin_cmp),
+            output_sort: Arc::new(bip69_txout_cmp),
+        };
+
+        custom_bip69_ordering.sort_tx(&mut tx);
 
         assert_eq!(
             tx.input[0].previous_output,
@@ -948,6 +983,63 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_output_ordering_custom_with_sha256() {
+        use bitcoin::hashes::{sha256, Hash};
+
+        let original_tx = ordering_test_tx!();
+        let mut tx_1 = original_tx.clone();
+        let mut tx_2 = original_tx.clone();
+        let shared_secret = "secret_tweak";
+
+        let hash_txin_with_shared_secret_seed = Arc::new(|tx_a: &TxIn, tx_b: &TxIn| {
+            let secret_digest_from_txin = |txin: &TxIn| {
+                sha256::Hash::hash(
+                    &[
+                        &txin.previous_output.txid.to_raw_hash()[..],
+                        &txin.previous_output.vout.to_be_bytes(),
+                        shared_secret.as_bytes(),
+                    ]
+                    .concat(),
+                )
+            };
+            secret_digest_from_txin(tx_a).cmp(&secret_digest_from_txin(tx_b))
+        });
+
+        let hash_txout_with_shared_secret_seed = Arc::new(|tx_a: &TxOut, tx_b: &TxOut| {
+            let secret_digest_from_txout = |txin: &TxOut| {
+                sha256::Hash::hash(
+                    &[
+                        &txin.value.to_sat().to_be_bytes(),
+                        &txin.script_pubkey.clone().into_bytes()[..],
+                        shared_secret.as_bytes(),
+                    ]
+                    .concat(),
+                )
+            };
+            secret_digest_from_txout(tx_a).cmp(&secret_digest_from_txout(tx_b))
+        });
+
+        let custom_ordering_from_salted_sha256_1 = TxOrdering::Custom {
+            input_sort: hash_txin_with_shared_secret_seed.clone(),
+            output_sort: hash_txout_with_shared_secret_seed.clone(),
+        };
+
+        let custom_ordering_from_salted_sha256_2 = TxOrdering::Custom {
+            input_sort: hash_txin_with_shared_secret_seed,
+            output_sort: hash_txout_with_shared_secret_seed,
+        };
+
+        custom_ordering_from_salted_sha256_1.sort_tx(&mut tx_1);
+        custom_ordering_from_salted_sha256_2.sort_tx(&mut tx_2);
+
+        // Check the ordering is consistent between calls
+        assert_eq!(tx_1, tx_2);
+        // Check transaction order has changed
+        assert_ne!(tx_1, original_tx);
+        assert_ne!(tx_2, original_tx);
+    }
+
     fn get_test_utxos() -> Vec<LocalOutput> {
         use bitcoin::hashes::Hash;
 

crates/wallet/tests/wallet.rs

@@ -1,3 +1,5 @@
+extern crate alloc;
+
 use std::path::Path;
 use std::str::FromStr;
 
@@ -985,13 +987,32 @@ fn test_create_tx_drain_to_dust_amount() {
 
 #[test]
 fn test_create_tx_ordering_respected() {
+    use alloc::sync::Arc;
+
     let (mut wallet, _) = get_funded_wallet_wpkh();
     let addr = wallet.next_unused_address(KeychainKind::External);
+
+    let bip69_txin_cmp = |tx_a: &TxIn, tx_b: &TxIn| {
+        let project_outpoint = |t: &TxIn| (t.previous_output.txid, t.previous_output.vout);
+        project_outpoint(tx_a).cmp(&project_outpoint(tx_b))
+    };
+
+    let bip69_txout_cmp = |tx_a: &TxOut, tx_b: &TxOut| {
+        let project_utxo = |t: &TxOut| (t.value, t.script_pubkey.clone());
+        project_utxo(tx_a).cmp(&project_utxo(tx_b))
+    };
+
+    let custom_bip69_ordering = bdk_wallet::wallet::tx_builder::TxOrdering::Custom {
+        input_sort: Arc::new(bip69_txin_cmp),
+        output_sort: Arc::new(bip69_txout_cmp),
+    };
+
     let mut builder = wallet.build_tx();
     builder
         .add_recipient(addr.script_pubkey(), Amount::from_sat(30_000))
         .add_recipient(addr.script_pubkey(), Amount::from_sat(10_000))
-        .ordering(bdk_wallet::wallet::tx_builder::TxOrdering::Bip69Lexicographic);
+        .ordering(custom_bip69_ordering);
+
     let psbt = builder.finish().unwrap();
     let fee = check_fee!(wallet, psbt);