From 5eaa3b0916055f54861ddec7e1e8a83ff62ea36b Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Mon, 21 Dec 2020 20:08:24 +1100
Subject: [PATCH 01/21] Use `unwrap_or_else`

As directed by clippy use `unwrap_or_else` in order to take advantage of
lazy evaluation.
---
 testutils/src/lib.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/testutils/src/lib.rs b/testutils/src/lib.rs
index 15b920c8..c60573e2 100644
--- a/testutils/src/lib.rs
+++ b/testutils/src/lib.rs
@@ -60,13 +60,13 @@ fn get_auth() -> Auth {
         ),
         _ => Auth::CookieFile(PathBuf::from(
             env::var("BDK_RPC_COOKIEFILE")
-                .unwrap_or("/home/user/.bitcoin/regtest/.cookie".to_string()),
+                .unwrap_or_else(|_| "/home/user/.bitcoin/regtest/.cookie".to_string()),
         )),
     }
 }
 
 pub fn get_electrum_url() -> String {
-    env::var("BDK_ELECTRUM_URL").unwrap_or("tcp://127.0.0.1:50001".to_string())
+    env::var("BDK_ELECTRUM_URL").unwrap_or_else(|_| "tcp://127.0.0.1:50001".to_string())
 }
 
 pub struct TestClient {
@@ -311,8 +311,8 @@ where
 
 impl TestClient {
     pub fn new() -> Self {
-        let url = env::var("BDK_RPC_URL").unwrap_or("127.0.0.1:18443".to_string());
-        let wallet = env::var("BDK_RPC_WALLET").unwrap_or("bdk-test".to_string());
+        let url = env::var("BDK_RPC_URL").unwrap_or_else(|_| "127.0.0.1:18443".to_string());
+        let wallet = env::var("BDK_RPC_WALLET").unwrap_or_else(|_| "bdk-test".to_string());
         let client =
             RpcClient::new(format!("http://{}/wallet/{}", url, wallet), get_auth()).unwrap();
         let electrum = ElectrumClient::new(&get_electrum_url()).unwrap();

From 2057c354685e89cce4a273160eb7c69063b02cae Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Mon, 21 Dec 2020 20:09:45 +1100
Subject: [PATCH 02/21] Use ! is_empty instead of len > 0

As directed by clippy use `!a.is_empty()` instead of `a.len() > 0`.
---
 testutils/src/lib.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testutils/src/lib.rs b/testutils/src/lib.rs
index c60573e2..3a0e5b63 100644
--- a/testutils/src/lib.rs
+++ b/testutils/src/lib.rs
@@ -349,7 +349,7 @@ impl TestClient {
 
     pub fn receive(&mut self, meta_tx: TestIncomingTx) -> Txid {
         assert!(
-            meta_tx.output.len() > 0,
+            !meta_tx.output.is_empty(),
             "can't create a transaction with no outputs"
         );
 

From 3a0a1e6d4a833f9f215f65a345ac4fef6dbc8afa Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 13:33:05 +1100
Subject: [PATCH 03/21] Remove static lifetime

const str types do not need an explicit lifetime, remove it. Found by
clippy.
---
 src/wallet/tx_builder.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs
index 86dca563..d4ba8a4e 100644
--- a/src/wallet/tx_builder.rs
+++ b/src/wallet/tx_builder.rs
@@ -629,7 +629,7 @@ impl ChangeSpendPolicy {
 
 #[cfg(test)]
 mod test {
-    const ORDERING_TEST_TX: &'static str = "0200000003c26f3eb7932f7acddc5ddd26602b77e7516079b03090a16e2c2f54\
+    const ORDERING_TEST_TX: &str = "0200000003c26f3eb7932f7acddc5ddd26602b77e7516079b03090a16e2c2f54\
                                             85d1fd600f0100000000ffffffffc26f3eb7932f7acddc5ddd26602b77e75160\
                                             79b03090a16e2c2f5485d1fd600f0000000000ffffffff571fb3e02278217852\
                                             dd5d299947e2b7354a639adc32ec1fa7b82cfb5dec530e0500000000ffffffff\

From 0e99d02fbe7ff611860c8fd7f0711a864ea98b5d Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 13:42:52 +1100
Subject: [PATCH 04/21] Remove redundant calls to clone

No need to clone copy types, found by clippy.
---
 src/database/mod.rs          |  4 ++--
 src/descriptor/dsl.rs        | 15 ++++++---------
 src/wallet/coin_selection.rs |  2 +-
 src/wallet/tx_builder.rs     |  2 +-
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/src/database/mod.rs b/src/database/mod.rs
index 33ec7ccf..ad2b90d8 100644
--- a/src/database/mod.rs
+++ b/src/database/mod.rs
@@ -227,7 +227,7 @@ pub mod test {
         );
         assert_eq!(
             tree.get_path_from_script_pubkey(&script).unwrap(),
-            Some((keychain, path.clone()))
+            Some((keychain, path))
         );
     }
 
@@ -256,7 +256,7 @@ pub mod test {
         );
         assert_eq!(
             tree.get_path_from_script_pubkey(&script).unwrap(),
-            Some((keychain, path.clone()))
+            Some((keychain, path))
         );
     }
 
diff --git a/src/descriptor/dsl.rs b/src/descriptor/dsl.rs
index 37e9a1a4..b3f62ae5 100644
--- a/src/descriptor/dsl.rs
+++ b/src/descriptor/dsl.rs
@@ -968,7 +968,7 @@ mod test {
     fn test_valid_networks() {
         let xprv = bip32::ExtendedPrivKey::from_str("tprv8ZgxMBicQKsPcx5nBGsR63Pe8KnRUqmbJNENAfGftF3yuXoMMoVJJcYeUw5eVkm9WBPjWYt6HMWYJNesB5HaNVBaFc1M6dRjWSYnmewUMYy").unwrap();
         let path = bip32::DerivationPath::from_str("m/0").unwrap();
-        let desc_key = (xprv, path.clone()).into_descriptor_key().unwrap();
+        let desc_key = (xprv, path).into_descriptor_key().unwrap();
 
         let (_desc, _key_map, valid_networks) = descriptor!(pkh(desc_key)).unwrap();
         assert_eq!(
@@ -978,7 +978,7 @@ mod test {
 
         let xprv = bip32::ExtendedPrivKey::from_str("xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHi").unwrap();
         let path = bip32::DerivationPath::from_str("m/10/20/30/40").unwrap();
-        let desc_key = (xprv, path.clone()).into_descriptor_key().unwrap();
+        let desc_key = (xprv, path).into_descriptor_key().unwrap();
 
         let (_desc, _key_map, valid_networks) = descriptor!(wpkh(desc_key)).unwrap();
         assert_eq!(valid_networks, [Bitcoin].iter().cloned().collect());
@@ -1005,12 +1005,9 @@ mod test {
             descriptor!(sh(wsh(multi(2, desc_key1, desc_key2, desc_key3)))).unwrap();
         assert_eq!(key_map.len(), 3);
 
-        let desc_key1: DescriptorKey<Segwitv0> =
-            (xprv1, path1.clone()).into_descriptor_key().unwrap();
-        let desc_key2: DescriptorKey<Segwitv0> =
-            (xprv2, path2.clone()).into_descriptor_key().unwrap();
-        let desc_key3: DescriptorKey<Segwitv0> =
-            (xprv3, path3.clone()).into_descriptor_key().unwrap();
+        let desc_key1: DescriptorKey<Segwitv0> = (xprv1, path1).into_descriptor_key().unwrap();
+        let desc_key2: DescriptorKey<Segwitv0> = (xprv2, path2).into_descriptor_key().unwrap();
+        let desc_key3: DescriptorKey<Segwitv0> = (xprv3, path3).into_descriptor_key().unwrap();
 
         let (key1, _key_map, _valid_networks) = desc_key1.extract(&secp).unwrap();
         let (key2, _key_map, _valid_networks) = desc_key2.extract(&secp).unwrap();
@@ -1026,7 +1023,7 @@ mod test {
         // this compiles
         let xprv = bip32::ExtendedPrivKey::from_str("tprv8ZgxMBicQKsPcx5nBGsR63Pe8KnRUqmbJNENAfGftF3yuXoMMoVJJcYeUw5eVkm9WBPjWYt6HMWYJNesB5HaNVBaFc1M6dRjWSYnmewUMYy").unwrap();
         let path = bip32::DerivationPath::from_str("m/0").unwrap();
-        let desc_key: DescriptorKey<Legacy> = (xprv, path.clone()).into_descriptor_key().unwrap();
+        let desc_key: DescriptorKey<Legacy> = (xprv, path).into_descriptor_key().unwrap();
 
         let (desc, _key_map, _valid_networks) = descriptor!(pkh(desc_key)).unwrap();
         assert_eq!(desc.to_string(), "pkh(tpubD6NzVbkrYhZ4WR7a4vY1VT3khMJMeAxVsfq9TBJyJWrNk247zCJtV7AWf6UJP7rAVsn8NNKdJi3gFyKPTmWZS9iukb91xbn2HbFSMQm2igY/0/*)#yrnz9pp2");
diff --git a/src/wallet/coin_selection.rs b/src/wallet/coin_selection.rs
index c57f251d..9e1b90c3 100644
--- a/src/wallet/coin_selection.rs
+++ b/src/wallet/coin_selection.rs
@@ -804,7 +804,7 @@ mod test {
             .coin_select(
                 &database,
                 vec![],
-                utxos.clone(),
+                utxos,
                 FeeRate::from_sat_per_vb(1.0),
                 99932, // first utxo's effective value
                 0.0,
diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs
index d4ba8a4e..35c7d073 100644
--- a/src/wallet/tx_builder.rs
+++ b/src/wallet/tx_builder.rs
@@ -678,7 +678,7 @@ mod test {
         use std::str::FromStr;
 
         let original_tx = ordering_test_tx!();
-        let mut tx = original_tx.clone();
+        let mut tx = original_tx;
 
         TxOrdering::BIP69Lexicographic.sort_tx(&mut tx);
 

From 2afc9faa08a1f8c9c644eb43e9647205808dc7a3 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 13:45:39 +1100
Subject: [PATCH 05/21] Remove needles explicit reference

Clippy emits warning:

	warning: needlessly taken reference of both operands

Remove the explicit reference's as suggested.
---
 src/descriptor/policy.rs | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/descriptor/policy.rs b/src/descriptor/policy.rs
index 8929e085..7dbfda60 100644
--- a/src/descriptor/policy.rs
+++ b/src/descriptor/policy.rs
@@ -938,7 +938,7 @@ mod test {
             .unwrap();
 
         assert!(
-            matches!(&policy.item, Signature(pk_or_f) if &pk_or_f.fingerprint.unwrap() == &fingerprint)
+            matches!(&policy.item, Signature(pk_or_f) if pk_or_f.fingerprint.unwrap() == fingerprint)
         );
         assert!(matches!(&policy.contribution, Satisfaction::None));
 
@@ -953,7 +953,7 @@ mod test {
             .unwrap();
 
         assert!(
-            matches!(&policy.item, Signature(pk_or_f) if &pk_or_f.fingerprint.unwrap() == &fingerprint)
+            matches!(&policy.item, Signature(pk_or_f) if pk_or_f.fingerprint.unwrap() == fingerprint)
         );
         assert!(
             matches!(&policy.contribution, Satisfaction::Complete {condition} if condition.csv == None && condition.timelock == None)
@@ -1039,8 +1039,8 @@ mod test {
 
         assert!(
             matches!(&policy.item, Multisig { keys, threshold } if threshold == &1
-            && &keys[0].fingerprint.unwrap() == &fingerprint0
-            && &keys[1].fingerprint.unwrap() == &fingerprint1)
+            && keys[0].fingerprint.unwrap() == fingerprint0
+            && keys[1].fingerprint.unwrap() == fingerprint1)
         );
         assert!(
             matches!(&policy.contribution, Satisfaction::PartialComplete { n, m, items, conditions, .. } if n == &2
@@ -1071,8 +1071,8 @@ mod test {
 
         assert!(
             matches!(&policy.item, Multisig { keys, threshold } if threshold == &2
-            && &keys[0].fingerprint.unwrap() == &fingerprint0
-            && &keys[1].fingerprint.unwrap() == &fingerprint1)
+            && keys[0].fingerprint.unwrap() == fingerprint0
+            && keys[1].fingerprint.unwrap() == fingerprint1)
         );
 
         assert!(
@@ -1103,7 +1103,7 @@ mod test {
             .unwrap();
 
         assert!(
-            matches!(&policy.item, Signature(pk_or_f) if &pk_or_f.fingerprint.unwrap() == &fingerprint)
+            matches!(&policy.item, Signature(pk_or_f) if pk_or_f.fingerprint.unwrap() == fingerprint)
         );
         assert!(matches!(&policy.contribution, Satisfaction::None));
 
@@ -1119,7 +1119,7 @@ mod test {
             .unwrap();
 
         assert!(
-            matches!(&policy.item, Signature(pk_or_f) if &pk_or_f.fingerprint.unwrap() == &fingerprint)
+            matches!(&policy.item, Signature(pk_or_f) if pk_or_f.fingerprint.unwrap() == fingerprint)
         );
         assert!(
             matches!(&policy.contribution, Satisfaction::Complete {condition} if condition.csv == None && condition.timelock == None)
@@ -1147,8 +1147,8 @@ mod test {
 
         assert!(
             matches!(&policy.item, Multisig { keys, threshold } if threshold == &1
-            && &keys[0].fingerprint.unwrap() == &fingerprint0
-            && &keys[1].fingerprint.unwrap() == &fingerprint1)
+            && keys[0].fingerprint.unwrap() == fingerprint0
+            && keys[1].fingerprint.unwrap() == fingerprint1)
         );
         assert!(
             matches!(&policy.contribution, Satisfaction::PartialComplete { n, m, items, conditions, .. } if n == &2

From 79cab93d49f03f480df03287cdf645dc07bc6c93 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 14:07:09 +1100
Subject: [PATCH 06/21] Use count instead of collect and len

Clippy emits warning:

	warning: avoid using `collect()` when not needed

As suggested by clippy just use `count` directly on the iterator instead
of `collect` followed by `len`.
---
 src/wallet/tx_builder.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs
index 35c7d073..9a9b8d2e 100644
--- a/src/wallet/tx_builder.rs
+++ b/src/wallet/tx_builder.rs
@@ -736,9 +736,9 @@ mod test {
         let filtered = get_test_utxos()
             .into_iter()
             .filter(|u| change_spend_policy.is_satisfied_by(u))
-            .collect::<Vec<_>>();
+            .count();
 
-        assert_eq!(filtered.len(), 2);
+        assert_eq!(filtered, 2);
     }
 
     #[test]

From 824b00c9e049b4b9bee95619204730e63b29f5a7 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 14:08:54 +1100
Subject: [PATCH 07/21] Use next instead of nth(0)

As suggested by clippy we can use `.next()` on an iterator instead of
`nth(0)`. Although arguably no clearer, and certainly no worse, it keeps
clippy quiet and a clean lint is a good thing.
---
 src/wallet/mod.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs
index 8bb254c4..8f26d06f 100644
--- a/src/wallet/mod.rs
+++ b/src/wallet/mod.rs
@@ -1942,7 +1942,7 @@ mod test {
 
         assert_eq!(psbt.inputs[0].bip32_derivation.len(), 1);
         assert_eq!(
-            psbt.inputs[0].bip32_derivation.values().nth(0).unwrap(),
+            psbt.inputs[0].bip32_derivation.values().next().unwrap(),
             &(
                 Fingerprint::from_str("d34db33f").unwrap(),
                 DerivationPath::from_str("m/44'/0'/0'/0/0").unwrap()
@@ -1968,7 +1968,7 @@ mod test {
 
         assert_eq!(psbt.outputs[0].bip32_derivation.len(), 1);
         assert_eq!(
-            psbt.outputs[0].bip32_derivation.values().nth(0).unwrap(),
+            psbt.outputs[0].bip32_derivation.values().next().unwrap(),
             &(
                 Fingerprint::from_str("d34db33f").unwrap(),
                 DerivationPath::from_str("m/44'/0'/0'/0/5").unwrap()

From 35184e6908b935c4c0fd2ee3be95f63af3515a00 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 14:16:43 +1100
Subject: [PATCH 08/21] Use default pattern

Clippy emits warning:

  warning: field assignment outside of initializer for an instance
  created with Default::default()

Do as suggested by clippy and use the default init pattern.

```
    let foo = Foo {
    	bar: ...,
        Default::default()
    }
```
---
 src/wallet/mod.rs | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs
index 8f26d06f..17cf0c98 100644
--- a/src/wallet/mod.rs
+++ b/src/wallet/mod.rs
@@ -3220,15 +3220,18 @@ mod test {
         let (mut psbt, _) = builder.finish().unwrap();
 
         // add another input to the psbt that is at least passable.
-        let mut dud_input = bitcoin::util::psbt::Input::default();
-        dud_input.witness_utxo = Some(TxOut {
-            value: 100_000,
-            script_pubkey: miniscript::Descriptor::<bitcoin::PublicKey>::from_str(
-                "wpkh(025476c2e83188368da1ff3e292e7acafcdb3566bb0ad253f62fc70f07aeee6357)",
-            )
-            .unwrap()
-            .script_pubkey(),
-        });
+        let dud_input = bitcoin::util::psbt::Input {
+            witness_utxo: Some(TxOut {
+                value: 100_000,
+                script_pubkey: miniscript::Descriptor::<bitcoin::PublicKey>::from_str(
+                    "wpkh(025476c2e83188368da1ff3e292e7acafcdb3566bb0ad253f62fc70f07aeee6357)",
+                )
+                .unwrap()
+                .script_pubkey(),
+            }),
+            ..Default::default()
+        };
+
         psbt.inputs.push(dud_input);
         psbt.global.unsigned_tx.input.push(bitcoin::TxIn::default());
         let (psbt, is_final) = wallet.sign(psbt, None).unwrap();

From ba8ce7233d0e747b4c7bff2997641ecd9cdceea0 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 15:58:57 +1100
Subject: [PATCH 09/21] Allow mutex_atomic

 Clippy complains about use of a mutex, suggesting we use an
 `AtomicUsize`. While the same functionality _could_ be achieved using an
 `AtomicUsize` and a CAS loop it makes the code harder to reason about
 for little gain. Lets just quieten clippy with an allow attribute and
 document why we did so.
---
 src/blockchain/compact_filters/mod.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/blockchain/compact_filters/mod.rs b/src/blockchain/compact_filters/mod.rs
index 649c53df..6785d923 100644
--- a/src/blockchain/compact_filters/mod.rs
+++ b/src/blockchain/compact_filters/mod.rs
@@ -239,6 +239,7 @@ impl Blockchain for CompactFiltersBlockchain {
         vec![Capability::FullHistory].into_iter().collect()
     }
 
+    #[allow(clippy::mutex_atomic)] // Mutex is easier to understand than a CAS loop.
     fn setup<D: BatchDatabase, P: 'static + Progress>(
         &self,
         _stop_gap: Option<usize>, // TODO: move to electrum and esplora only

From 343e97da0e2d0d84af402d94d55caa2e4a9d3859 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 16:15:09 +1100
Subject: [PATCH 10/21] Conditionally compile constructor

The `ChunksIterator` constructor is only used when either `electrum` or
`esplora` features are enabled. Conditionally build it so that we do not
get a clippy warning when building without these features.
---
 src/wallet/utils.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wallet/utils.rs b/src/wallet/utils.rs
index a71d09c1..d0065d0e 100644
--- a/src/wallet/utils.rs
+++ b/src/wallet/utils.rs
@@ -156,8 +156,8 @@ pub struct ChunksIterator<I: Iterator> {
     size: usize,
 }
 
+#[cfg(any(feature = "electrum", feature = "esplora"))]
 impl<I: Iterator> ChunksIterator<I> {
-    #[allow(dead_code)]
     pub fn new(iter: I, size: usize) -> Self {
         ChunksIterator { iter, size }
     }

From 0e6add0cfbf0e25f06b354e82a3f4bf704fcbe69 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 23 Dec 2020 16:19:37 +1100
Subject: [PATCH 11/21] Refactor db/batch matching

Remove the TODO; refactor matching to correctly handle conditionally
built `Sled` variants. Use `unreachable` instead of `unimplemented` with
a comment hinting that this is a bug, this makes it explicit, both at
runtime and when reading the code, that this match arm should not be hit.
---
 src/database/any.rs | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/src/database/any.rs b/src/database/any.rs
index 021ab7d6..7436f0c9 100644
--- a/src/database/any.rs
+++ b/src/database/any.rs
@@ -312,24 +312,17 @@ impl BatchDatabase for AnyDatabase {
         }
     }
     fn commit_batch(&mut self, batch: Self::Batch) -> Result<(), Error> {
-        // TODO: refactor once `move_ref_pattern` is stable
-        #[allow(irrefutable_let_patterns)]
         match self {
-            AnyDatabase::Memory(db) => {
-                if let AnyBatch::Memory(batch) = batch {
-                    db.commit_batch(batch)
-                } else {
-                    unimplemented!()
-                }
-            }
+            AnyDatabase::Memory(db) => match batch {
+                AnyBatch::Memory(batch) => db.commit_batch(batch),
+                #[cfg(feature = "key-value-db")]
+                _ => unimplemented!("Sled batch shouldn't be used with Memory db."),
+            },
             #[cfg(feature = "key-value-db")]
-            AnyDatabase::Sled(db) => {
-                if let AnyBatch::Sled(batch) = batch {
-                    db.commit_batch(batch)
-                } else {
-                    unimplemented!()
-                }
-            }
+            AnyDatabase::Sled(db) => match batch {
+                AnyBatch::Sled(batch) => db.commit_batch(batch),
+                _ => unimplemented!("Memory batch shouldn't be used with Sled db."),
+            },
         }
     }
 }

From cb3b8cf21b6bb48d3b28c9e98df06464823bdcb0 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 30 Dec 2020 14:58:42 +1100
Subject: [PATCH 12/21] Do not compare vtable

Clippy emits error:

 comparing trait object pointers compares a non-unique vtable address

The vtable is an implementation detail, it may change in future. we
should not be comparing vtable addresses for equality. Instead we can
get a pointer to the data field of a fat pointer and compare on that.
---
 src/wallet/signer.rs | 49 +++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs
index 83888f50..b076b981 100644
--- a/src/wallet/signer.rs
+++ b/src/wallet/signer.rs
@@ -569,6 +569,21 @@ mod signers_container_tests {
     use miniscript::ScriptContext;
     use std::str::FromStr;
 
+    // Return true if data field of `fat` pointer is the same address as `thin` pointer.
+    fn is_equal(fat: &Arc<dyn Signer>, thin: &Arc<DummySigner>) -> bool {
+        let (left, right) = raw_pointers(fat, thin);
+        left == right
+    }
+
+    // Make pointer values out of `fat` and `thin` that can be compared.
+    fn raw_pointers(fat: &Arc<dyn Signer>, thin: &Arc<DummySigner>) -> (*const u8, *const u8) {
+        let (left, _vtable): (*const u8, *const u8) =
+            unsafe { std::mem::transmute(Arc::as_ptr(fat)) };
+        let right = Arc::as_ptr(thin) as *const u8;
+
+        (left, right)
+    }
+
     // Signers added with the same ordering (like `Ordering::default`) created from `KeyMap`
     // should be preserved and not overwritten.
     // This happens usually when a set of signers is created from a descriptor with private keys.
@@ -615,18 +630,22 @@ mod signers_container_tests {
 
         // Check that signers are sorted from lowest to highest ordering
         let signers = signers.signers();
-        assert_eq!(Arc::as_ptr(signers[0]), Arc::as_ptr(&signer1));
-        assert_eq!(Arc::as_ptr(signers[1]), Arc::as_ptr(&signer2));
-        assert_eq!(Arc::as_ptr(signers[2]), Arc::as_ptr(&signer3));
+
+        let (left, right) = raw_pointers(signers[0], &signer1);
+        assert_eq!(left, right);
+        let (left, right) = raw_pointers(signers[1], &signer2);
+        assert_eq!(left, right);
+        let (left, right) = raw_pointers(signers[2], &signer3);
+        assert_eq!(left, right);
     }
 
     #[test]
     fn find_signer_by_id() {
         let mut signers = SignersContainer::new();
-        let signer1: Arc<dyn Signer> = Arc::new(DummySigner);
-        let signer2: Arc<dyn Signer> = Arc::new(DummySigner);
-        let signer3: Arc<dyn Signer> = Arc::new(DummySigner);
-        let signer4: Arc<dyn Signer> = Arc::new(DummySigner);
+        let signer1 = Arc::new(DummySigner);
+        let signer2 = Arc::new(DummySigner);
+        let signer3 = Arc::new(DummySigner);
+        let signer4 = Arc::new(DummySigner);
 
         let id1 = SignerId::Fingerprint(b"cafe"[..].into());
         let id2 = SignerId::Fingerprint(b"babe"[..].into());
@@ -637,22 +656,14 @@ mod signers_container_tests {
         signers.add_external(id2.clone(), SignerOrdering(2), signer2.clone());
         signers.add_external(id3.clone(), SignerOrdering(3), signer3.clone());
 
-        assert!(
-            matches!(signers.find(id1), Some(signer) if Arc::as_ptr(&signer1) == Arc::as_ptr(signer))
-        );
-        assert!(
-            matches!(signers.find(id2), Some(signer) if Arc::as_ptr(&signer2) == Arc::as_ptr(signer))
-        );
-        assert!(
-            matches!(signers.find(id3.clone()), Some(signer) if Arc::as_ptr(&signer3) == Arc::as_ptr(signer))
-        );
+        assert!(matches!(signers.find(id1), Some(signer) if is_equal(signer, &signer1)));
+        assert!(matches!(signers.find(id2), Some(signer) if is_equal(signer, &signer2)));
+        assert!(matches!(signers.find(id3.clone()), Some(signer) if is_equal(signer, &signer3)));
 
         // The `signer4` has the same ID as `signer3` but lower ordering.
         // It should be found by `id3` instead of `signer3`.
         signers.add_external(id3.clone(), SignerOrdering(2), signer4.clone());
-        assert!(
-            matches!(signers.find(id3), Some(signer) if Arc::as_ptr(&signer4) == Arc::as_ptr(signer))
-        );
+        assert!(matches!(signers.find(id3), Some(signer) if is_equal(signer, &signer4)));
 
         // Can't find anything with ID that doesn't exist
         assert!(matches!(signers.find(id_nonexistent), None));

From 24df438607bcf58c5304b6c2976b4d70ce6a6716 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 10 Feb 2021 10:20:06 +1100
Subject: [PATCH 13/21] Remove useless question mark operator

Clippy emits:

  warning: Question mark operator is useless here

No need to use the `?` operator inside an `Ok()` statement when
returning, just return directly.
---
 src/blockchain/compact_filters/store.rs | 44 ++++++++++++-------------
 src/descriptor/template.rs              | 20 +++++------
 src/keys/mod.rs                         |  2 +-
 3 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/src/blockchain/compact_filters/store.rs b/src/blockchain/compact_filters/store.rs
index 5e278f24..3b879194 100644
--- a/src/blockchain/compact_filters/store.rs
+++ b/src/blockchain/compact_filters/store.rs
@@ -119,7 +119,7 @@ where
     }
 
     fn deserialize(data: &[u8]) -> Result<Self, CompactFiltersError> {
-        Ok(deserialize(data).map_err(|_| CompactFiltersError::DataCorruption)?)
+        deserialize(data).map_err(|_| CompactFiltersError::DataCorruption)
     }
 }
 
@@ -436,15 +436,14 @@ impl ChainStore<Full> {
 
         let key = StoreEntry::BlockHeaderIndex(Some(*block_hash)).get_key();
         let data = read_store.get_pinned_cf(cf_handle, key)?;
-        Ok(data
-            .map(|data| {
-                Ok::<_, CompactFiltersError>(usize::from_be_bytes(
-                    data.as_ref()
-                        .try_into()
-                        .map_err(|_| CompactFiltersError::DataCorruption)?,
-                ))
-            })
-            .transpose()?)
+        data.map(|data| {
+            Ok::<_, CompactFiltersError>(usize::from_be_bytes(
+                data.as_ref()
+                    .try_into()
+                    .map_err(|_| CompactFiltersError::DataCorruption)?,
+            ))
+        })
+        .transpose()
     }
 
     pub fn get_block_hash(&self, height: usize) -> Result<Option<BlockHash>, CompactFiltersError> {
@@ -453,13 +452,12 @@ impl ChainStore<Full> {
 
         let key = StoreEntry::BlockHeader(Some(height)).get_key();
         let data = read_store.get_pinned_cf(cf_handle, key)?;
-        Ok(data
-            .map(|data| {
-                let (header, _): (BlockHeader, Uint256) =
-                    deserialize(&data).map_err(|_| CompactFiltersError::DataCorruption)?;
-                Ok::<_, CompactFiltersError>(header.block_hash())
-            })
-            .transpose()?)
+        data.map(|data| {
+            let (header, _): (BlockHeader, Uint256) =
+                deserialize(&data).map_err(|_| CompactFiltersError::DataCorruption)?;
+            Ok::<_, CompactFiltersError>(header.block_hash())
+        })
+        .transpose()
     }
 
     pub fn save_full_block(&self, block: &Block, height: usize) -> Result<(), CompactFiltersError> {
@@ -475,10 +473,10 @@ impl ChainStore<Full> {
         let key = StoreEntry::Block(Some(height)).get_key();
         let opt_block = read_store.get_pinned(key)?;
 
-        Ok(opt_block
+        opt_block
             .map(|data| deserialize(&data))
             .transpose()
-            .map_err(|_| CompactFiltersError::DataCorruption)?)
+            .map_err(|_| CompactFiltersError::DataCorruption)
     }
 
     pub fn delete_blocks_until(&self, height: usize) -> Result<(), CompactFiltersError> {
@@ -565,14 +563,14 @@ impl<T: StoreType> ChainStore<T> {
         let prefix = StoreEntry::BlockHeader(None).get_key();
         let iterator = read_store.prefix_iterator_cf(cf_handle, prefix);
 
-        Ok(iterator
+        iterator
             .last()
             .map(|(_, v)| -> Result<_, CompactFiltersError> {
                 let (header, _): (BlockHeader, Uint256) = SerializeDb::deserialize(&v)?;
 
                 Ok(header.block_hash())
             })
-            .transpose()?)
+            .transpose()
     }
 
     pub fn apply(
@@ -716,11 +714,11 @@ impl CFStore {
 
         // FIXME: we have to filter manually because rocksdb sometimes returns stuff that doesn't
         // have the right prefix
-        Ok(iterator
+        iterator
             .filter(|(k, _)| k.starts_with(&prefix))
             .skip(1)
             .map(|(_, data)| Ok::<_, CompactFiltersError>(BundleEntry::deserialize(&data)?.1))
-            .collect::<Result<_, _>>()?)
+            .collect::<Result<_, _>>()
     }
 
     pub fn replace_checkpoints(
diff --git a/src/descriptor/template.rs b/src/descriptor/template.rs
index 14c3aef7..8ad9eaf7 100644
--- a/src/descriptor/template.rs
+++ b/src/descriptor/template.rs
@@ -75,7 +75,7 @@ impl<T: DescriptorTemplate> IntoWalletDescriptor for T {
         secp: &SecpCtx,
         network: Network,
     ) -> Result<(ExtendedDescriptor, KeyMap), DescriptorError> {
-        Ok(self.build()?.into_wallet_descriptor(secp, network)?)
+        self.build()?.into_wallet_descriptor(secp, network)
     }
 }
 
@@ -108,7 +108,7 @@ pub struct P2PKH<K: IntoDescriptorKey<Legacy>>(pub K);
 
 impl<K: IntoDescriptorKey<Legacy>> DescriptorTemplate for P2PKH<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(descriptor!(pkh(self.0))?)
+        descriptor!(pkh(self.0))
     }
 }
 
@@ -142,7 +142,7 @@ pub struct P2WPKH_P2SH<K: IntoDescriptorKey<Segwitv0>>(pub K);
 
 impl<K: IntoDescriptorKey<Segwitv0>> DescriptorTemplate for P2WPKH_P2SH<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(descriptor!(sh(wpkh(self.0)))?)
+        descriptor!(sh(wpkh(self.0)))
     }
 }
 
@@ -175,7 +175,7 @@ pub struct P2WPKH<K: IntoDescriptorKey<Segwitv0>>(pub K);
 
 impl<K: IntoDescriptorKey<Segwitv0>> DescriptorTemplate for P2WPKH<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(descriptor!(wpkh(self.0))?)
+        descriptor!(wpkh(self.0))
     }
 }
 
@@ -210,7 +210,7 @@ pub struct BIP44<K: DerivableKey<Legacy>>(pub K, pub KeychainKind);
 
 impl<K: DerivableKey<Legacy>> DescriptorTemplate for BIP44<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(P2PKH(legacy::make_bipxx_private(44, self.0, self.1)?).build()?)
+        P2PKH(legacy::make_bipxx_private(44, self.0, self.1)?).build()
     }
 }
 
@@ -249,7 +249,7 @@ pub struct BIP44Public<K: DerivableKey<Legacy>>(pub K, pub bip32::Fingerprint, p
 
 impl<K: DerivableKey<Legacy>> DescriptorTemplate for BIP44Public<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(P2PKH(legacy::make_bipxx_public(44, self.0, self.1, self.2)?).build()?)
+        P2PKH(legacy::make_bipxx_public(44, self.0, self.1, self.2)?).build()
     }
 }
 
@@ -284,7 +284,7 @@ pub struct BIP49<K: DerivableKey<Segwitv0>>(pub K, pub KeychainKind);
 
 impl<K: DerivableKey<Segwitv0>> DescriptorTemplate for BIP49<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(P2WPKH_P2SH(segwit_v0::make_bipxx_private(49, self.0, self.1)?).build()?)
+        P2WPKH_P2SH(segwit_v0::make_bipxx_private(49, self.0, self.1)?).build()
     }
 }
 
@@ -323,7 +323,7 @@ pub struct BIP49Public<K: DerivableKey<Segwitv0>>(pub K, pub bip32::Fingerprint,
 
 impl<K: DerivableKey<Segwitv0>> DescriptorTemplate for BIP49Public<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(P2WPKH_P2SH(segwit_v0::make_bipxx_public(49, self.0, self.1, self.2)?).build()?)
+        P2WPKH_P2SH(segwit_v0::make_bipxx_public(49, self.0, self.1, self.2)?).build()
     }
 }
 
@@ -358,7 +358,7 @@ pub struct BIP84<K: DerivableKey<Segwitv0>>(pub K, pub KeychainKind);
 
 impl<K: DerivableKey<Segwitv0>> DescriptorTemplate for BIP84<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(P2WPKH(segwit_v0::make_bipxx_private(84, self.0, self.1)?).build()?)
+        P2WPKH(segwit_v0::make_bipxx_private(84, self.0, self.1)?).build()
     }
 }
 
@@ -397,7 +397,7 @@ pub struct BIP84Public<K: DerivableKey<Segwitv0>>(pub K, pub bip32::Fingerprint,
 
 impl<K: DerivableKey<Segwitv0>> DescriptorTemplate for BIP84Public<K> {
     fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
-        Ok(P2WPKH(segwit_v0::make_bipxx_public(84, self.0, self.1, self.2)?).build()?)
+        P2WPKH(segwit_v0::make_bipxx_public(84, self.0, self.1, self.2)?).build()
     }
 }
 
diff --git a/src/keys/mod.rs b/src/keys/mod.rs
index 0ce5d128..6edb419c 100644
--- a/src/keys/mod.rs
+++ b/src/keys/mod.rs
@@ -733,7 +733,7 @@ fn expand_multi_keys<Pk: IntoDescriptorKey<Ctx>, Ctx: ScriptContext>(
 ) -> Result<(Vec<DescriptorPublicKey>, KeyMap, ValidNetworks), KeyError> {
     let (pks, key_maps_networks): (Vec<_>, Vec<_>) = pks
         .into_iter()
-        .map(|key| Ok::<_, KeyError>(key.into_descriptor_key()?.extract(secp)?))
+        .map(|key| key.into_descriptor_key()?.extract(secp))
         .collect::<Result<Vec<_>, _>>()?
         .into_iter()
         .map(|(a, b, c)| (a, (b, c)))

From e35601bb19ec2295d4100ef6d3d5eb1756fd075e Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 10 Feb 2021 10:25:46 +1100
Subject: [PATCH 14/21] Use vec! instead of mut and push

As suggested by Clippy, use the `vec!` macro directly instead of
declaring a mutable vector and pushing elements onto it.
---
 src/descriptor/dsl.rs      | 18 ++++++++++--------
 src/descriptor/template.rs | 10 +++++-----
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/descriptor/dsl.rs b/src/descriptor/dsl.rs
index b3f62ae5..405be39f 100644
--- a/src/descriptor/dsl.rs
+++ b/src/descriptor/dsl.rs
@@ -228,10 +228,11 @@ macro_rules! impl_sortedmulti {
         use $crate::keys::IntoDescriptorKey;
         let secp = $crate::bitcoin::secp256k1::Secp256k1::new();
 
-        let mut keys = vec![];
-        $(
-            keys.push($key.into_descriptor_key());
-        )*
+        let keys = vec![
+            $(
+                $key.into_descriptor_key(),
+            )*
+        ];
 
         keys.into_iter().collect::<Result<Vec<_>, _>>()
             .map_err($crate::descriptor::DescriptorError::Key)
@@ -656,10 +657,11 @@ macro_rules! fragment {
         use $crate::keys::IntoDescriptorKey;
         let secp = $crate::bitcoin::secp256k1::Secp256k1::new();
 
-        let mut keys = vec![];
-        $(
-            keys.push($key.into_descriptor_key());
-        )*
+        let keys = vec![
+            $(
+                $key.into_descriptor_key(),
+            )*
+        ];
 
         keys.into_iter().collect::<Result<Vec<_>, _>>()
             .map_err($crate::descriptor::DescriptorError::Key)
diff --git a/src/descriptor/template.rs b/src/descriptor/template.rs
index 8ad9eaf7..3fdbec89 100644
--- a/src/descriptor/template.rs
+++ b/src/descriptor/template.rs
@@ -440,11 +440,11 @@ macro_rules! expand_make_bipxx {
                     KeychainKind::Internal => vec![bip32::ChildNumber::from_normal_idx(1)?].into(),
                 };
 
-                let mut source_path = Vec::with_capacity(3);
-                source_path.push(bip32::ChildNumber::from_hardened_idx(bip)?);
-                source_path.push(bip32::ChildNumber::from_hardened_idx(0)?);
-                source_path.push(bip32::ChildNumber::from_hardened_idx(0)?);
-                let source_path: bip32::DerivationPath = source_path.into();
+                let source_path = bip32::DerivationPath::from(vec![
+                    bip32::ChildNumber::from_hardened_idx(bip)?,
+                    bip32::ChildNumber::from_hardened_idx(0)?,
+                    bip32::ChildNumber::from_hardened_idx(0)?,
+                ]);
 
                 Ok((key, (parent_fingerprint, source_path), derivation_path))
             }

From bfe29c4ef60b52a71123274f445205ee01b2504b Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 10 Feb 2021 11:36:12 +1100
Subject: [PATCH 15/21] Use map instead of and_then

As suggested by Clippy us `map` instead of `and_then` with an inner
option.
---
 examples/compiler.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/compiler.rs b/examples/compiler.rs
index 3b62147c..284aa12e 100644
--- a/examples/compiler.rs
+++ b/examples/compiler.rs
@@ -97,7 +97,7 @@ fn main() -> Result<(), Box<dyn Error>> {
 
     let network = matches
         .value_of("network")
-        .and_then(|n| Some(Network::from_str(n)))
+        .map(|n| Network::from_str(n))
         .transpose()
         .unwrap()
         .unwrap_or(Network::Testnet);

From 2b5e177ab2156a6aacde4753be9986816adfd654 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 10 Feb 2021 11:41:50 +1100
Subject: [PATCH 16/21] Use lazy_static

`lazy_static` is not imported, this error is hidden behind the
`compact_filters` feature flag.
---
 src/blockchain/compact_filters/store.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/blockchain/compact_filters/store.rs b/src/blockchain/compact_filters/store.rs
index 3b879194..bc81f066 100644
--- a/src/blockchain/compact_filters/store.rs
+++ b/src/blockchain/compact_filters/store.rs
@@ -46,6 +46,8 @@ use bitcoin::BlockHash;
 use bitcoin::BlockHeader;
 use bitcoin::Network;
 
+use lazy_static::lazy_static;
+
 use super::CompactFiltersError;
 
 lazy_static! {

From 97ad0f1b4fb27fe7f045d28d3b051ca73acebb5d Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 10 Feb 2021 11:48:53 +1100
Subject: [PATCH 17/21] Remove unused macro_use

Found by Clippy, we don't need this `macro_use` statement.
---
 src/lib.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/lib.rs b/src/lib.rs
index df6cf7c6..c8af648e 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -225,7 +225,6 @@ extern crate async_trait;
 extern crate bdk_macros;
 
 #[cfg(feature = "compact_filters")]
-#[macro_use]
 extern crate lazy_static;
 
 #[cfg(feature = "electrum")]

From bdb2a535975d7707aaab6ac036aa445727db22e6 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 10 Feb 2021 11:18:39 +1100
Subject: [PATCH 18/21] Add cargo check script

We build against various targets on CI, in order to not abuse CI its
nice to see if code is good before pushing.

Add a script that runs `cargo check` with various combinations of
features and targets in order to enable thorough checking of the project
source code during development.
---
 scripts/cargo-check.sh | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100755 scripts/cargo-check.sh

diff --git a/scripts/cargo-check.sh b/scripts/cargo-check.sh
new file mode 100755
index 00000000..01b94411
--- /dev/null
+++ b/scripts/cargo-check.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+#
+# Run various invocations of cargo check
+
+features=( "default" "compiler" "electrum" "esplora" "compact_filters" "key-value-db" "async-interface" "all-keys" "keys-bip39" )
+toolchains=( "+stable" "+1.45" "+nightly" )
+
+main() {
+    check_src
+    check_all_targets
+}
+
+# Check with all features, with various toolchains.
+check_src() {
+    for toolchain in "${toolchains[@]}"; do
+        cmd="cargo $toolchain clippy --all-targets --no-default-features"
+
+        for feature in "${features[@]}"; do
+            touch_files
+            $cmd --features "$feature"
+        done
+    done
+}
+
+# Touch files to prevent cached warnings from not showing up.
+touch_files() {
+    touch $(find . -name *.rs)
+}
+
+main
+exit 0

From d2a094aa4c23b46cff4735327a34471cfef5735e Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 24 Feb 2021 12:32:14 +1100
Subject: [PATCH 19/21] Align multi-line string

Recently we shortened the first line of a multi-line string and failed
to re-align the rest of the lines.

Thanks @afilini
---
 src/wallet/tx_builder.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs
index 9a9b8d2e..c887672e 100644
--- a/src/wallet/tx_builder.rs
+++ b/src/wallet/tx_builder.rs
@@ -630,11 +630,11 @@ impl ChangeSpendPolicy {
 #[cfg(test)]
 mod test {
     const ORDERING_TEST_TX: &str = "0200000003c26f3eb7932f7acddc5ddd26602b77e7516079b03090a16e2c2f54\
-                                            85d1fd600f0100000000ffffffffc26f3eb7932f7acddc5ddd26602b77e75160\
-                                            79b03090a16e2c2f5485d1fd600f0000000000ffffffff571fb3e02278217852\
-                                            dd5d299947e2b7354a639adc32ec1fa7b82cfb5dec530e0500000000ffffffff\
-                                            03e80300000000000002aaeee80300000000000001aa200300000000000001ff\
-                                            00000000";
+                                    85d1fd600f0100000000ffffffffc26f3eb7932f7acddc5ddd26602b77e75160\
+                                    79b03090a16e2c2f5485d1fd600f0000000000ffffffff571fb3e02278217852\
+                                    dd5d299947e2b7354a639adc32ec1fa7b82cfb5dec530e0500000000ffffffff\
+                                    03e80300000000000002aaeee80300000000000001aa200300000000000001ff\
+                                    00000000";
     macro_rules! ordering_test_tx {
         () => {
             deserialize::<bitcoin::Transaction>(&Vec::<u8>::from_hex(ORDERING_TEST_TX).unwrap())

From a838c2bacc4e64308d8ba9cfb728bb412b4bdd0d Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 24 Feb 2021 13:23:17 +1100
Subject: [PATCH 20/21] Use id() for DummySigner comparison

If we give the `DummySigner` a valid identifier then we can use this to
do comparison.

Half the time we do comparison we only have a `dyn Signer` so we cannot
use `PartialEq`, add a helper function to check equality (this is in
test code so its not toooo ugly).

Thanks @afilini for the suggestion.
---
 src/wallet/signer.rs | 74 ++++++++++++++++----------------------------
 1 file changed, 26 insertions(+), 48 deletions(-)

diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs
index b076b981..a573c6e1 100644
--- a/src/wallet/signer.rs
+++ b/src/wallet/signer.rs
@@ -569,19 +569,9 @@ mod signers_container_tests {
     use miniscript::ScriptContext;
     use std::str::FromStr;
 
-    // Return true if data field of `fat` pointer is the same address as `thin` pointer.
-    fn is_equal(fat: &Arc<dyn Signer>, thin: &Arc<DummySigner>) -> bool {
-        let (left, right) = raw_pointers(fat, thin);
-        left == right
-    }
-
-    // Make pointer values out of `fat` and `thin` that can be compared.
-    fn raw_pointers(fat: &Arc<dyn Signer>, thin: &Arc<DummySigner>) -> (*const u8, *const u8) {
-        let (left, _vtable): (*const u8, *const u8) =
-            unsafe { std::mem::transmute(Arc::as_ptr(fat)) };
-        let right = Arc::as_ptr(thin) as *const u8;
-
-        (left, right)
+    fn is_equal(this: &Arc<dyn Signer>, that: &Arc<DummySigner>) -> bool {
+        let secp = Secp256k1::new();
+        this.id(&secp) == that.id(&secp)
     }
 
     // Signers added with the same ordering (like `Ordering::default`) created from `KeyMap`
@@ -608,49 +598,34 @@ mod signers_container_tests {
     #[test]
     fn signers_sorted_by_ordering() {
         let mut signers = SignersContainer::new();
-        let signer1 = Arc::new(DummySigner);
-        let signer2 = Arc::new(DummySigner);
-        let signer3 = Arc::new(DummySigner);
+        let signer1 = Arc::new(DummySigner { number: 1 });
+        let signer2 = Arc::new(DummySigner { number: 2 });
+        let signer3 = Arc::new(DummySigner { number: 3 });
 
-        signers.add_external(
-            SignerId::Fingerprint(b"cafe"[..].into()),
-            SignerOrdering(1),
-            signer1.clone(),
-        );
-        signers.add_external(
-            SignerId::Fingerprint(b"babe"[..].into()),
-            SignerOrdering(2),
-            signer2.clone(),
-        );
-        signers.add_external(
-            SignerId::Fingerprint(b"feed"[..].into()),
-            SignerOrdering(3),
-            signer3.clone(),
-        );
+        signers.add_external(SignerId::Dummy(1), SignerOrdering(1), signer1.clone());
+        signers.add_external(SignerId::Dummy(2), SignerOrdering(2), signer2.clone());
+        signers.add_external(SignerId::Dummy(3), SignerOrdering(3), signer3.clone());
 
         // Check that signers are sorted from lowest to highest ordering
         let signers = signers.signers();
 
-        let (left, right) = raw_pointers(signers[0], &signer1);
-        assert_eq!(left, right);
-        let (left, right) = raw_pointers(signers[1], &signer2);
-        assert_eq!(left, right);
-        let (left, right) = raw_pointers(signers[2], &signer3);
-        assert_eq!(left, right);
+        assert!(is_equal(signers[0], &signer1));
+        assert!(is_equal(signers[1], &signer2));
+        assert!(is_equal(signers[2], &signer3));
     }
 
     #[test]
     fn find_signer_by_id() {
         let mut signers = SignersContainer::new();
-        let signer1 = Arc::new(DummySigner);
-        let signer2 = Arc::new(DummySigner);
-        let signer3 = Arc::new(DummySigner);
-        let signer4 = Arc::new(DummySigner);
+        let signer1 = Arc::new(DummySigner { number: 1 });
+        let signer2 = Arc::new(DummySigner { number: 2 });
+        let signer3 = Arc::new(DummySigner { number: 3 });
+        let signer4 = Arc::new(DummySigner { number: 3 }); // Same ID as `signer3` but will use lower ordering.
 
-        let id1 = SignerId::Fingerprint(b"cafe"[..].into());
-        let id2 = SignerId::Fingerprint(b"babe"[..].into());
-        let id3 = SignerId::Fingerprint(b"feed"[..].into());
-        let id_nonexistent = SignerId::Fingerprint(b"fefe"[..].into());
+        let id1 = SignerId::Dummy(1);
+        let id2 = SignerId::Dummy(2);
+        let id3 = SignerId::Dummy(3);
+        let id_nonexistent = SignerId::Dummy(999);
 
         signers.add_external(id1.clone(), SignerOrdering(1), signer1.clone());
         signers.add_external(id2.clone(), SignerOrdering(2), signer2.clone());
@@ -669,8 +644,11 @@ mod signers_container_tests {
         assert!(matches!(signers.find(id_nonexistent), None));
     }
 
-    #[derive(Debug)]
-    struct DummySigner;
+    #[derive(Debug, Clone, Copy)]
+    struct DummySigner {
+        number: u64,
+    }
+
     impl Signer for DummySigner {
         fn sign(
             &self,
@@ -682,7 +660,7 @@ mod signers_container_tests {
         }
 
         fn id(&self, _secp: &SecpCtx) -> SignerId {
-            SignerId::Dummy(42)
+            SignerId::Dummy(self.number)
         }
 
         fn sign_whole_tx(&self) -> bool {

From bda416df0acee8e0df2e32227a94aceb0c08e2a5 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Wed, 24 Feb 2021 13:39:36 +1100
Subject: [PATCH 21/21] Use mixed order insertions

Currently we have a unit test to test that signers are sorted by
ordering. We call `add_external` to add them but currently we add them
in the same order we expect them to be in. This means if the
implementation happens to insert them simply in the order they are
added (i.e. insert to end of list) then this test will still pass.

Insert in a mixed order, including one lower followed by one higher -
this ensures we are not inserting at the front or at the back but are
actually sorting based on the `SignerOrdering`.
---
 src/wallet/signer.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs
index a573c6e1..eebafa58 100644
--- a/src/wallet/signer.rs
+++ b/src/wallet/signer.rs
@@ -602,8 +602,9 @@ mod signers_container_tests {
         let signer2 = Arc::new(DummySigner { number: 2 });
         let signer3 = Arc::new(DummySigner { number: 3 });
 
-        signers.add_external(SignerId::Dummy(1), SignerOrdering(1), signer1.clone());
+        // Mixed order insertions verifies we are not inserting at head or tail.
         signers.add_external(SignerId::Dummy(2), SignerOrdering(2), signer2.clone());
+        signers.add_external(SignerId::Dummy(1), SignerOrdering(1), signer1.clone());
         signers.add_external(SignerId::Dummy(3), SignerOrdering(3), signer3.clone());
 
         // Check that signers are sorted from lowest to highest ordering