From f3e7b67bf1195eadf48a8f26f960b3f39d6134f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Fri, 21 Apr 2023 13:29:44 +0800 Subject: [PATCH] [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 | 18 +++---- crates/chain/src/chain_graph.rs | 2 +- crates/chain/src/indexed_tx_graph.rs | 77 ++++++++++++---------------- crates/chain/src/tx_data_traits.rs | 29 ++++------- crates/chain/src/tx_graph.rs | 2 +- 5 files changed, 53 insertions(+), 75 deletions(-) diff --git a/crates/chain/src/chain_data.rs b/crates/chain/src/chain_data.rs index 33381ab7..e585d82d 100644 --- a/crates/chain/src/chain_data.rs +++ b/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 FullTxOut

{ } } -impl FullTxOut> { +impl FullTxOut> { /// Whether the `txout` is considered mature. /// /// This is the alternative version of [`is_mature`] which depends on `chain_position` being a /// [`ObservedAs`] 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 FullTxOut> { /// 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`] 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 { diff --git a/crates/chain/src/chain_graph.rs b/crates/chain/src/chain_graph.rs index 0e3e3439..acf104e7 100644 --- a/crates/chain/src/chain_graph.rs +++ b/crates/chain/src/chain_graph.rs @@ -465,7 +465,7 @@ where #[must_use] pub struct ChangeSet

{ pub chain: sparse_chain::ChangeSet

, - pub graph: tx_graph::Additions<()>, + pub graph: tx_graph::Additions, } impl

ChangeSet

{ diff --git a/crates/chain/src/indexed_tx_graph.rs b/crates/chain/src/indexed_tx_graph.rs index 5ce53111..f44b7847 100644 --- a/crates/chain/src/indexed_tx_graph.rs +++ b/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>( + pub fn insert_relevant_txs<'t>( &mut self, - txs: T, + txs: impl IntoIterator, anchors: impl IntoIterator + Clone, seen_at: Option, ) -> IndexedAdditions { - 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::>(); + 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 IndexedTxGraph { self.try_list_owned_unspents(chain, chain_tip) .map(|r| r.expect("oracle is infallible")) } -} -impl IndexedTxGraph { pub fn try_balance( &self, chain: &C, chain_tip: BlockId, - tip: u32, mut should_trust: F, ) -> Result 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 IndexedTxGraph { }) } - pub fn balance(&self, chain: &C, chain_tip: BlockId, tip: u32, should_trust: F) -> Balance + pub fn balance(&self, chain: &C, chain_tip: BlockId, should_trust: F) -> Balance where C: ChainOracle, F: FnMut(&Script) -> bool, { - self.try_balance(chain, chain_tip, tip, should_trust) - .expect("error is infallible") - } - - pub fn try_balance_at( - &self, - chain: &C, - chain_tip: BlockId, - height: u32, - ) -> Result - 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(&self, chain: &C, chain_tip: BlockId, height: u32) -> u64 - where - C: ChainOracle, - { - self.try_balance_at(chain, chain_tip, height) + self.try_balance(chain, chain_tip, should_trust) .expect("error is infallible") } } diff --git a/crates/chain/src/tx_data_traits.rs b/crates/chain/src/tx_data_traits.rs index bd7e66ac..8ec695ad 100644 --- a/crates/chain/src/tx_data_traits.rs +++ b/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,11 +41,17 @@ 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; + + /// 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 &'static A { @@ -54,21 +60,6 @@ impl Anchor for &'static A { } } -impl Anchor for (u32, BlockHash) { - fn anchor_block(&self) -> BlockId { - (*self).into() - } -} - -/// 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`. diff --git a/crates/chain/src/tx_graph.rs b/crates/chain/src/tx_graph.rs index c7c496ef..04c9fa27 100644 --- a/crates/chain/src/tx_graph.rs +++ b/crates/chain/src/tx_graph.rs @@ -908,7 +908,7 @@ impl TxGraph { ) )] #[must_use] -pub struct Additions { +pub struct Additions { pub tx: BTreeSet, pub txout: BTreeMap, pub anchors: BTreeSet<(A, Txid)>,