Various tweaks to code arrangement and documentation

As per reviews by @danielabrozzoni and @LLFourn

crates/bdk/src/wallet/mod.rs

@@ -501,20 +501,7 @@ impl<D> Wallet<D> {
         Ok(changed)
     }
 
-    #[deprecated(note = "use Wallet::transactions instead")]
+    /// Iterate over the transactions in the wallet.
-    /// Deprecated. use `Wallet::transactions` instead.
-    pub fn list_transactions(
-        &self,
-        include_raw: bool,
-    ) -> impl Iterator<Item = TransactionDetails> + '_ {
-        self.indexed_graph
-            .graph()
-            .list_chain_txs(&self.chain, self.chain.tip().unwrap_or_default())
-            .map(move |canonical_tx| new_tx_details(&self.indexed_graph, canonical_tx, include_raw))
-    }
-
-    /// Iterate over the transactions in the wallet in order of ascending confirmation time with
-    /// unconfirmed transactions last.
     pub fn transactions(
         &self,
     ) -> impl Iterator<Item = CanonicalTx<'_, Transaction, ConfirmationTimeAnchor>> + '_ {
@@ -1685,6 +1672,8 @@ impl<D> Wallet<D> {
 
     /// Applies an update to the wallet and stages the changes (but does not [`commit`] them).
     ///
+    /// This returns whether the `update` resulted in any changes.
+    ///
     /// Usually you create an `update` by interacting with some blockchain data source and inserting
     /// transactions related to your wallet into it.
     ///
@@ -1706,7 +1695,10 @@ impl<D> Wallet<D> {
         Ok(changed)
     }
 
-    /// Commits all curently [`staged`] changed to the persistence backend returning and error when this fails.
+    /// Commits all curently [`staged`] changed to the persistence backend returning and error when
+    /// this fails.
+    ///
+    /// This returns whether the `update` resulted in any changes.
     ///
     /// [`staged`]: Self::staged
     pub fn commit(&mut self) -> Result<bool, D::WriteError>

crates/electrum/src/v2.rs

@@ -53,7 +53,6 @@ impl<'a, K, A: Anchor> ElectrumUpdate<K, A> {
                 let _ = graph_update.insert_anchor(txid, anchor);
             }
         }
-        dbg!(graph_update.full_txs().count());
         LocalUpdate {
             keychain: self.keychain_update,
             graph: graph_update,
@@ -63,6 +62,12 @@ impl<'a, K, A: Anchor> ElectrumUpdate<K, A> {
 }
 
 impl<K> ElectrumUpdate<K, ConfirmationHeightAnchor> {
+    /// Finalizes the [`ElectrumUpdate`] with `new_txs` and anchors of type
+    /// [`ConfirmationTimeAnchor`].
+    ///
+    /// **Note:** The confirmation time might not be precisely correct if there has been a reorg.
+    /// Electrum's API intends that we use the merkle proof API, we should change `bdk_electrum` to
+    /// use it.
     pub fn finalize_as_confirmation_time<T>(
         self,
         client: &Client,
@@ -73,7 +78,6 @@ impl<K> ElectrumUpdate<K, ConfirmationHeightAnchor> {
         T: IntoIterator<Item = Transaction>,
     {
         let update = self.finalize(seen_at, new_txs);
-        let update_tip = update.chain.tip().expect("must have tip");
 
         let relevant_heights = {
             let mut visited_heights = HashSet::new();
@@ -97,16 +101,6 @@ impl<K> ElectrumUpdate<K, ConfirmationHeightAnchor> {
             )
             .collect::<HashMap<u32, u64>>();
 
-        if update_tip.hash != client.block_header(update_tip.height as _)?.block_hash() {
-            // [TODO] We should alter the logic so we won't have to return an error. This is to
-            // [TODO] ensure obtained block times are "anchored" to our tip. If we exclude this, it
-            // [TODO] should be "safe" as well. Tx confirmation times would just slightly vary.
-            return Err(Error::Message(format!(
-                "tip changed during update: update_tip={:?}",
-                update_tip
-            )));
-        }
-
         let graph_additions = {
             let old_additions = TxGraph::default().determine_additions(&update.graph);
             tx_graph::Additions {
@@ -336,6 +330,10 @@ fn determine_tx_anchor(
     raw_height: i32,
     txid: Txid,
 ) -> Option<ConfirmationHeightAnchor> {
+    // The electrum API has a weird quirk where an unconfirmed transaction is presented with a
+    // height of 0. To avoid invalid representation in our data structures, we manually set
+    // transactions residing in the genesis block to have height 0, then interpret a height of 0 as
+    // unconfirmed for all other transactions.
     if txid
         == Txid::from_hex("4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b")
             .expect("must deserialize genesis coinbase txid")

example-crates/example_cli/src/lib.rs

@@ -7,10 +7,8 @@ use std::{cmp::Reverse, collections::HashMap, path::PathBuf, sync::Mutex, time::
 
 use bdk_chain::{
     bitcoin::{
-        psbt::Prevouts,
+        psbt::Prevouts, secp256k1::Secp256k1, util::sighash::SighashCache, Address, LockTime,
-        secp256k1::{self, Secp256k1},
+        Network, Sequence, Transaction, TxIn, TxOut,
-        util::sighash::SighashCache,
-        Address, LockTime, Network, Sequence, Transaction, TxIn, TxOut,
     },
     indexed_tx_graph::{IndexedAdditions, IndexedTxGraph},
     keychain::{DerivationAdditions, KeychainTxOutIndex},
@@ -73,7 +71,7 @@ pub enum Commands<S: clap::Subcommand> {
     Send {
         value: u64,
         address: Address,
-        #[clap(short, default_value = "largest-first")]
+        #[clap(short, default_value = "bnb")]
         coin_select: CoinSelectionAlgo,
     },
 }
@@ -238,6 +236,13 @@ pub fn run_balance_cmd<A: Anchor, O: ChainOracle>(
     graph: &KeychainTxGraph<A>,
     chain: &O,
 ) -> Result<(), O::Error> {
+    fn print_balances<'a>(title_str: &'a str, items: impl IntoIterator<Item = (&'a str, u64)>) {
+        println!("{}:", title_str);
+        for (name, amount) in items.into_iter() {
+            println!("    {:<10} {:>12} sats", name, amount)
+        }
+    }
+
     let balance = graph.graph().try_balance(
         chain,
         chain.get_chain_tip()?.unwrap_or_default(),
@@ -248,15 +253,22 @@ pub fn run_balance_cmd<A: Anchor, O: ChainOracle>(
     let confirmed_total = balance.confirmed + balance.immature;
     let unconfirmed_total = balance.untrusted_pending + balance.trusted_pending;
 
-    println!("[confirmed]");
+    print_balances(
-    println!("  total     = {}sats", confirmed_total);
+        "confirmed",
-    println!("  spendable = {}sats", balance.confirmed);
+        [
-    println!("  immature  = {}sats", balance.immature);
+            ("total", confirmed_total),
-
+            ("spendable", balance.confirmed),
-    println!("[unconfirmed]");
+            ("immature", balance.immature),
-    println!("  total     = {}sats", unconfirmed_total,);
+        ],
-    println!("  trusted   = {}sats", balance.trusted_pending);
+    );
-    println!("  untrusted = {}sats", balance.untrusted_pending);
+    print_balances(
+        "unconfirmed",
+        [
+            ("total", unconfirmed_total),
+            ("trusted", balance.trusted_pending),
+            ("untrusted", balance.untrusted_pending),
+        ],
+    );
 
     Ok(())
 }
@@ -339,9 +351,11 @@ where
             // 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.
-            db.lock()
+            {
-                .unwrap()
+                let db = &mut *db.lock().unwrap();
-                .stage(C::from(KeychainAdditions::from(index_additions)));
+                db.stage(C::from(KeychainAdditions::from(index_additions)));
+                db.commit()?;
+            }
 
             // We don't want other callers/threads to use this address while we're using it
             // but we also don't want to scan the tx we just created because it's not
@@ -502,6 +516,8 @@ where
 
     let mut transaction = Transaction {
         version: 0x02,
+        // because the temporary planning module does not support timelocks, we can use the chain
+        // tip as the `lock_time` for anti-fee-sniping purposes
         lock_time: chain
             .get_chain_tip()?
             .and_then(|block_id| LockTime::from_height(block_id.height).ok())
@@ -666,29 +682,6 @@ where
     }
 }
 
-pub fn prepare_index<S: clap::Subcommand, CTX: secp256k1::Signing>(
-    args: &Args<S>,
-    secp: &Secp256k1<CTX>,
-) -> anyhow::Result<(KeychainTxOutIndex<Keychain>, KeyMap)> {
-    let mut index = KeychainTxOutIndex::<Keychain>::default();
-
-    let (descriptor, mut keymap) =
-        Descriptor::<DescriptorPublicKey>::parse_descriptor(secp, &args.descriptor)?;
-    index.add_keychain(Keychain::External, descriptor);
-
-    if let Some((internal_descriptor, internal_keymap)) = args
-        .change_descriptor
-        .as_ref()
-        .map(|desc_str| Descriptor::<DescriptorPublicKey>::parse_descriptor(secp, desc_str))
-        .transpose()?
-    {
-        keymap.extend(internal_keymap);
-        index.add_keychain(Keychain::Internal, internal_descriptor);
-    }
-
-    Ok((index, keymap))
-}
-
 #[allow(clippy::type_complexity)]
 pub fn init<'m, S: clap::Subcommand, C>(
     db_magic: &'m [u8],
@@ -708,7 +701,22 @@ where
     }
     let args = Args::<S>::parse();
     let secp = Secp256k1::default();
-    let (index, keymap) = prepare_index(&args, &secp)?;
+
+    let mut index = KeychainTxOutIndex::<Keychain>::default();
+
+    let (descriptor, mut keymap) =
+        Descriptor::<DescriptorPublicKey>::parse_descriptor(&secp, &args.descriptor)?;
+    index.add_keychain(Keychain::External, descriptor);
+
+    if let Some((internal_descriptor, internal_keymap)) = args
+        .change_descriptor
+        .as_ref()
+        .map(|desc_str| Descriptor::<DescriptorPublicKey>::parse_descriptor(&secp, desc_str))
+        .transpose()?
+    {
+        keymap.extend(internal_keymap);
+        index.add_keychain(Keychain::Internal, internal_descriptor);
+    }
 
     let mut db_backend = match Store::<'m, C>::new_from_path(db_magic, &args.db_path) {
         Ok(db_backend) => db_backend,

example-crates/example_electrum/src/main.rs

@@ -27,7 +27,7 @@ const ASSUME_FINAL_DEPTH: usize = 10;
 
 #[derive(Subcommand, Debug, Clone)]
 enum ElectrumCommands {
-    /// Scans the addresses in the wallet using the esplora API.
+    /// Scans the addresses in the wallet using the electrum API.
     Scan {
         /// When a gap this large has been found for a keychain, it will stop.
         #[clap(long, default_value = "5")]
@@ -35,7 +35,7 @@ enum ElectrumCommands {
         #[clap(flatten)]
         scan_options: ScanOptions,
     },
-    /// Scans particular addresses using the esplora API.
+    /// Scans particular addresses using the electrum API.
     Sync {
         /// Scan all the unused addresses.
         #[clap(long)]
@@ -119,7 +119,7 @@ fn main() -> anyhow::Result<()> {
             stop_gap,
             scan_options,
         } => {
-            let (keychain_spks, c) = {
+            let (keychain_spks, local_chain) = {
                 let graph = &*graph.lock().unwrap();
                 let chain = &*chain.lock().unwrap();
 
@@ -155,7 +155,7 @@ fn main() -> anyhow::Result<()> {
 
             client
                 .scan(
-                    &c,
+                    &local_chain,
                     keychain_spks,
                     core::iter::empty(),
                     core::iter::empty(),