[bdk_chain_redesign] Various tweaks and fixes

The `ConfirmationHeight` trait has been removed in favour of a second
method on the `Anchor` trait: `confirmation_height_upper_bound()`.

Methods `try_balance_at()` and `balance_at()` of `IndexedTxGraph` have
been removed as they do not provide additional functionality.

`IndexedTxGraph::insert_relevant_txs` now uses two loops, the first loop
indexes all transactions first. This is done as some indexes require
ancestor transactions to be indexed first and we cannot guarantee that
the input transactions are in topological order.

crates/chain/src/chain_data.rs

@@ -2,7 +2,7 @@ use bitcoin::{hashes::Hash, BlockHash, OutPoint, TxOut, Txid};
 
 use crate::{
     sparse_chain::{self, ChainPosition},
-    Anchor, ConfirmationHeight, COINBASE_MATURITY,
+    Anchor, COINBASE_MATURITY,
 };
 
 /// Represents an observation of some chain data.
@@ -241,20 +241,20 @@ impl<P: ChainPosition> FullTxOut<P> {
     }
 }
 
-impl<A: Anchor + ConfirmationHeight> FullTxOut<ObservedAs<A>> {
+impl<A: Anchor> FullTxOut<ObservedAs<A>> {
     /// Whether the `txout` is considered mature.
     ///
     /// This is the alternative version of [`is_mature`] which depends on `chain_position` being a
     /// [`ObservedAs<A>`] where `A` implements [`Anchor`].
     ///
     /// [`is_mature`]: Self::is_mature
-    pub fn is_observed_as_mature(&self, tip: u32) -> bool {
+    pub fn is_mature(&self, tip: u32) -> bool {
         if !self.is_on_coinbase {
-            return false;
+            return true;
         }
 
         let tx_height = match &self.chain_position {
-            ObservedAs::Confirmed(anchor) => anchor.confirmation_height(),
+            ObservedAs::Confirmed(anchor) => anchor.confirmation_height_upper_bound(),
             ObservedAs::Unconfirmed(_) => {
                 debug_assert!(false, "coinbase tx can never be unconfirmed");
                 return false;
@@ -271,19 +271,19 @@ impl<A: Anchor + ConfirmationHeight> FullTxOut<ObservedAs<A>> {
 
     /// Whether the utxo is/was/will be spendable with chain `tip`.
     ///
-    /// Currently this method does not take into account the locktime.
+    /// This method does not take into account the locktime.
     ///
     /// This is the alternative version of [`is_spendable_at`] which depends on `chain_position`
     /// being a [`ObservedAs<A>`] where `A` implements [`Anchor`].
     ///
     /// [`is_spendable_at`]: Self::is_spendable_at
-    pub fn is_observed_as_confirmed_and_spendable(&self, tip: u32) -> bool {
-        if !self.is_observed_as_mature(tip) {
+    pub fn is_confirmed_and_spendable(&self, tip: u32) -> bool {
+        if !self.is_mature(tip) {
             return false;
         }
 
         let confirmation_height = match &self.chain_position {
-            ObservedAs::Confirmed(anchor) => anchor.confirmation_height(),
+            ObservedAs::Confirmed(anchor) => anchor.confirmation_height_upper_bound(),
             ObservedAs::Unconfirmed(_) => return false,
         };
         if confirmation_height > tip {

crates/chain/src/chain_graph.rs

@@ -465,7 +465,7 @@ where
 #[must_use]
 pub struct ChangeSet<P> {
     pub chain: sparse_chain::ChangeSet<P>,
-    pub graph: tx_graph::Additions<()>,
+    pub graph: tx_graph::Additions,
 }
 
 impl<P> ChangeSet<P> {

crates/chain/src/indexed_tx_graph.rs

@@ -1,11 +1,12 @@
 use core::convert::Infallible;
 
+use alloc::vec::Vec;
 use bitcoin::{OutPoint, Script, Transaction, TxOut, Txid};
 
 use crate::{
     keychain::Balance,
     tx_graph::{Additions, CanonicalTx, TxGraph},
-    Anchor, Append, BlockId, ChainOracle, ConfirmationHeight, FullTxOut, ObservedAs,
+    Anchor, Append, BlockId, ChainOracle, FullTxOut, ObservedAs,
 };
 
 /// A struct that combines [`TxGraph`] and an [`Indexer`] implementation.
@@ -120,20 +121,34 @@ where
     ///
     /// `anchors` can be provided to anchor the transactions to blocks. `seen_at` is a unix
     /// timestamp of when the transactions are last seen.
-    pub fn insert_relevant_txs<'t, T: Iterator<Item = &'t Transaction>>(
+    pub fn insert_relevant_txs<'t>(
         &mut self,
-        txs: T,
+        txs: impl IntoIterator<Item = &'t Transaction>,
         anchors: impl IntoIterator<Item = A> + Clone,
         seen_at: Option<u64>,
     ) -> IndexedAdditions<A, I::Additions> {
-        txs.filter_map(|tx| match self.index.is_tx_relevant(tx) {
-            true => Some(self.insert_tx(tx, anchors.clone(), seen_at)),
-            false => None,
-        })
-        .fold(Default::default(), |mut acc, other| {
-            acc.append(other);
-            acc
-        })
+        // As mentioned by @LLFourn: This algorithm requires the transactions to be topologically
+        // sorted because most indexers cannot decide whether something is relevant unless you have
+        // first inserted its ancestors in the index. We can fix this if we instead do this:
+        // 1. insert all txs into the index. If they are irrelevant then that's fine it will just
+        //    not store anything about them.
+        // 2. decide whether to insert them into the graph depending on whether `is_tx_relevant`
+        //    returns true or not. (in a second loop).
+        let txs = txs
+            .into_iter()
+            .inspect(|tx| {
+                let _ = self.index.index_tx(tx);
+            })
+            .collect::<Vec<_>>();
+        txs.into_iter()
+            .filter_map(|tx| match self.index.is_tx_relevant(tx) {
+                true => Some(self.insert_tx(tx, anchors.clone(), seen_at)),
+                false => None,
+            })
+            .fold(Default::default(), |mut acc, other| {
+                acc.append(other);
+                acc
+            })
     }
 }
 
@@ -222,32 +237,31 @@ impl<A: Anchor, I: OwnedIndexer> IndexedTxGraph<A, I> {
         self.try_list_owned_unspents(chain, chain_tip)
             .map(|r| r.expect("oracle is infallible"))
     }
-}
 
-impl<A: Anchor + ConfirmationHeight, I: OwnedIndexer> IndexedTxGraph<A, I> {
     pub fn try_balance<C, F>(
         &self,
         chain: &C,
         chain_tip: BlockId,
-        tip: u32,
         mut should_trust: F,
     ) -> Result<Balance, C::Error>
     where
         C: ChainOracle,
         F: FnMut(&Script) -> bool,
     {
+        let tip_height = chain_tip.anchor_block().height;
+
         let mut immature = 0;
         let mut trusted_pending = 0;
         let mut untrusted_pending = 0;
         let mut confirmed = 0;
 
-        for res in self.try_list_owned_txouts(chain, chain_tip) {
+        for res in self.try_list_owned_unspents(chain, chain_tip) {
             let txout = res?;
 
             match &txout.chain_position {
                 ObservedAs::Confirmed(_) => {
                     if txout.is_on_coinbase {
-                        if txout.is_observed_as_mature(tip) {
+                        if txout.is_mature(tip_height) {
                             confirmed += txout.txout.value;
                         } else {
                             immature += txout.txout.value;
@@ -272,39 +286,12 @@ impl<A: Anchor + ConfirmationHeight, I: OwnedIndexer> IndexedTxGraph<A, I> {
         })
     }
 
-    pub fn balance<C, F>(&self, chain: &C, chain_tip: BlockId, tip: u32, should_trust: F) -> Balance
+    pub fn balance<C, F>(&self, chain: &C, chain_tip: BlockId, should_trust: F) -> Balance
     where
         C: ChainOracle<Error = Infallible>,
         F: FnMut(&Script) -> bool,
     {
-        self.try_balance(chain, chain_tip, tip, should_trust)
-            .expect("error is infallible")
-    }
-
-    pub fn try_balance_at<C>(
-        &self,
-        chain: &C,
-        chain_tip: BlockId,
-        height: u32,
-    ) -> Result<u64, C::Error>
-    where
-        C: ChainOracle,
-    {
-        let mut sum = 0;
-        for txo_res in self.try_list_owned_unspents(chain, chain_tip) {
-            let txo = txo_res?;
-            if txo.is_observed_as_confirmed_and_spendable(height) {
-                sum += txo.txout.value;
-            }
-        }
-        Ok(sum)
-    }
-
-    pub fn balance_at<C>(&self, chain: &C, chain_tip: BlockId, height: u32) -> u64
-    where
-        C: ChainOracle<Error = Infallible>,
-    {
-        self.try_balance_at(chain, chain_tip, height)
+        self.try_balance(chain, chain_tip, should_trust)
             .expect("error is infallible")
     }
 }

crates/chain/src/tx_data_traits.rs

@@ -1,7 +1,7 @@
 use crate::collections::BTreeMap;
 use crate::collections::BTreeSet;
 use crate::BlockId;
-use bitcoin::{Block, BlockHash, OutPoint, Transaction, TxOut};
+use bitcoin::{Block, OutPoint, Transaction, TxOut};
 
 /// Trait to do something with every txout contained in a structure.
 ///
@@ -41,34 +41,25 @@ impl ForEachTxOut for Transaction {
 /// assume that transaction A is also confirmed in the best chain. This does not necessarily mean
 /// that transaction A is confirmed in block B. It could also mean transaction A is confirmed in a
 /// parent block of B.
-pub trait Anchor:
-    core::fmt::Debug + Clone + Eq + PartialOrd + Ord + core::hash::Hash + Send + Sync + 'static
-{
+pub trait Anchor: core::fmt::Debug + Clone + Eq + PartialOrd + Ord + core::hash::Hash {
     /// Returns the [`BlockId`] that the associated blockchain data is "anchored" in.
     fn anchor_block(&self) -> BlockId;
-}
 
-impl<A: Anchor> Anchor for &'static A {
-    fn anchor_block(&self) -> BlockId {
-        <A as Anchor>::anchor_block(self)
+    /// Get the upper bound of the chain data's confirmation height.
+    ///
+    /// The default definition gives a pessimistic answer. This can be overridden by the `Anchor`
+    /// implementation for a more accurate value.
+    fn confirmation_height_upper_bound(&self) -> u32 {
+        self.anchor_block().height
     }
 }
 
-impl Anchor for (u32, BlockHash) {
+impl<A: Anchor> Anchor for &'static A {
     fn anchor_block(&self) -> BlockId {
-        (*self).into()
+        <A as Anchor>::anchor_block(self)
     }
 }
 
-/// A trait that returns a confirmation height.
-///
-/// This is typically used to provide an [`Anchor`] implementation the exact confirmation height of
-/// the data being anchored.
-pub trait ConfirmationHeight {
-    /// Returns the confirmation height.
-    fn confirmation_height(&self) -> u32;
-}
-
 /// Trait that makes an object appendable.
 pub trait Append {
     /// Append another object of the same type onto `self`.

crates/chain/src/tx_graph.rs

@@ -908,7 +908,7 @@ impl<A: Anchor> TxGraph<A> {
     )
 )]
 #[must_use]
-pub struct Additions<A> {
+pub struct Additions<A = ()> {
     pub tx: BTreeSet<Transaction>,
     pub txout: BTreeMap<OutPoint, TxOut>,
     pub anchors: BTreeSet<(A, Txid)>,