From e413d3e42455e26de2a079151aab166af71a3c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 3 May 2023 11:43:16 +0800 Subject: [PATCH 1/6] [chain_redesign] Change behavior of `try_get_chain_position` `TxGraph::try_get_chain_position` used to always exclude unconfirmed transactions with last_seen value of 0. However, what is the point of including a transaction in the graph if it cannot be part of the chain history? Additionally, maybe sometimes we don't wish to use the last_seen field at all. The new behavior will consider unconfirmed transactions with last_seen of 0. --- crates/chain/src/tx_graph.rs | 12 +++++------- crates/chain/tests/test_tx_graph.rs | 9 +++++---- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/crates/chain/src/tx_graph.rs b/crates/chain/src/tx_graph.rs index cee688be..d8b13030 100644 --- a/crates/chain/src/tx_graph.rs +++ b/crates/chain/src/tx_graph.rs @@ -624,11 +624,9 @@ impl TxGraph { chain_tip: BlockId, txid: Txid, ) -> Result>, C::Error> { - let (tx_node, anchors, &last_seen) = match self.txs.get(&txid) { - Some((tx, anchors, last_seen)) if !(anchors.is_empty() && *last_seen == 0) => { - (tx, anchors, last_seen) - } - _ => return Ok(None), + let (tx_node, anchors, last_seen) = match self.txs.get(&txid) { + Some(v) => v, + None => return Ok(None), }; for anchor in anchors { @@ -657,12 +655,12 @@ impl TxGraph { return Ok(None); } } - if conflicting_tx.last_seen_unconfirmed > last_seen { + if conflicting_tx.last_seen_unconfirmed > *last_seen { return Ok(None); } } - Ok(Some(ObservedAs::Unconfirmed(last_seen))) + Ok(Some(ObservedAs::Unconfirmed(*last_seen))) } /// Get the position of the transaction in `chain` with tip `chain_tip`. diff --git a/crates/chain/tests/test_tx_graph.rs b/crates/chain/tests/test_tx_graph.rs index 41b2ae02..7e8c3ad0 100644 --- a/crates/chain/tests/test_tx_graph.rs +++ b/crates/chain/tests/test_tx_graph.rs @@ -717,10 +717,11 @@ fn test_chain_spends() { ObservedAs::Confirmed(&local_chain.get_block(95).expect("block expected")) ); - // As long the unconfirmed tx isn't marked as seen, chain_spend will return None. - assert!(graph - .get_chain_spend(&local_chain, tip, OutPoint::new(tx_0.txid(), 1)) - .is_none()); + // Even if unconfirmed tx has a last_seen of 0, it can still be part of a chain spend. + assert_eq!( + graph.get_chain_spend(&local_chain, tip, OutPoint::new(tx_0.txid(), 1)), + Some((ObservedAs::Unconfirmed(0), tx_2.txid())), + ); // Mark the unconfirmed as seen and check correct ObservedAs status is returned. let _ = graph.insert_seen_at(tx_2.txid(), 1234567); From 085bf9413d182b8e2f2954ee5a2d8d74ded6758a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 3 May 2023 15:01:39 +0800 Subject: [PATCH 2/6] [chain_redesign] Add `LocalChain::insert_block` --- crates/chain/src/local_chain.rs | 46 +++++++++++++++++++ crates/chain/tests/test_local_chain.rs | 63 +++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/crates/chain/src/local_chain.rs b/crates/chain/src/local_chain.rs index 30dfe80b..b070c890 100644 --- a/crates/chain/src/local_chain.rs +++ b/crates/chain/src/local_chain.rs @@ -173,6 +173,31 @@ impl LocalChain { pub fn heights(&self) -> BTreeSet { self.blocks.keys().cloned().collect() } + + /// Insert a block of [`BlockId`] into the [`LocalChain`]. + /// + /// # Error + /// + /// If the insertion height already contains a block, and the block has a different blockhash, + /// this will result in an [`InsertBlockNotMatchingError`]. + pub fn insert_block( + &mut self, + block_id: BlockId, + ) -> Result { + let mut update = Self::from_blocks(self.tip()); + + if let Some(original_hash) = update.blocks.insert(block_id.height, block_id.hash) { + if original_hash != block_id.hash { + return Err(InsertBlockNotMatchingError { + height: block_id.height, + original_hash, + update_hash: block_id.hash, + }); + } + } + + Ok(self.apply_update(update).expect("should always connect")) + } } /// This is the return value of [`determine_changeset`] and represents changes to [`LocalChain`]. @@ -201,3 +226,24 @@ impl core::fmt::Display for UpdateNotConnectedError { #[cfg(feature = "std")] impl std::error::Error for UpdateNotConnectedError {} + +/// Represents a failure when trying to insert a checkpoint into [`LocalChain`]. +#[derive(Clone, Debug, PartialEq)] +pub struct InsertBlockNotMatchingError { + pub height: u32, + pub original_hash: BlockHash, + pub update_hash: BlockHash, +} + +impl core::fmt::Display for InsertBlockNotMatchingError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!( + f, + "failed to insert block at height {} as blockhashes conflict: original={}, update={}", + self.height, self.original_hash, self.update_hash + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InsertBlockNotMatchingError {} diff --git a/crates/chain/tests/test_local_chain.rs b/crates/chain/tests/test_local_chain.rs index 1aea9850..55d8af11 100644 --- a/crates/chain/tests/test_local_chain.rs +++ b/crates/chain/tests/test_local_chain.rs @@ -1,4 +1,7 @@ -use bdk_chain::local_chain::{LocalChain, UpdateNotConnectedError}; +use bdk_chain::local_chain::{ + ChangeSet, InsertBlockNotMatchingError, LocalChain, UpdateNotConnectedError, +}; +use bitcoin::BlockHash; #[macro_use] mod common; @@ -165,3 +168,61 @@ fn invalidation_but_no_connection() { Err(UpdateNotConnectedError(0)) ) } + +#[test] +fn insert_block() { + struct TestCase { + original: LocalChain, + insert: (u32, BlockHash), + expected_result: Result, + expected_final: LocalChain, + } + + let test_cases = [ + TestCase { + original: local_chain![], + insert: (5, h!("block5")), + expected_result: Ok([(5, Some(h!("block5")))].into()), + expected_final: local_chain![(5, h!("block5"))], + }, + TestCase { + original: local_chain![(3, h!("A"))], + insert: (4, h!("B")), + expected_result: Ok([(4, Some(h!("B")))].into()), + expected_final: local_chain![(3, h!("A")), (4, h!("B"))], + }, + TestCase { + original: local_chain![(4, h!("B"))], + insert: (3, h!("A")), + expected_result: Ok([(3, Some(h!("A")))].into()), + expected_final: local_chain![(3, h!("A")), (4, h!("B"))], + }, + TestCase { + original: local_chain![(2, h!("K"))], + insert: (2, h!("K")), + expected_result: Ok([].into()), + expected_final: local_chain![(2, h!("K"))], + }, + TestCase { + original: local_chain![(2, h!("K"))], + insert: (2, h!("J")), + expected_result: Err(InsertBlockNotMatchingError { + height: 2, + original_hash: h!("K"), + update_hash: h!("J"), + }), + expected_final: local_chain![(2, h!("K"))], + }, + ]; + + for (i, t) in test_cases.into_iter().enumerate() { + let mut chain = t.original; + assert_eq!( + chain.insert_block(t.insert.into()), + t.expected_result, + "[{}] unexpected result when inserting block", + i, + ); + assert_eq!(chain, t.expected_final, "[{}] unexpected final chain", i,); + } +} From 4ae727a1fb469beacee79fa8d3800152623961eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 3 May 2023 15:20:49 +0800 Subject: [PATCH 3/6] [chain_redesign] Relax generic constraints --- crates/chain/src/indexed_tx_graph.rs | 7 +++++-- crates/chain/src/keychain/txout_index.rs | 6 +++--- crates/chain/src/spk_txout_index.rs | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/chain/src/indexed_tx_graph.rs b/crates/chain/src/indexed_tx_graph.rs index 6d8c16ff..c550d86f 100644 --- a/crates/chain/src/indexed_tx_graph.rs +++ b/crates/chain/src/indexed_tx_graph.rs @@ -12,6 +12,7 @@ use crate::{ /// A struct that combines [`TxGraph`] and an [`Indexer`] implementation. /// /// This structure ensures that [`TxGraph`] and [`Indexer`] are updated atomically. +#[derive(Debug)] pub struct IndexedTxGraph { /// Transaction index. pub index: I, @@ -27,12 +28,14 @@ impl Default for IndexedTxGraph { } } -impl IndexedTxGraph { +impl IndexedTxGraph { /// Get a reference of the internal transaction graph. pub fn graph(&self) -> &TxGraph { &self.graph } +} +impl IndexedTxGraph { /// Applies the [`IndexedAdditions`] to the [`IndexedTxGraph`]. pub fn apply_additions(&mut self, additions: IndexedAdditions) { let IndexedAdditions { @@ -217,7 +220,7 @@ impl IndexedTxGraph { C: ChainOracle, F: FnMut(&Script) -> bool, { - let tip_height = chain_tip.anchor_block().height; + let tip_height = chain_tip.height; let mut immature = 0; let mut trusted_pending = 0; diff --git a/crates/chain/src/keychain/txout_index.rs b/crates/chain/src/keychain/txout_index.rs index f8ef46be..c7a8dd54 100644 --- a/crates/chain/src/keychain/txout_index.rs +++ b/crates/chain/src/keychain/txout_index.rs @@ -89,7 +89,7 @@ impl Deref for KeychainTxOutIndex { } } -impl Indexer for KeychainTxOutIndex { +impl Indexer for KeychainTxOutIndex { type Additions = DerivationAdditions; fn index_txout(&mut self, outpoint: OutPoint, txout: &TxOut) -> Self::Additions { @@ -109,9 +109,9 @@ impl Indexer for KeychainTxOutIndex { } } -impl OwnedIndexer for KeychainTxOutIndex { +impl OwnedIndexer for KeychainTxOutIndex { fn is_spk_owned(&self, spk: &Script) -> bool { - self.inner().is_spk_owned(spk) + self.index_of_spk(spk).is_some() } } diff --git a/crates/chain/src/spk_txout_index.rs b/crates/chain/src/spk_txout_index.rs index 9fdf3bc0..ae944149 100644 --- a/crates/chain/src/spk_txout_index.rs +++ b/crates/chain/src/spk_txout_index.rs @@ -53,7 +53,7 @@ impl Default for SpkTxOutIndex { } } -impl Indexer for SpkTxOutIndex { +impl Indexer for SpkTxOutIndex { type Additions = (); fn index_txout(&mut self, outpoint: OutPoint, txout: &TxOut) -> Self::Additions { From 2ccc116edabaf59d77324f291a438fc5a8204b3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 3 May 2023 16:03:23 +0800 Subject: [PATCH 4/6] [chain_redesign] `BlockId` should not implement `Anchor` If `BlockId` implements `Anchor`, the meaning is ambiguous. We cannot tell whether it means the tx is anchors at the block, or whether it also means the tx is confirmed at that block. Instead, `ConfirmationHeightAnchor` and `ConfirmationTimeAnchor` structs are introduced as non-ambiguous `Anchor` implementations. Additionally, `TxGraph::relevant_heights` is removed because it is also ambiguous. What heights are deemed relevant? A simpler and more flexible method `TxGraph::all_anchors` is introduced instead. --- crates/chain/src/chain_data.rs | 58 +++++++- crates/chain/src/tx_graph.rs | 20 +-- crates/chain/tests/test_indexed_tx_graph.rs | 147 +++++++++++--------- crates/chain/tests/test_tx_graph.rs | 106 ++++---------- 4 files changed, 159 insertions(+), 172 deletions(-) diff --git a/crates/chain/src/chain_data.rs b/crates/chain/src/chain_data.rs index a360b304..3252febc 100644 --- a/crates/chain/src/chain_data.rs +++ b/crates/chain/src/chain_data.rs @@ -160,12 +160,6 @@ impl Default for BlockId { } } -impl Anchor for BlockId { - fn anchor_block(&self) -> BlockId { - *self - } -} - impl From<(u32, BlockHash)> for BlockId { fn from((height, hash): (u32, BlockHash)) -> Self { Self { height, hash } @@ -187,6 +181,58 @@ impl From<(&u32, &BlockHash)> for BlockId { } } +/// An [`Anchor`] implementation that also records the exact confirmation height of the transaction. +#[derive(Debug, Default, Clone, PartialEq, Eq, Copy, PartialOrd, Ord, core::hash::Hash)] +#[cfg_attr( + feature = "serde", + derive(serde::Deserialize, serde::Serialize), + serde(crate = "serde_crate") +)] +pub struct ConfirmationHeightAnchor { + /// The anchor block. + pub anchor_block: BlockId, + + /// The exact confirmation height of the transaction. + /// + /// It is assumed that this value is never larger than the height of the anchor block. + pub confirmation_height: u32, +} + +impl Anchor for ConfirmationHeightAnchor { + fn anchor_block(&self) -> BlockId { + self.anchor_block + } + + fn confirmation_height_upper_bound(&self) -> u32 { + self.confirmation_height + } +} + +/// An [`Anchor`] implementation that also records the exact confirmation time and height of the +/// transaction. +#[derive(Debug, Default, Clone, PartialEq, Eq, Copy, PartialOrd, Ord, core::hash::Hash)] +#[cfg_attr( + feature = "serde", + derive(serde::Deserialize, serde::Serialize), + serde(crate = "serde_crate") +)] +pub struct ConfirmationTimeAnchor { + /// The anchor block. + pub anchor_block: BlockId, + + pub confirmation_height: u32, + pub confirmation_time: u64, +} + +impl Anchor for ConfirmationTimeAnchor { + fn anchor_block(&self) -> BlockId { + self.anchor_block + } + + fn confirmation_height_upper_bound(&self) -> u32 { + self.confirmation_height + } +} /// A `TxOut` with as much data as we can retrieve about it #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct FullTxOut

{ diff --git a/crates/chain/src/tx_graph.rs b/crates/chain/src/tx_graph.rs index d8b13030..e75255e4 100644 --- a/crates/chain/src/tx_graph.rs +++ b/crates/chain/src/tx_graph.rs @@ -349,6 +349,11 @@ impl TxGraph { .filter(move |(_, conflicting_txid)| *conflicting_txid != txid) } + /// Get all transaction anchors known by [`TxGraph`]. + pub fn all_anchors(&self) -> &BTreeSet<(A, Txid)> { + &self.anchors + } + /// Whether the graph has any transactions or outputs in it. pub fn is_empty(&self) -> bool { self.txs.is_empty() @@ -592,21 +597,6 @@ impl TxGraph { } impl TxGraph { - /// Get all heights that are relevant to the graph. - pub fn relevant_heights(&self) -> impl Iterator + '_ { - let mut last_height = Option::::None; - self.anchors - .iter() - .map(|(a, _)| a.anchor_block().height) - .filter(move |&height| { - let is_unique = Some(height) != last_height; - if is_unique { - last_height = Some(height); - } - is_unique - }) - } - /// Get the position of the transaction in `chain` with tip `chain_tip`. /// /// If the given transaction of `txid` does not exist in the chain of `chain_tip`, `None` is diff --git a/crates/chain/tests/test_indexed_tx_graph.rs b/crates/chain/tests/test_indexed_tx_graph.rs index 78f3c3a6..d88c1e39 100644 --- a/crates/chain/tests/test_indexed_tx_graph.rs +++ b/crates/chain/tests/test_indexed_tx_graph.rs @@ -8,7 +8,7 @@ use bdk_chain::{ keychain::{Balance, DerivationAdditions, KeychainTxOutIndex}, local_chain::LocalChain, tx_graph::Additions, - BlockId, ObservedAs, + ConfirmationHeightAnchor, ObservedAs, }; use bitcoin::{secp256k1::Secp256k1, BlockHash, OutPoint, Script, Transaction, TxIn, TxOut}; use miniscript::Descriptor; @@ -28,7 +28,7 @@ fn insert_relevant_txs() { let spk_0 = descriptor.at_derivation_index(0).script_pubkey(); let spk_1 = descriptor.at_derivation_index(9).script_pubkey(); - let mut graph = IndexedTxGraph::>::default(); + let mut graph = IndexedTxGraph::>::default(); graph.index.add_keychain((), descriptor); graph.index.set_lookahead(&(), 10); @@ -118,7 +118,8 @@ fn test_list_owned_txouts() { let (desc_1, _) = Descriptor::parse_descriptor(&Secp256k1::signing_only(), "tr(tprv8ZgxMBicQKsPd3krDUsBAmtnRsK3rb8u5yi1zhQgMhF1tR8MW7xfE4rnrbbsrbPR52e7rKapu6ztw1jXveJSCGHEriUGZV7mCe88duLp5pj/86'/1'/0'/0/*)").unwrap(); let (desc_2, _) = Descriptor::parse_descriptor(&Secp256k1::signing_only(), "tr(tprv8ZgxMBicQKsPd3krDUsBAmtnRsK3rb8u5yi1zhQgMhF1tR8MW7xfE4rnrbbsrbPR52e7rKapu6ztw1jXveJSCGHEriUGZV7mCe88duLp5pj/86'/1'/0'/1/*)").unwrap(); - let mut graph = IndexedTxGraph::>::default(); + let mut graph = + IndexedTxGraph::>::default(); graph.index.add_keychain("keychain_1".into(), desc_1); graph.index.add_keychain("keychain_2".into(), desc_2); @@ -206,86 +207,94 @@ fn test_list_owned_txouts() { // For unconfirmed txs we pass in `None`. let _ = graph.insert_relevant_txs( - [&tx1, &tx2, &tx3, &tx6] - .iter() - .enumerate() - .map(|(i, tx)| (*tx, [local_chain.get_block(i as u32).unwrap()])), + [&tx1, &tx2, &tx3, &tx6].iter().enumerate().map(|(i, tx)| { + ( + *tx, + local_chain + .get_block(i as u32) + .map(|anchor_block| ConfirmationHeightAnchor { + anchor_block, + confirmation_height: anchor_block.height, + }), + ) + }), None, ); let _ = graph.insert_relevant_txs([&tx4, &tx5].iter().map(|tx| (*tx, None)), Some(100)); // A helper lambda to extract and filter data from the graph. - let fetch = |ht: u32, graph: &IndexedTxGraph>| { - let txouts = graph - .list_owned_txouts(&local_chain, local_chain.get_block(ht).unwrap()) - .collect::>(); + let fetch = + |ht: u32, graph: &IndexedTxGraph>| { + let txouts = graph + .list_owned_txouts(&local_chain, local_chain.get_block(ht).unwrap()) + .collect::>(); - let utxos = graph - .list_owned_unspents(&local_chain, local_chain.get_block(ht).unwrap()) - .collect::>(); + let utxos = graph + .list_owned_unspents(&local_chain, local_chain.get_block(ht).unwrap()) + .collect::>(); - let balance = graph.balance( - &local_chain, - local_chain.get_block(ht).unwrap(), - |spk: &Script| trusted_spks.contains(spk), - ); + let balance = graph.balance( + &local_chain, + local_chain.get_block(ht).unwrap(), + |spk: &Script| trusted_spks.contains(spk), + ); - assert_eq!(txouts.len(), 5); - assert_eq!(utxos.len(), 4); + assert_eq!(txouts.len(), 5); + assert_eq!(utxos.len(), 4); - let confirmed_txouts_txid = txouts - .iter() - .filter_map(|full_txout| { - if matches!(full_txout.chain_position, ObservedAs::Confirmed(_)) { - Some(full_txout.outpoint.txid) - } else { - None - } - }) - .collect::>(); + let confirmed_txouts_txid = txouts + .iter() + .filter_map(|full_txout| { + if matches!(full_txout.chain_position, ObservedAs::Confirmed(_)) { + Some(full_txout.outpoint.txid) + } else { + None + } + }) + .collect::>(); - let unconfirmed_txouts_txid = txouts - .iter() - .filter_map(|full_txout| { - if matches!(full_txout.chain_position, ObservedAs::Unconfirmed(_)) { - Some(full_txout.outpoint.txid) - } else { - None - } - }) - .collect::>(); + let unconfirmed_txouts_txid = txouts + .iter() + .filter_map(|full_txout| { + if matches!(full_txout.chain_position, ObservedAs::Unconfirmed(_)) { + Some(full_txout.outpoint.txid) + } else { + None + } + }) + .collect::>(); - let confirmed_utxos_txid = utxos - .iter() - .filter_map(|full_txout| { - if matches!(full_txout.chain_position, ObservedAs::Confirmed(_)) { - Some(full_txout.outpoint.txid) - } else { - None - } - }) - .collect::>(); + let confirmed_utxos_txid = utxos + .iter() + .filter_map(|full_txout| { + if matches!(full_txout.chain_position, ObservedAs::Confirmed(_)) { + Some(full_txout.outpoint.txid) + } else { + None + } + }) + .collect::>(); - let unconfirmed_utxos_txid = utxos - .iter() - .filter_map(|full_txout| { - if matches!(full_txout.chain_position, ObservedAs::Unconfirmed(_)) { - Some(full_txout.outpoint.txid) - } else { - None - } - }) - .collect::>(); + let unconfirmed_utxos_txid = utxos + .iter() + .filter_map(|full_txout| { + if matches!(full_txout.chain_position, ObservedAs::Unconfirmed(_)) { + Some(full_txout.outpoint.txid) + } else { + None + } + }) + .collect::>(); - ( - confirmed_txouts_txid, - unconfirmed_txouts_txid, - confirmed_utxos_txid, - unconfirmed_utxos_txid, - balance, - ) - }; + ( + confirmed_txouts_txid, + unconfirmed_txouts_txid, + confirmed_utxos_txid, + unconfirmed_utxos_txid, + balance, + ) + }; // ----- TEST BLOCK ----- diff --git a/crates/chain/tests/test_tx_graph.rs b/crates/chain/tests/test_tx_graph.rs index 7e8c3ad0..427de71e 100644 --- a/crates/chain/tests/test_tx_graph.rs +++ b/crates/chain/tests/test_tx_graph.rs @@ -4,7 +4,7 @@ use bdk_chain::{ collections::*, local_chain::LocalChain, tx_graph::{Additions, TxGraph}, - Append, BlockId, ObservedAs, + Append, BlockId, ConfirmationHeightAnchor, ObservedAs, }; use bitcoin::{ hashes::Hash, BlockHash, OutPoint, PackedLockTime, Script, Transaction, TxIn, TxOut, Txid, @@ -684,7 +684,7 @@ fn test_chain_spends() { ..common::new_tx(0) }; - let mut graph = TxGraph::::default(); + let mut graph = TxGraph::::default(); let _ = graph.insert_tx(tx_0.clone()); let _ = graph.insert_tx(tx_1.clone()); @@ -694,27 +694,36 @@ fn test_chain_spends() { .iter() .zip([&tx_0, &tx_1].into_iter()) .for_each(|(ht, tx)| { - let block_id = local_chain.get_block(*ht).expect("block expected"); - let _ = graph.insert_anchor(tx.txid(), block_id); + // let block_id = local_chain.get_block(*ht).expect("block expected"); + let _ = graph.insert_anchor( + tx.txid(), + ConfirmationHeightAnchor { + anchor_block: tip, + confirmation_height: *ht, + }, + ); }); // Assert that confirmed spends are returned correctly. assert_eq!( - graph - .get_chain_spend(&local_chain, tip, OutPoint::new(tx_0.txid(), 0)) - .unwrap(), - ( - ObservedAs::Confirmed(&local_chain.get_block(98).expect("block expected")), - tx_1.txid() - ) + graph.get_chain_spend(&local_chain, tip, OutPoint::new(tx_0.txid(), 0)), + Some(( + ObservedAs::Confirmed(&ConfirmationHeightAnchor { + anchor_block: tip, + confirmation_height: 98 + }), + tx_1.txid(), + )), ); // Check if chain position is returned correctly. assert_eq!( - graph - .get_chain_position(&local_chain, tip, tx_0.txid()) - .expect("position expected"), - ObservedAs::Confirmed(&local_chain.get_block(95).expect("block expected")) + graph.get_chain_position(&local_chain, tip, tx_0.txid()), + // Some(ObservedAs::Confirmed(&local_chain.get_block(95).expect("block expected"))), + Some(ObservedAs::Confirmed(&ConfirmationHeightAnchor { + anchor_block: tip, + confirmation_height: 95 + })) ); // Even if unconfirmed tx has a last_seen of 0, it can still be part of a chain spend. @@ -784,73 +793,6 @@ fn test_chain_spends() { .is_none()); } -#[test] -fn test_relevant_heights() { - let mut graph = TxGraph::::default(); - - let tx1 = common::new_tx(1); - let tx2 = common::new_tx(2); - - let _ = graph.insert_tx(tx1.clone()); - assert_eq!( - graph.relevant_heights().collect::>(), - vec![], - "no anchors in graph" - ); - - let _ = graph.insert_anchor( - tx1.txid(), - BlockId { - height: 3, - hash: h!("3a"), - }, - ); - assert_eq!( - graph.relevant_heights().collect::>(), - vec![3], - "one anchor at height 3" - ); - - let _ = graph.insert_anchor( - tx1.txid(), - BlockId { - height: 3, - hash: h!("3b"), - }, - ); - assert_eq!( - graph.relevant_heights().collect::>(), - vec![3], - "introducing duplicate anchor at height 3, must not iterate over duplicate heights" - ); - - let _ = graph.insert_anchor( - tx1.txid(), - BlockId { - height: 4, - hash: h!("4a"), - }, - ); - assert_eq!( - graph.relevant_heights().collect::>(), - vec![3, 4], - "anchors in height 3 and now 4" - ); - - let _ = graph.insert_anchor( - tx2.txid(), - BlockId { - height: 5, - hash: h!("5a"), - }, - ); - assert_eq!( - graph.relevant_heights().collect::>(), - vec![3, 4, 5], - "anchor for non-existant tx is inserted at height 5, must still be in relevant heights", - ); -} - /// Ensure that `last_seen` values only increase during [`Append::append`]. #[test] fn test_additions_last_seen_append() { From a56d289eef0ea241bfe29b4c1069966353e4ca57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Fri, 5 May 2023 16:55:21 +0800 Subject: [PATCH 5/6] [chain_redesign] Various `LocalChain` improvements * Introduce `LocalChain::inner` method to get the inner map of block height to hash. * Replace `LocalChain::get_block` (which outputted `BlockId`, thus able to return invalid representation) with `get_blockhash` that just returns a `BlockHash`. * Remove `TODO` comments that should be github tickets. --- crates/chain/src/local_chain.rs | 20 ++++++-------- crates/chain/tests/test_indexed_tx_graph.rs | 30 ++++++++++++++++----- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/crates/chain/src/local_chain.rs b/crates/chain/src/local_chain.rs index b070c890..74dbb5a8 100644 --- a/crates/chain/src/local_chain.rs +++ b/crates/chain/src/local_chain.rs @@ -6,13 +6,6 @@ use bitcoin::BlockHash; use crate::{BlockId, ChainOracle}; /// This is a local implementation of [`ChainOracle`]. -/// -/// TODO: We need a cache/snapshot thing for chain oracle. -/// * Minimize calls to remotes. -/// * Can we cache it forever? Should we drop stuff? -/// * Assume anything deeper than (i.e. 10) blocks won't be reorged. -/// * Is this a cache on txs or block? or both? -/// TODO: Parents of children are confirmed if children are confirmed. #[derive(Debug, Default, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct LocalChain { blocks: BTreeMap, @@ -71,6 +64,11 @@ impl LocalChain { } } + /// Get a reference to the inner map of block height to hash. + pub fn inner(&self) -> &BTreeMap { + &self.blocks + } + pub fn tip(&self) -> Option { self.blocks .iter() @@ -78,11 +76,9 @@ impl LocalChain { .map(|(&height, &hash)| BlockId { height, hash }) } - /// Get a block at the given height. - pub fn get_block(&self, height: u32) -> Option { - self.blocks - .get(&height) - .map(|&hash| BlockId { height, hash }) + /// Get a [`BlockHash`] at the given height. + pub fn get_blockhash(&self, height: u32) -> Option { + self.blocks.get(&height).cloned() } /// This is like the sparsechain's logic, expect we must guarantee that all invalidated heights diff --git a/crates/chain/tests/test_indexed_tx_graph.rs b/crates/chain/tests/test_indexed_tx_graph.rs index d88c1e39..f5011c3e 100644 --- a/crates/chain/tests/test_indexed_tx_graph.rs +++ b/crates/chain/tests/test_indexed_tx_graph.rs @@ -8,7 +8,7 @@ use bdk_chain::{ keychain::{Balance, DerivationAdditions, KeychainTxOutIndex}, local_chain::LocalChain, tx_graph::Additions, - ConfirmationHeightAnchor, ObservedAs, + BlockId, ConfirmationHeightAnchor, ObservedAs, }; use bitcoin::{secp256k1::Secp256k1, BlockHash, OutPoint, Script, Transaction, TxIn, TxOut}; use miniscript::Descriptor; @@ -208,10 +208,12 @@ fn test_list_owned_txouts() { let _ = graph.insert_relevant_txs( [&tx1, &tx2, &tx3, &tx6].iter().enumerate().map(|(i, tx)| { + let height = i as u32; ( *tx, local_chain - .get_block(i as u32) + .get_blockhash(height) + .map(|hash| BlockId { height, hash }) .map(|anchor_block| ConfirmationHeightAnchor { anchor_block, confirmation_height: anchor_block.height, @@ -225,18 +227,34 @@ fn test_list_owned_txouts() { // A helper lambda to extract and filter data from the graph. let fetch = - |ht: u32, graph: &IndexedTxGraph>| { + |height: u32, + graph: &IndexedTxGraph>| { let txouts = graph - .list_owned_txouts(&local_chain, local_chain.get_block(ht).unwrap()) + .list_owned_txouts( + &local_chain, + local_chain + .get_blockhash(height) + .map(|hash| BlockId { height, hash }) + .unwrap(), + ) .collect::>(); let utxos = graph - .list_owned_unspents(&local_chain, local_chain.get_block(ht).unwrap()) + .list_owned_unspents( + &local_chain, + local_chain + .get_blockhash(height) + .map(|hash| BlockId { height, hash }) + .unwrap(), + ) .collect::>(); let balance = graph.balance( &local_chain, - local_chain.get_block(ht).unwrap(), + local_chain + .get_blockhash(height) + .map(|hash| BlockId { height, hash }) + .unwrap(), |spk: &Script| trusted_spks.contains(spk), ); From 065c64a675457cbcfe7c4e74aa5997e0ec5e0b0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Fri, 5 May 2023 19:49:30 +0800 Subject: [PATCH 6/6] [chain_redesign] Rename `LocalChain::inner()` to `blocks()` Also, we can get rid of `LocalChain::get_blockhash`, since we can already expose the internal map. Additionally, tests and docs are improved. --- crates/chain/src/chain_oracle.rs | 7 ++-- crates/chain/src/local_chain.rs | 9 ++--- crates/chain/tests/test_indexed_tx_graph.rs | 37 ++++++++------------- crates/chain/tests/test_tx_graph.rs | 1 - 4 files changed, 19 insertions(+), 35 deletions(-) diff --git a/crates/chain/src/chain_oracle.rs b/crates/chain/src/chain_oracle.rs index 2b4ad36f..58fbf6c1 100644 --- a/crates/chain/src/chain_oracle.rs +++ b/crates/chain/src/chain_oracle.rs @@ -10,12 +10,13 @@ pub trait ChainOracle { /// Error type. type Error: core::fmt::Debug; - /// Determines whether `block` of [`BlockId`] exists as an ancestor of `static_block`. + /// Determines whether `block` of [`BlockId`] exists as an ancestor of `chain_tip`. /// - /// If `None` is returned, it means the implementation cannot determine whether `block` exists. + /// If `None` is returned, it means the implementation cannot determine whether `block` exists + /// under `chain_tip`. fn is_block_in_chain( &self, block: BlockId, - static_block: BlockId, + chain_tip: BlockId, ) -> Result, Self::Error>; } diff --git a/crates/chain/src/local_chain.rs b/crates/chain/src/local_chain.rs index 74dbb5a8..a32a615c 100644 --- a/crates/chain/src/local_chain.rs +++ b/crates/chain/src/local_chain.rs @@ -64,8 +64,8 @@ impl LocalChain { } } - /// Get a reference to the inner map of block height to hash. - pub fn inner(&self) -> &BTreeMap { + /// Get a reference to a map of block height to hash. + pub fn blocks(&self) -> &BTreeMap { &self.blocks } @@ -76,11 +76,6 @@ impl LocalChain { .map(|(&height, &hash)| BlockId { height, hash }) } - /// Get a [`BlockHash`] at the given height. - pub fn get_blockhash(&self, height: u32) -> Option { - self.blocks.get(&height).cloned() - } - /// This is like the sparsechain's logic, expect we must guarantee that all invalidated heights /// are to be re-filled. pub fn determine_changeset(&self, update: &Self) -> Result { diff --git a/crates/chain/tests/test_indexed_tx_graph.rs b/crates/chain/tests/test_indexed_tx_graph.rs index f5011c3e..f32ffe4f 100644 --- a/crates/chain/tests/test_indexed_tx_graph.rs +++ b/crates/chain/tests/test_indexed_tx_graph.rs @@ -212,8 +212,9 @@ fn test_list_owned_txouts() { ( *tx, local_chain - .get_blockhash(height) - .map(|hash| BlockId { height, hash }) + .blocks() + .get(&height) + .map(|&hash| BlockId { height, hash }) .map(|anchor_block| ConfirmationHeightAnchor { anchor_block, confirmation_height: anchor_block.height, @@ -229,34 +230,22 @@ fn test_list_owned_txouts() { let fetch = |height: u32, graph: &IndexedTxGraph>| { + let chain_tip = local_chain + .blocks() + .get(&height) + .map(|&hash| BlockId { height, hash }) + .expect("block must exist"); let txouts = graph - .list_owned_txouts( - &local_chain, - local_chain - .get_blockhash(height) - .map(|hash| BlockId { height, hash }) - .unwrap(), - ) + .list_owned_txouts(&local_chain, chain_tip) .collect::>(); let utxos = graph - .list_owned_unspents( - &local_chain, - local_chain - .get_blockhash(height) - .map(|hash| BlockId { height, hash }) - .unwrap(), - ) + .list_owned_unspents(&local_chain, chain_tip) .collect::>(); - let balance = graph.balance( - &local_chain, - local_chain - .get_blockhash(height) - .map(|hash| BlockId { height, hash }) - .unwrap(), - |spk: &Script| trusted_spks.contains(spk), - ); + let balance = graph.balance(&local_chain, chain_tip, |spk: &Script| { + trusted_spks.contains(spk) + }); assert_eq!(txouts.len(), 5); assert_eq!(utxos.len(), 4); diff --git a/crates/chain/tests/test_tx_graph.rs b/crates/chain/tests/test_tx_graph.rs index 427de71e..2b845611 100644 --- a/crates/chain/tests/test_tx_graph.rs +++ b/crates/chain/tests/test_tx_graph.rs @@ -694,7 +694,6 @@ fn test_chain_spends() { .iter() .zip([&tx_0, &tx_1].into_iter()) .for_each(|(ht, tx)| { - // let block_id = local_chain.get_block(*ht).expect("block expected"); let _ = graph.insert_anchor( tx.txid(), ConfirmationHeightAnchor {