Merge bitcoindevkit/bdk#1186: Clean up clippy allows

1c15cb2f9169bb08a4128c183eeeca80fff97ab7 ref(example_cli): Add new struct Init (vmammal)
89a7ddca7f6ed8c65ed0124774f3a868d08faf68 ref(esplora): `Box` a large `esplora_client::Error` (vmammal)
097d818d4c7fff12d72ce0ef10ceb9b575c154e1 ref(wallet): `Wallet::preselect_utxos` now accepts a `&TxParams` (vmammal)
f11d663b7efb98dd72fed903ade8c5e7af0b5a3a ref(psbt): refactor body of `get_utxo_for` to address `clippy::manual_map` (vmammal)
4679ca1df7209d2b356fbb8cbd675f07756d1301 ref(example_cli): add typedefs to reduce type complexity (vmammal)
64a90192d9d49c3df22d2b5f489ba20222fd941a refactor: remove old clippy allow attributes (vmammal)

Pull request description:

  closes #1127

  There are several instances in the code where we allow clippy lints that would otherwise be flagged during regular checks. It would be preferable to minimize the number of "clippy allow" attributes either by fixing the affected areas or setting a specific configuration in `clippy.toml`. In cases where we have to allow a particular lint, it should be documented why the lint doesn't apply.

  For context see https://github.com/bitcoindevkit/bdk/issues/1127#issuecomment-1784256647 as well as the commit message details.

  One area I'm unsure of is whether `Box`ing a large error in 4fc2216 is the right approach. Logically it makes sense to avoid allocating a needlessly heavy `Result`, but I haven't studied the implications or tradeoffs of such a change.

  ### Checklists

  #### All Submissions:

  * [x] 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

ACKs for top commit:
  evanlinjin:
    ACK 1c15cb2f9169bb08a4128c183eeeca80fff97ab7

Tree-SHA512: 5fa3796a33678651414e7aad7ef8309b4cbe2a9ab00dce094964b40784edb2f46a44067785d95ea26f4cd88d593420485be94c9b09ac589f632453fbd8c94d85

crates/bdk/src/psbt/mod.rs

@@ -35,24 +35,16 @@ pub trait PsbtUtils {
 }
 
 impl PsbtUtils for Psbt {
-    #[allow(clippy::all)] // We want to allow `manual_map` but it is too new.
     fn get_utxo_for(&self, input_index: usize) -> Option<TxOut> {
         let tx = &self.unsigned_tx;
+        let input = self.inputs.get(input_index)?;
 
-        if input_index >= tx.input.len() {
+        match (&input.witness_utxo, &input.non_witness_utxo) {
-            return None;
+            (Some(_), _) => input.witness_utxo.clone(),
-        }
+            (_, Some(_)) => input.non_witness_utxo.as_ref().map(|in_tx| {
-
+                in_tx.output[tx.input[input_index].previous_output.vout as usize].clone()
-        if let Some(input) = self.inputs.get(input_index) {
+            }),
-            if let Some(wit_utxo) = &input.witness_utxo {
+            _ => None,
-                Some(wit_utxo.clone())
-            } else if let Some(in_tx) = &input.non_witness_utxo {
-                Some(in_tx.output[tx.input[input_index].previous_output.vout as usize].clone())
-            } else {
-                None
-            }
-        } else {
-            None
         }
     }
 

crates/bdk/src/wallet/mod.rs

@@ -12,7 +12,7 @@
 //! Wallet
 //!
 //! This module defines the [`Wallet`].
-use crate::collections::{BTreeMap, HashMap, HashSet};
+use crate::collections::{BTreeMap, HashMap};
 use alloc::{
     boxed::Box,
     string::{String, ToString},
@@ -1518,15 +1518,8 @@ impl<D> Wallet<D> {
             return Err(CreateTxError::ChangePolicyDescriptor);
         }
 
-        let (required_utxos, optional_utxos) = self.preselect_utxos(
+        let (required_utxos, optional_utxos) =
-            params.change_policy,
+            self.preselect_utxos(&params, Some(current_height.to_consensus_u32()));
-            &params.unspendable,
-            params.utxos.clone(),
-            params.drain_wallet,
-            params.manually_selected_only,
-            params.bumping_fee.is_some(), // we mandate confirmed transactions if we're bumping the fee
-            Some(current_height.to_consensus_u32()),
-        );
 
         // get drain script
         let drain_script = match params.drain_to {
@@ -2063,17 +2056,26 @@ impl<D> Wallet<D> {
 
     /// Given the options returns the list of utxos that must be used to form the
     /// transaction and any further that may be used if needed.
-    #[allow(clippy::too_many_arguments)]
     fn preselect_utxos(
         &self,
-        change_policy: tx_builder::ChangeSpendPolicy,
+        params: &TxParams,
-        unspendable: &HashSet<OutPoint>,
-        manually_selected: Vec<WeightedUtxo>,
-        must_use_all_available: bool,
-        manual_only: bool,
-        must_only_use_confirmed_tx: bool,
         current_height: Option<u32>,
     ) -> (Vec<WeightedUtxo>, Vec<WeightedUtxo>) {
+        let TxParams {
+            change_policy,
+            unspendable,
+            utxos,
+            drain_wallet,
+            manually_selected_only,
+            bumping_fee,
+            ..
+        } = params;
+
+        let manually_selected = utxos.clone();
+        // we mandate confirmed transactions if we're bumping the fee
+        let must_only_use_confirmed_tx = bumping_fee.is_some();
+        let must_use_all_available = *drain_wallet;
+
         let chain_tip = self.chain.tip().block_id();
         //    must_spend <- manually selected utxos
         //    may_spend  <- all other available utxos
@@ -2088,7 +2090,7 @@ impl<D> Wallet<D> {
 
         // NOTE: we are intentionally ignoring `unspendable` here. i.e manual
         // selection overrides unspendable.
-        if manual_only {
+        if *manually_selected_only {
             return (must_spend, vec![]);
         }
 
@@ -2290,9 +2292,6 @@ impl<D> Wallet<D> {
     ) -> Result<(), MiniscriptPsbtError> {
         // We need to borrow `psbt` mutably within the loops, so we have to allocate a vec for all
         // the input utxos and outputs
-        //
-        // Clippy complains that the collect is not required, but that's wrong
-        #[allow(clippy::needless_collect)]
         let utxos = (0..psbt.inputs.len())
             .filter_map(|i| psbt.get_utxo_for(i).map(|utxo| (true, i, utxo)))
             .chain(

crates/bdk/src/wallet/signer.rs

@@ -820,7 +820,6 @@ pub enum TapLeavesOptions {
     None,
 }
 
-#[allow(clippy::derivable_impls)]
 impl Default for SignOptions {
     fn default() -> Self {
         SignOptions {

crates/esplora/src/async_ext.rs

@@ -6,11 +6,14 @@ use bdk_chain::{
     local_chain::{self, CheckPoint},
     BlockId, ConfirmationTimeHeightAnchor, TxGraph,
 };
-use esplora_client::{Error, TxStatus};
+use esplora_client::TxStatus;
 use futures::{stream::FuturesOrdered, TryStreamExt};
 
 use crate::anchor_from_status;
 
+/// [`esplora_client::Error`]
+type Error = Box<esplora_client::Error>;
+
 /// Trait to extend the functionality of [`esplora_client::AsyncClient`].
 ///
 /// Refer to [crate-level documentation] for more.
@@ -35,7 +38,6 @@ pub trait EsploraAsyncExt {
     /// [`LocalChain`]: bdk_chain::local_chain::LocalChain
     /// [`LocalChain::tip`]: bdk_chain::local_chain::LocalChain::tip
     /// [`LocalChain::apply_update`]: bdk_chain::local_chain::LocalChain::apply_update
-    #[allow(clippy::result_large_err)]
     async fn update_local_chain(
         &self,
         local_tip: CheckPoint,
@@ -50,7 +52,6 @@ pub trait EsploraAsyncExt {
     /// The full scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated
     /// transactions. `parallel_requests` specifies the max number of HTTP requests to make in
     /// parallel.
-    #[allow(clippy::result_large_err)]
     async fn full_scan<K: Ord + Clone + Send>(
         &self,
         keychain_spks: BTreeMap<
@@ -73,7 +74,6 @@ pub trait EsploraAsyncExt {
     /// may include scripts that have been used, use [`full_scan`] with the keychain.
     ///
     /// [`full_scan`]: EsploraAsyncExt::full_scan
-    #[allow(clippy::result_large_err)]
     async fn sync(
         &self,
         misc_spks: impl IntoIterator<IntoIter = impl Iterator<Item = ScriptBuf> + Send> + Send,

crates/esplora/src/blocking_ext.rs

@@ -7,10 +7,13 @@ use bdk_chain::{
     local_chain::{self, CheckPoint},
     BlockId, ConfirmationTimeHeightAnchor, TxGraph,
 };
-use esplora_client::{Error, TxStatus};
+use esplora_client::TxStatus;
 
 use crate::anchor_from_status;
 
+/// [`esplora_client::Error`]
+type Error = Box<esplora_client::Error>;
+
 /// Trait to extend the functionality of [`esplora_client::BlockingClient`].
 ///
 /// Refer to [crate-level documentation] for more.
@@ -33,7 +36,6 @@ pub trait EsploraExt {
     /// [`LocalChain`]: bdk_chain::local_chain::LocalChain
     /// [`LocalChain::tip`]: bdk_chain::local_chain::LocalChain::tip
     /// [`LocalChain::apply_update`]: bdk_chain::local_chain::LocalChain::apply_update
-    #[allow(clippy::result_large_err)]
     fn update_local_chain(
         &self,
         local_tip: CheckPoint,
@@ -48,7 +50,6 @@ pub trait EsploraExt {
     /// The full scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated
     /// transactions. `parallel_requests` specifies the max number of HTTP requests to make in
     /// parallel.
-    #[allow(clippy::result_large_err)]
     fn full_scan<K: Ord + Clone>(
         &self,
         keychain_spks: BTreeMap<K, impl IntoIterator<Item = (u32, ScriptBuf)>>,
@@ -68,7 +69,6 @@ pub trait EsploraExt {
     /// may include scripts that have been used, use [`full_scan`] with the keychain.
     ///
     /// [`full_scan`]: EsploraExt::full_scan
-    #[allow(clippy::result_large_err)]
     fn sync(
         &self,
         misc_spks: impl IntoIterator<Item = ScriptBuf>,
@@ -247,7 +247,12 @@ impl EsploraExt for esplora_client::BlockingClient {
                 .map(|txid| {
                     std::thread::spawn({
                         let client = self.clone();
-                        move || client.get_tx_status(&txid).map(|s| (txid, s))
+                        move || {
+                            client
+                                .get_tx_status(&txid)
+                                .map_err(Box::new)
+                                .map(|s| (txid, s))
+                        }
                     })
                 })
                 .collect::<Vec<JoinHandle<Result<(Txid, TxStatus), Error>>>>();

example-crates/example_bitcoind_rpc_polling/src/main.rs

@@ -110,9 +110,13 @@ enum RpcCommands {
 
 fn main() -> anyhow::Result<()> {
     let start = Instant::now();
-
+    let example_cli::Init {
-    let (args, keymap, index, db, init_changeset) =
+        args,
-        example_cli::init::<RpcCommands, RpcArgs, ChangeSet>(DB_MAGIC, DB_PATH)?;
+        keymap,
+        index,
+        db,
+        init_changeset,
+    } = example_cli::init::<RpcCommands, RpcArgs, ChangeSet>(DB_MAGIC, DB_PATH)?;
     println!(
         "[{:>10}s] loaded initial changeset from db",
         start.elapsed().as_secs_f32()

example-crates/example_cli/src/lib.rs

@@ -53,7 +53,6 @@ pub struct Args<CS: clap::Subcommand, S: clap::Args> {
     pub command: Commands<CS, S>,
 }
 
-#[allow(clippy::almost_swapped)]
 #[derive(Subcommand, Debug, Clone)]
 pub enum Commands<CS: clap::Subcommand, S: clap::Args> {
     #[clap(flatten)]
@@ -137,7 +136,6 @@ impl core::fmt::Display for CoinSelectionAlgo {
     }
 }
 
-#[allow(clippy::almost_swapped)]
 #[derive(Subcommand, Debug, Clone)]
 pub enum AddressCmd {
     /// Get the next unused address.
@@ -190,7 +188,12 @@ impl core::fmt::Display for Keychain {
     }
 }
 
-#[allow(clippy::type_complexity)]
+pub struct CreateTxChange {
+    pub index_changeset: keychain::ChangeSet<Keychain>,
+    pub change_keychain: Keychain,
+    pub index: u32,
+}
+
 pub fn create_tx<A: Anchor, O: ChainOracle>(
     graph: &mut KeychainTxGraph<A>,
     chain: &O,
@@ -198,10 +201,7 @@ pub fn create_tx<A: Anchor, O: ChainOracle>(
     cs_algorithm: CoinSelectionAlgo,
     address: Address,
     value: u64,
-) -> anyhow::Result<(
+) -> anyhow::Result<(Transaction, Option<CreateTxChange>)>
-    Transaction,
-    Option<(keychain::ChangeSet<Keychain>, (Keychain, u32))>,
-)>
 where
     O::Error: std::error::Error + Send + Sync + 'static,
 {
@@ -393,7 +393,11 @@ where
     }
 
     let change_info = if selection_meta.drain_value.is_some() {
-        Some((changeset, (internal_keychain, change_index)))
+        Some(CreateTxChange {
+            index_changeset: changeset,
+            change_keychain: internal_keychain,
+            index: change_index,
+        })
     } else {
         None
     };
@@ -401,35 +405,34 @@ where
     Ok((transaction, change_info))
 }
 
-#[allow(clippy::type_complexity)]
+// Alias the elements of `Result` of `planned_utxos`
+pub type PlannedUtxo<K, A> = (bdk_tmp_plan::Plan<K>, FullTxOut<A>);
+
 pub fn planned_utxos<A: Anchor, O: ChainOracle, K: Clone + bdk_tmp_plan::CanDerive>(
     graph: &KeychainTxGraph<A>,
     chain: &O,
     assets: &bdk_tmp_plan::Assets<K>,
-) -> Result<Vec<(bdk_tmp_plan::Plan<K>, FullTxOut<A>)>, O::Error> {
+) -> Result<Vec<PlannedUtxo<K, A>>, O::Error> {
     let chain_tip = chain.get_chain_tip()?;
     let outpoints = graph.index.outpoints().iter().cloned();
     graph
         .graph()
         .try_filter_chain_unspents(chain, chain_tip, outpoints)
-        .filter_map(
+        .filter_map(|r| -> Option<Result<PlannedUtxo<K, A>, _>> {
-            #[allow(clippy::type_complexity)]
+            let (k, i, full_txo) = match r {
-            |r| -> Option<Result<(bdk_tmp_plan::Plan<K>, FullTxOut<A>), _>> {
+                Err(err) => return Some(Err(err)),
-                let (k, i, full_txo) = match r {
+                Ok(((k, i), full_txo)) => (k, i, full_txo),
-                    Err(err) => return Some(Err(err)),
+            };
-                    Ok(((k, i), full_txo)) => (k, i, full_txo),
+            let desc = graph
-                };
+                .index
-                let desc = graph
+                .keychains()
-                    .index
+                .get(&k)
-                    .keychains()
+                .expect("keychain must exist")
-                    .get(&k)
+                .at_derivation_index(i)
-                    .expect("keychain must exist")
+                .expect("i can't be hardened");
-                    .at_derivation_index(i)
+            let plan = bdk_tmp_plan::plan_satisfaction(&desc, assets)?;
-                    .expect("i can't be hardened");
+            Some(Ok((plan, full_txo)))
-                let plan = bdk_tmp_plan::plan_satisfaction(&desc, assets)?;
+        })
-                Some(Ok((plan, full_txo)))
-            },
-        )
         .collect()
 }
 
@@ -599,7 +602,12 @@ where
                 let (tx, change_info) =
                     create_tx(graph, chain, keymap, coin_select, address, value)?;
 
-                if let Some((index_changeset, (change_keychain, index))) = change_info {
+                if let Some(CreateTxChange {
+                    index_changeset,
+                    change_keychain,
+                    index,
+                }) = change_info
+                {
                     // We must first persist to disk the fact that we've got a new address from the
                     // change keychain so future scans will find the tx we're about to broadcast.
                     // If we're unable to persist this, then we don't want to broadcast.
@@ -648,17 +656,26 @@ where
     }
 }
 
-#[allow(clippy::type_complexity)]
+/// The initial state returned by [`init`].
+pub struct Init<CS: clap::Subcommand, S: clap::Args, C> {
+    /// Arguments parsed by the cli.
+    pub args: Args<CS, S>,
+    /// Descriptor keymap.
+    pub keymap: KeyMap,
+    /// Keychain-txout index.
+    pub index: KeychainTxOutIndex<Keychain>,
+    /// Persistence backend.
+    pub db: Mutex<Database<C>>,
+    /// Initial changeset.
+    pub init_changeset: C,
+}
+
+/// Parses command line arguments and initializes all components, creating
+/// a file store with the given parameters, or loading one if it exists.
 pub fn init<CS: clap::Subcommand, S: clap::Args, C>(
     db_magic: &[u8],
     db_default_path: &str,
-) -> anyhow::Result<(
+) -> anyhow::Result<Init<CS, S, C>>
-    Args<CS, S>,
-    KeyMap,
-    KeychainTxOutIndex<Keychain>,
-    Mutex<Database<C>>,
-    C,
-)>
 where
     C: Default + Append + Serialize + DeserializeOwned,
 {
@@ -692,11 +709,11 @@ where
 
     let init_changeset = db_backend.load_from_persistence()?.unwrap_or_default();
 
-    Ok((
+    Ok(Init {
         args,
         keymap,
         index,
-        Mutex::new(Database::new(db_backend)),
+        db: Mutex::new(Database::new(db_backend)),
         init_changeset,
-    ))
+    })
 }

example-crates/example_electrum/src/main.rs

@@ -103,8 +103,15 @@ type ChangeSet = (
 );
 
 fn main() -> anyhow::Result<()> {
-    let (args, keymap, index, db, (disk_local_chain, disk_tx_graph)) =
+    let example_cli::Init {
-        example_cli::init::<ElectrumCommands, ElectrumArgs, ChangeSet>(DB_MAGIC, DB_PATH)?;
+        args,
+        keymap,
+        index,
+        db,
+        init_changeset,
+    } = example_cli::init::<ElectrumCommands, ElectrumArgs, ChangeSet>(DB_MAGIC, DB_PATH)?;
+
+    let (disk_local_chain, disk_tx_graph) = init_changeset;
 
     let graph = Mutex::new({
         let mut graph = IndexedTxGraph::new(index);

example-crates/example_esplora/src/main.rs

@@ -99,8 +99,13 @@ pub struct ScanOptions {
 }
 
 fn main() -> anyhow::Result<()> {
-    let (args, keymap, index, db, init_changeset) =
+    let example_cli::Init {
-        example_cli::init::<EsploraCommands, EsploraArgs, ChangeSet>(DB_MAGIC, DB_PATH)?;
+        args,
+        keymap,
+        index,
+        db,
+        init_changeset,
+    } = example_cli::init::<EsploraCommands, EsploraArgs, ChangeSet>(DB_MAGIC, DB_PATH)?;
 
     let genesis_hash = genesis_block(args.network).block_hash();