diff --git a/bdk_core/src/chain_graph.rs b/bdk_core/src/chain_graph.rs index 37a1702c..01c8115a 100644 --- a/bdk_core/src/chain_graph.rs +++ b/bdk_core/src/chain_graph.rs @@ -63,22 +63,33 @@ impl ChainGraph { &self, update: &Self, ) -> Result, sparse_chain::UpdateFailure> { - let mut chain_changeset = self.chain.determine_changeset(&update.chain)?; + let (mut chain_changeset, invalid_from) = self.chain.determine_changeset(&update.chain)?; + let invalid_from: TxHeight = invalid_from.into(); let conflicting_original_txids = update .chain .iter_txids() + // skip txids that already exist in the original chain (for efficiency) + .filter(|&(_, txid)| self.chain.tx_index(*txid).is_none()) // skip txids that do not have full txs, as we can't check for conflicts for them - .filter_map(|&(_, txid)| update.graph.tx(txid)) + .filter_map(|&(_, txid)| update.graph.tx(txid).or_else(|| self.graph.tx(txid))) // choose original txs that conflicts with the update .flat_map(|update_tx| { self.graph .conflicting_txids(update_tx) - .map(|(_, txid)| txid) - .filter(|&txid| self.chain.tx_index(txid).is_some()) + .filter_map(|(_, txid)| self.chain.tx_index(txid).map(|i| (txid, i))) }); - for txid in conflicting_original_txids { + for (txid, original_index) in conflicting_original_txids { + // if the evicted txid lies before "invalid_from", we screwed up + if original_index.height() < invalid_from { + return Err(sparse_chain::UpdateFailure::::InconsistentTx { + inconsistent_txid: txid, + original_index: original_index.clone(), + update_index: None, + }); + } + chain_changeset.txids.insert(txid, None); } diff --git a/bdk_core/src/sparse_chain.rs b/bdk_core/src/sparse_chain.rs index e469070a..89c7aed5 100644 --- a/bdk_core/src/sparse_chain.rs +++ b/bdk_core/src/sparse_chain.rs @@ -71,12 +71,12 @@ pub enum UpdateFailure { /// connect to the existing chain. This error case contains the checkpoint height to include so /// that the chains can connect. NotConnected(u32), - /// The update contains inconsistent tx states (e.g. it changed the transaction's hieght). + /// The update contains inconsistent tx states (e.g. it changed the transaction's height). /// This error is usually the inconsistency found. InconsistentTx { inconsistent_txid: Txid, original_index: I, - update_index: I, + update_index: Option, }, } @@ -164,7 +164,10 @@ impl SparseChain { /// Determine the changeset when `update` is applied to self. Invalidated checkpoints result in /// invalidated transactions becoming "unconfirmed". - pub fn determine_changeset(&self, update: &Self) -> Result, UpdateFailure> { + pub fn determine_changeset( + &self, + update: &Self, + ) -> Result<(ChangeSet, Option), UpdateFailure> { let agreement_point = update .checkpoints .iter() @@ -203,7 +206,7 @@ impl SparseChain { return Err(UpdateFailure::InconsistentTx { inconsistent_txid: txid, original_index: I::clone(original_index), - update_index: update_index.clone(), + update_index: Some(update_index.clone()), }); } } @@ -219,7 +222,7 @@ impl SparseChain { .collect(), // invalidated transactions become unconfirmed txids: self - .range_txids_by_height(TxHeight::Confirmed(invalid_from)..) + .range_txids_by_height(TxHeight::Confirmed(invalid_from)..TxHeight::Unconfirmed) .map(|(_, txid)| (*txid, Some(I::max_ord_of_height(TxHeight::Unconfirmed)))) .collect(), }) @@ -253,12 +256,12 @@ impl SparseChain { } } - Ok(changeset) + Ok((changeset, invalid_from)) } /// Tries to update `self` with another chain that connects to it. pub fn apply_update(&mut self, update: Self) -> Result<(), UpdateFailure> { - let changeset = self.determine_changeset(&update)?; + let (changeset, _) = self.determine_changeset(&update)?; self.apply_changeset(changeset); Ok(()) } diff --git a/bdk_core/tests/test_chain_graph.rs b/bdk_core/tests/test_chain_graph.rs index 8c5afc59..afa37441 100644 --- a/bdk_core/tests/test_chain_graph.rs +++ b/bdk_core/tests/test_chain_graph.rs @@ -1,5 +1,13 @@ -use bdk_core::{chain_graph::ChainGraph, TxHeight}; -use bitcoin::{OutPoint, PackedLockTime, Transaction, TxIn, TxOut}; +#[macro_use] +mod common; + +use bdk_core::{ + chain_graph::{ChainGraph, ChangeSet}, + sparse_chain, + tx_graph::Additions, + BlockId, TxHeight, +}; +use bitcoin::{OutPoint, PackedLockTime, Script, Sequence, Transaction, TxIn, TxOut, Witness}; #[test] fn test_spent_by() { @@ -47,3 +55,106 @@ fn test_spent_by() { assert_eq!(cg1.spent_by(op), Some((&TxHeight::Unconfirmed, tx2.txid()))); assert_eq!(cg2.spent_by(op), Some((&TxHeight::Unconfirmed, tx3.txid()))); } + +#[test] +fn update_evicts_conflicting_tx() { + let cp_a = BlockId { + height: 0, + hash: h!("A"), + }; + let cp_b = BlockId { + height: 1, + hash: h!("B"), + }; + + let tx_a = Transaction { + version: 0x01, + lock_time: PackedLockTime(0), + input: vec![], + output: vec![TxOut::default()], + }; + + let tx_b = Transaction { + version: 0x01, + lock_time: PackedLockTime(0), + input: vec![TxIn { + previous_output: OutPoint::new(tx_a.txid(), 0), + script_sig: Script::new(), + sequence: Sequence::default(), + witness: Witness::new(), + }], + output: vec![TxOut::default()], + }; + + let tx_b2 = Transaction { + version: 0x02, + lock_time: PackedLockTime(0), + input: vec![TxIn { + previous_output: OutPoint::new(tx_a.txid(), 0), + script_sig: Script::new(), + sequence: Sequence::default(), + witness: Witness::new(), + }], + output: vec![TxOut::default(), TxOut::default()], + }; + + let cg1 = { + let mut cg = ChainGraph::default(); + cg.insert_checkpoint(cp_a).expect("should insert cp"); + cg.insert_tx(tx_a.clone(), TxHeight::Confirmed(0)) + .expect("should insert tx"); + cg.insert_tx(tx_b.clone(), TxHeight::Unconfirmed) + .expect("should insert tx"); + cg + }; + let cg2 = { + let mut cg = ChainGraph::default(); + cg.insert_tx(tx_b2.clone(), TxHeight::Unconfirmed) + .expect("should insert tx"); + cg + }; + assert_eq!( + cg1.determine_changeset(&cg2), + Ok(ChangeSet:: { + chain: sparse_chain::ChangeSet { + checkpoints: Default::default(), + txids: [ + (tx_b.txid(), None), + (tx_b2.txid(), Some(TxHeight::Unconfirmed)) + ] + .into() + }, + graph: Additions { + tx: [tx_b2.clone()].into(), + txout: [].into() + }, + }), + "tx should be evicted from mempool" + ); + + let cg1 = { + let mut cg = ChainGraph::default(); + cg.insert_checkpoint(cp_a).expect("should insert cp"); + cg.insert_checkpoint(cp_b).expect("should insert cp"); + cg.insert_tx(tx_a.clone(), TxHeight::Confirmed(0)) + .expect("should insert tx"); + cg.insert_tx(tx_b.clone(), TxHeight::Confirmed(1)) + .expect("should insert tx"); + cg + }; + let cg2 = { + let mut cg = ChainGraph::default(); + cg.insert_tx(tx_b2.clone(), TxHeight::Unconfirmed) + .expect("should insert tx"); + cg + }; + assert_eq!( + cg1.determine_changeset(&cg2), + Err(sparse_chain::UpdateFailure::InconsistentTx { + inconsistent_txid: tx_b.txid(), + original_index: TxHeight::Confirmed(1), + update_index: None + }), + "fail if tx is evicted from valid block" + ); +} diff --git a/bdk_core/tests/test_sparse_chain.rs b/bdk_core/tests/test_sparse_chain.rs index 5b42e162..49c16e72 100644 --- a/bdk_core/tests/test_sparse_chain.rs +++ b/bdk_core/tests/test_sparse_chain.rs @@ -36,10 +36,13 @@ fn add_first_checkpoint() { let chain = SparseChain::default(); assert_eq!( chain.determine_changeset(&chain!([0, h!("A")])), - Ok(changeset! { - checkpoints: [(0, Some(h!("A")))], - txids: [] - }), + Ok(( + changeset! { + checkpoints: [(0, Some(h!("A")))], + txids: [] + }, + None + )), "add first tip" ); } @@ -49,10 +52,13 @@ fn add_second_tip() { let chain = chain!([0, h!("A")]); assert_eq!( chain.determine_changeset(&chain!([0, h!("A")], [1, h!("B")])), - Ok(changeset! { - checkpoints: [(1, Some(h!("B")))], - txids: [] - }), + Ok(( + changeset! { + checkpoints: [(1, Some(h!("B")))], + txids: [] + }, + None + )), "extend tip by one" ); } @@ -73,7 +79,7 @@ fn duplicate_chains_should_merge() { let chain2 = chain!([0, h!("A")]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(ChangeSet::default()) + Ok((ChangeSet::default(), None)) ); } @@ -83,7 +89,7 @@ fn duplicate_chains_with_txs_should_merge() { let chain2 = chain!(checkpoints: [[0,h!("A")]], txids: [(h!("tx0"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(ChangeSet::default()) + Ok((ChangeSet::default(), None)) ); } @@ -93,10 +99,13 @@ fn duplicate_chains_with_different_txs_should_merge() { let chain2 = chain!(checkpoints: [[0,h!("A")]], txids: [(h!("tx1"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [], - txids: [(h!("tx1"), Some(TxHeight::Confirmed(0)))] - }) + Ok(( + changeset! { + checkpoints: [], + txids: [(h!("tx1"), Some(TxHeight::Confirmed(0)))] + }, + None + )) ); } @@ -106,10 +115,13 @@ fn invalidate_first_and_only_checkpoint_without_tx_changes() { let chain2 = chain!(checkpoints: [[0,h!("A'")]], txids: [(h!("tx0"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(0, Some(h!("A'")))], - txids: [] - }) + Ok(( + changeset! { + checkpoints: [(0, Some(h!("A'")))], + txids: [] + }, + Some(0) + )) ); } @@ -119,10 +131,13 @@ fn invalidate_first_and_only_checkpoint_with_tx_move_forward() { let chain2 = chain!(checkpoints: [[0,h!("A'")],[1, h!("B")]], txids: [(h!("tx0"), TxHeight::Confirmed(1))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(0, Some(h!("A'"))), (1, Some(h!("B")))], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(1)))] - }) + Ok(( + changeset! { + checkpoints: [(0, Some(h!("A'"))), (1, Some(h!("B")))], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(1)))] + }, + Some(0) + )) ); } @@ -132,10 +147,13 @@ fn invalidate_first_and_only_checkpoint_with_tx_move_backward() { let chain2 = chain!(checkpoints: [[0,h!("A")],[1, h!("B'")]], txids: [(h!("tx0"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(0, Some(h!("A"))), (1, Some(h!("B'")))], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] - }) + Ok(( + changeset! { + checkpoints: [(0, Some(h!("A"))), (1, Some(h!("B'")))], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] + }, + Some(1) + )) ); } @@ -162,10 +180,13 @@ fn move_invalidated_tx_into_earlier_checkpoint() { let chain2 = chain!(checkpoints: [[0, h!("A")], [1, h!("B'")]], txids: [(h!("tx0"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(1, Some(h!("B'")))], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] - }) + Ok(( + changeset! { + checkpoints: [(1, Some(h!("B'")))], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] + }, + Some(1) + )) ); } @@ -175,10 +196,13 @@ fn invalidate_first_and_only_checkpoint_with_tx_move_to_mempool() { let chain2 = chain!(checkpoints: [[0,h!("A'")]], txids: [(h!("tx0"), TxHeight::Unconfirmed)]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(0, Some(h!("A'")))], - txids: [(h!("tx0"), Some(TxHeight::Unconfirmed))] - }) + Ok(( + changeset! { + checkpoints: [(0, Some(h!("A'")))], + txids: [(h!("tx0"), Some(TxHeight::Unconfirmed))] + }, + Some(0) + )) ); } @@ -188,10 +212,13 @@ fn confirm_tx_without_extending_chain() { let chain2 = chain!(checkpoints: [[0,h!("A")]], txids: [(h!("tx0"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] - }) + Ok(( + changeset! { + checkpoints: [], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] + }, + None + )) ); } @@ -201,10 +228,13 @@ fn confirm_tx_backwards_while_extending_chain() { let chain2 = chain!(checkpoints: [[0,h!("A")],[1,h!("B")]], txids: [(h!("tx0"), TxHeight::Confirmed(0))]); assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(1, Some(h!("B")))], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] - }) + Ok(( + changeset! { + checkpoints: [(1, Some(h!("B")))], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(0)))] + }, + None + )) ); } @@ -217,10 +247,13 @@ fn confirm_tx_in_new_block() { }; assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(1, Some(h!("B")))], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(1)))] - }) + Ok(( + changeset! { + checkpoints: [(1, Some(h!("B")))], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(1)))] + }, + None + )) ); } @@ -231,10 +264,13 @@ fn merging_mempool_of_empty_chains_doesnt_fail() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [], - txids: [(h!("tx1"), Some(TxHeight::Unconfirmed))] - }) + Ok(( + changeset! { + checkpoints: [], + txids: [(h!("tx1"), Some(TxHeight::Unconfirmed))] + }, + None + )) ); } @@ -254,10 +290,13 @@ fn empty_chain_can_add_unconfirmed_transactions() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [], - txids: [ (h!("tx0"), Some(TxHeight::Unconfirmed)) ] - }) + Ok(( + changeset! { + checkpoints: [], + txids: [ (h!("tx0"), Some(TxHeight::Unconfirmed)) ] + }, + None + )) ); } @@ -268,10 +307,13 @@ fn can_update_with_shorter_chain() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [], - txids: [(h!("tx0"), Some(TxHeight::Confirmed(1)))] - }) + Ok(( + changeset! { + checkpoints: [], + txids: [(h!("tx0"), Some(TxHeight::Confirmed(1)))] + }, + None + )) ) } @@ -282,10 +324,13 @@ fn can_introduce_older_checkpoints() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(1, Some(h!("B")))], - txids: [] - }) + Ok(( + changeset! { + checkpoints: [(1, Some(h!("B")))], + txids: [] + }, + None + )) ); } @@ -296,10 +341,15 @@ fn fix_blockhash_before_agreement_point() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(0, Some(h!("fix")))], - txids: [] - }) + Ok(( + changeset! { + checkpoints: [(0, Some(h!("fix")))], + txids: [] + }, + // logically, this should be `Some(1)`, however the logic assumes no hash will change + // before agreement point + None + )) ) } @@ -322,7 +372,7 @@ fn cannot_change_ext_index_of_confirmed_tx() { Err(UpdateFailure::InconsistentTx { inconsistent_txid: h!("tx0"), original_index: TestIndex(TxHeight::Confirmed(1), 10), - update_index: TestIndex(TxHeight::Confirmed(1), 20), + update_index: Some(TestIndex(TxHeight::Confirmed(1), 20)), }), ) } @@ -342,10 +392,13 @@ fn can_change_index_of_unconfirmed_tx() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(ChangeSet { - checkpoints: [].into(), - txids: [(h!("tx1"), Some(TestIndex(TxHeight::Unconfirmed, 20)),)].into() - }), + Ok(( + ChangeSet { + checkpoints: [].into(), + txids: [(h!("tx1"), Some(TestIndex(TxHeight::Unconfirmed, 20)),)].into() + }, + None + )), ) } @@ -363,9 +416,12 @@ fn two_points_of_agreement() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [(0, Some(h!("A"))), (3, Some(h!("D")))] - }), + Ok(( + changeset! { + checkpoints: [(0, Some(h!("A"))), (3, Some(h!("D")))] + }, + None + )), ); } @@ -414,20 +470,23 @@ fn transitive_invalidation_applies_to_checkpoints_higher_than_invalidation() { assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [ - (2, Some(h!("B'"))), - (3, Some(h!("C'"))), - (4, Some(h!("D"))), - (5, None) - ], - txids: [ - (h!("b1"), Some(TxHeight::Confirmed(4))), - (h!("b2"), Some(TxHeight::Confirmed(3))), - (h!("d"), Some(TxHeight::Unconfirmed)), - (h!("e"), Some(TxHeight::Unconfirmed)) - ] - }) + Ok(( + changeset! { + checkpoints: [ + (2, Some(h!("B'"))), + (3, Some(h!("C'"))), + (4, Some(h!("D"))), + (5, None) + ], + txids: [ + (h!("b1"), Some(TxHeight::Confirmed(4))), + (h!("b2"), Some(TxHeight::Confirmed(3))), + (h!("d"), Some(TxHeight::Unconfirmed)), + (h!("e"), Some(TxHeight::Unconfirmed)) + ] + }, + Some(2) + )) ); } @@ -446,14 +505,17 @@ fn transitive_invalidation_applies_to_checkpoints_higher_than_invalidation_no_po assert_eq!( chain1.determine_changeset(&chain2), - Ok(changeset! { - checkpoints: [ - (1, Some(h!("B'"))), - (2, Some(h!("C'"))), - (3, Some(h!("D"))), - (4, None) - ] - }) + Ok(( + changeset! { + checkpoints: [ + (1, Some(h!("B'"))), + (2, Some(h!("C'"))), + (3, Some(h!("D"))), + (4, None) + ] + }, + Some(1) + )) ) } @@ -503,9 +565,12 @@ fn checkpoint_limit_is_respected() { assert_eq!(chain1.checkpoints().len(), 4); let changeset = chain1.determine_changeset(&chain!([6, h!("F")], [7, h!("G")])); - assert_eq!(changeset, Ok(changeset!(checkpoints: [(7, Some(h!("G")))]))); + assert_eq!( + changeset, + Ok((changeset!(checkpoints: [(7, Some(h!("G")))]), None)) + ); - chain1.apply_changeset(changeset.unwrap()); + chain1.apply_changeset(changeset.unwrap().0); assert_eq!(chain1.checkpoints().len(), 4); } @@ -719,3 +784,35 @@ fn range_txids() { ); } } + +#[test] +fn invalidated_txs_move_to_unconfirmed() { + let chain1 = chain! { + checkpoints: [[0, h!("A")], [1, h!("B")], [2, h!("C")]], + txids: [ + (h!("a"), TxHeight::Confirmed(0)), + (h!("b"), TxHeight::Confirmed(1)), + (h!("c"), TxHeight::Confirmed(2)), + (h!("d"), TxHeight::Unconfirmed) + ] + }; + + let chain2 = chain!([0, h!("A")], [1, h!("B'")]); + + assert_eq!( + chain1.determine_changeset(&chain2), + Ok(( + changeset! { + checkpoints: [ + (1, Some(h!("B'"))), + (2, None) + ], + txids: [ + (h!("b"), Some(TxHeight::Unconfirmed)), + (h!("c"), Some(TxHeight::Unconfirmed)) + ] + }, + Some(1) + )) + ); +}