From a3d4eef77d427b290e1d0041d3c760b8fe29f6a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 8 Oct 2024 09:15:55 +0000 Subject: [PATCH 1/4] feat(chain,wallet)!: rm `ConfirmationTime` We rm `ConfirmationTime` because it is essentially the same thing as `ChainPosition` without the block hash. We also impl `serde::Deserialize` and `serde::Serialize` for `ChainPosition`. --- crates/chain/src/chain_data.rs | 53 ++------ crates/wallet/src/types.rs | 6 +- crates/wallet/src/wallet/coin_selection.rs | 105 ++++++++------- crates/wallet/src/wallet/mod.rs | 39 +++--- crates/wallet/src/wallet/tx_builder.rs | 14 +- crates/wallet/tests/common.rs | 43 +++---- crates/wallet/tests/wallet.rs | 142 +++++++++------------ 7 files changed, 179 insertions(+), 223 deletions(-) diff --git a/crates/chain/src/chain_data.rs b/crates/chain/src/chain_data.rs index ce6076c51..e0202e1af 100644 --- a/crates/chain/src/chain_data.rs +++ b/crates/chain/src/chain_data.rs @@ -1,4 +1,3 @@ -use crate::ConfirmationBlockTime; use bitcoin::{OutPoint, TxOut, Txid}; use crate::{Anchor, COINBASE_MATURITY}; @@ -7,6 +6,14 @@ use crate::{Anchor, COINBASE_MATURITY}; /// /// The generic `A` should be a [`Anchor`] implementation. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, core::hash::Hash)] +#[cfg_attr( + feature = "serde", + derive(serde::Deserialize, serde::Serialize), + serde(bound( + deserialize = "A: Ord + serde::Deserialize<'de>", + serialize = "A: Ord + serde::Serialize", + )) +)] pub enum ChainPosition { /// The chain data is seen as confirmed, and in anchored by `A`. Confirmed(A), @@ -41,48 +48,6 @@ impl ChainPosition { } } -/// Block height and timestamp at which a transaction is confirmed. -#[derive(Debug, Clone, PartialEq, Eq, Copy, PartialOrd, Ord, core::hash::Hash)] -#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] -pub enum ConfirmationTime { - /// The transaction is confirmed - Confirmed { - /// Confirmation height. - height: u32, - /// Confirmation time in unix seconds. - time: u64, - }, - /// The transaction is unconfirmed - Unconfirmed { - /// The last-seen timestamp in unix seconds. - last_seen: u64, - }, -} - -impl ConfirmationTime { - /// Construct an unconfirmed variant using the given `last_seen` time in unix seconds. - pub fn unconfirmed(last_seen: u64) -> Self { - Self::Unconfirmed { last_seen } - } - - /// Returns whether [`ConfirmationTime`] is the confirmed variant. - pub fn is_confirmed(&self) -> bool { - matches!(self, Self::Confirmed { .. }) - } -} - -impl From> for ConfirmationTime { - fn from(observed_as: ChainPosition) -> Self { - match observed_as { - ChainPosition::Confirmed(a) => Self::Confirmed { - height: a.block_id.height, - time: a.confirmation_time, - }, - ChainPosition::Unconfirmed(last_seen) => Self::Unconfirmed { last_seen }, - } - } -} - /// A `TxOut` with as much data as we can retrieve about it #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct FullTxOut { @@ -159,6 +124,8 @@ impl FullTxOut { #[cfg(test)] mod test { + use bdk_core::ConfirmationBlockTime; + use crate::BlockId; use super::*; diff --git a/crates/wallet/src/types.rs b/crates/wallet/src/types.rs index 6ed17b575..c4626fbf3 100644 --- a/crates/wallet/src/types.rs +++ b/crates/wallet/src/types.rs @@ -10,9 +10,9 @@ // licenses. use alloc::boxed::Box; +use chain::{ChainPosition, ConfirmationBlockTime}; use core::convert::AsRef; -use bdk_chain::ConfirmationTime; use bitcoin::transaction::{OutPoint, Sequence, TxOut}; use bitcoin::{psbt, Weight}; @@ -61,8 +61,8 @@ pub struct LocalOutput { pub is_spent: bool, /// The derivation index for the script pubkey in the wallet pub derivation_index: u32, - /// The confirmation time for transaction containing this utxo - pub confirmation_time: ConfirmationTime, + /// The position of the output in the blockchain. + pub chain_position: ChainPosition, } /// A [`Utxo`] with its `satisfaction_weight`. diff --git a/crates/wallet/src/wallet/coin_selection.rs b/crates/wallet/src/wallet/coin_selection.rs index 381ff65b2..0f0e4a88e 100644 --- a/crates/wallet/src/wallet/coin_selection.rs +++ b/crates/wallet/src/wallet/coin_selection.rs @@ -278,7 +278,7 @@ impl CoinSelectionAlgorithm for OldestFirstCoinSelection { // For utxo that doesn't exist in DB, they will have lowest priority to be selected let utxos = { optional_utxos.sort_unstable_by_key(|wu| match &wu.utxo { - Utxo::Local(local) => Some(local.confirmation_time), + Utxo::Local(local) => Some(local.chain_position), Utxo::Foreign { .. } => None, }); @@ -733,11 +733,12 @@ where #[cfg(test)] mod test { use assert_matches::assert_matches; + use bitcoin::hashes::Hash; + use chain::{BlockId, ChainPosition, ConfirmationBlockTime}; use core::str::FromStr; use rand::rngs::StdRng; - use bdk_chain::ConfirmationTime; - use bitcoin::{Amount, ScriptBuf, TxIn, TxOut}; + use bitcoin::{Amount, BlockHash, ScriptBuf, TxIn, TxOut}; use super::*; use crate::types::*; @@ -752,7 +753,34 @@ mod test { const FEE_AMOUNT: u64 = 50; - fn utxo(value: u64, index: u32, confirmation_time: ConfirmationTime) -> WeightedUtxo { + fn unconfirmed_utxo(value: u64, index: u32, last_seen: u64) -> WeightedUtxo { + utxo(value, index, ChainPosition::Unconfirmed(last_seen)) + } + + fn confirmed_utxo( + value: u64, + index: u32, + confirmation_height: u32, + confirmation_time: u64, + ) -> WeightedUtxo { + utxo( + value, + index, + ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: chain::BlockId { + height: confirmation_height, + hash: bitcoin::BlockHash::all_zeros(), + }, + confirmation_time, + }), + ) + } + + fn utxo( + value: u64, + index: u32, + chain_position: ChainPosition, + ) -> WeightedUtxo { assert!(index < 10); let outpoint = OutPoint::from_str(&format!( "000000000000000000000000000000000000000000000000000000000000000{}:0", @@ -770,49 +798,24 @@ mod test { keychain: KeychainKind::External, is_spent: false, derivation_index: 42, - confirmation_time, + chain_position, }), } } fn get_test_utxos() -> Vec { vec![ - utxo(100_000, 0, ConfirmationTime::Unconfirmed { last_seen: 0 }), - utxo( - FEE_AMOUNT - 40, - 1, - ConfirmationTime::Unconfirmed { last_seen: 0 }, - ), - utxo(200_000, 2, ConfirmationTime::Unconfirmed { last_seen: 0 }), + unconfirmed_utxo(100_000, 0, 0), + unconfirmed_utxo(FEE_AMOUNT - 40, 1, 0), + unconfirmed_utxo(200_000, 2, 0), ] } fn get_oldest_first_test_utxos() -> Vec { // ensure utxos are from different tx - let utxo1 = utxo( - 120_000, - 1, - ConfirmationTime::Confirmed { - height: 1, - time: 1231006505, - }, - ); - let utxo2 = utxo( - 80_000, - 2, - ConfirmationTime::Confirmed { - height: 2, - time: 1231006505, - }, - ); - let utxo3 = utxo( - 300_000, - 3, - ConfirmationTime::Confirmed { - height: 3, - time: 1231006505, - }, - ); + let utxo1 = confirmed_utxo(120_000, 1, 1, 1231006505); + let utxo2 = confirmed_utxo(80_000, 2, 2, 1231006505); + let utxo3 = confirmed_utxo(300_000, 3, 3, 1231006505); vec![utxo1, utxo2, utxo3] } @@ -834,13 +837,16 @@ mod test { keychain: KeychainKind::External, is_spent: false, derivation_index: rng.next_u32(), - confirmation_time: if rng.gen_bool(0.5) { - ConfirmationTime::Confirmed { - height: rng.next_u32(), - time: rng.next_u64(), - } + chain_position: if rng.gen_bool(0.5) { + ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: chain::BlockId { + height: rng.next_u32(), + hash: BlockHash::all_zeros(), + }, + confirmation_time: rng.next_u64(), + }) } else { - ConfirmationTime::Unconfirmed { last_seen: 0 } + ChainPosition::Unconfirmed(0) }, }), }); @@ -865,7 +871,7 @@ mod test { keychain: KeychainKind::External, is_spent: false, derivation_index: 42, - confirmation_time: ConfirmationTime::Unconfirmed { last_seen: 0 }, + chain_position: ChainPosition::Unconfirmed(0), }), }) .collect() @@ -1222,7 +1228,7 @@ mod test { optional.push(utxo( 500_000, 3, - ConfirmationTime::Unconfirmed { last_seen: 0 }, + ChainPosition::::Unconfirmed(0), )); // Defensive assertions, for sanity and in case someone changes the test utxos vector. @@ -1584,10 +1590,13 @@ mod test { keychain: KeychainKind::External, is_spent: false, derivation_index: 0, - confirmation_time: ConfirmationTime::Confirmed { - height: 12345, - time: 12345, - }, + chain_position: ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: BlockId { + height: 12345, + hash: BlockHash::all_zeros(), + }, + confirmation_time: 12345, + }), }), } } diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 19604f441..562e684c1 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -34,8 +34,8 @@ use bdk_chain::{ SyncResult, }, tx_graph::{CanonicalTx, TxGraph, TxNode, TxUpdate}, - BlockId, ChainPosition, ConfirmationBlockTime, ConfirmationTime, DescriptorExt, FullTxOut, - Indexed, IndexedTxGraph, Merge, + BlockId, ChainPosition, ConfirmationBlockTime, DescriptorExt, FullTxOut, Indexed, + IndexedTxGraph, Merge, }; use bitcoin::sighash::{EcdsaSighashType, TapSighashType}; use bitcoin::{ @@ -1660,11 +1660,10 @@ impl Wallet { .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?; let txout = &prev_tx.output[txin.previous_output.vout as usize]; - let confirmation_time: ConfirmationTime = graph + let chain_position = graph .get_chain_position(&self.chain, chain_tip, txin.previous_output.txid) .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))? - .cloned() - .into(); + .cloned(); let weighted_utxo = match txout_index.index_of_spk(txout.script_pubkey.clone()) { Some(&(keychain, derivation_index)) => { @@ -1679,7 +1678,7 @@ impl Wallet { keychain, is_spent: true, derivation_index, - confirmation_time, + chain_position, }), satisfaction_weight, } @@ -2051,33 +2050,33 @@ impl Wallet { Some(tx) => tx, None => return false, }; - let confirmation_time: ConfirmationTime = match self - .indexed_graph - .graph() - .get_chain_position(&self.chain, chain_tip, txid) - { - Some(chain_position) => chain_position.cloned().into(), + let chain_position = match self.indexed_graph.graph().get_chain_position( + &self.chain, + chain_tip, + txid, + ) { + Some(chain_position) => chain_position.cloned(), None => return false, }; // Whether the UTXO is mature and, if needed, confirmed let mut spendable = true; - if must_only_use_confirmed_tx && !confirmation_time.is_confirmed() { + if must_only_use_confirmed_tx && !chain_position.is_confirmed() { return false; } if tx.is_coinbase() { debug_assert!( - confirmation_time.is_confirmed(), + chain_position.is_confirmed(), "coinbase must always be confirmed" ); if let Some(current_height) = current_height { - match confirmation_time { - ConfirmationTime::Confirmed { height, .. } => { + match chain_position { + ChainPosition::Confirmed(a) => { // https://github.com/bitcoin/bitcoin/blob/c5e67be03bb06a5d7885c55db1f016fbf2333fe3/src/validation.cpp#L373-L375 - spendable &= - (current_height.saturating_sub(height)) >= COINBASE_MATURITY; + spendable &= (current_height.saturating_sub(a.block_id.height)) + >= COINBASE_MATURITY; } - ConfirmationTime::Unconfirmed { .. } => spendable = false, + ChainPosition::Unconfirmed { .. } => spendable = false, } } } @@ -2546,7 +2545,7 @@ fn new_local_utxo( outpoint: full_txo.outpoint, txout: full_txo.txout, is_spent: full_txo.spent_by.is_some(), - confirmation_time: full_txo.chain_position.into(), + chain_position: full_txo.chain_position, keychain, derivation_index, } diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs index c3dde7330..343734a8d 100644 --- a/crates/wallet/src/wallet/tx_builder.rs +++ b/crates/wallet/src/wallet/tx_builder.rs @@ -858,7 +858,6 @@ mod test { }; } - use bdk_chain::ConfirmationTime; use bitcoin::consensus::deserialize; use bitcoin::hex::FromHex; use bitcoin::TxOut; @@ -1018,7 +1017,7 @@ mod test { txout: TxOut::NULL, keychain: KeychainKind::External, is_spent: false, - confirmation_time: ConfirmationTime::Unconfirmed { last_seen: 0 }, + chain_position: chain::ChainPosition::Unconfirmed(0), derivation_index: 0, }, LocalOutput { @@ -1029,10 +1028,13 @@ mod test { txout: TxOut::NULL, keychain: KeychainKind::Internal, is_spent: false, - confirmation_time: ConfirmationTime::Confirmed { - height: 32, - time: 42, - }, + chain_position: chain::ChainPosition::Confirmed(chain::ConfirmationBlockTime { + block_id: chain::BlockId { + height: 32, + hash: bitcoin::BlockHash::all_zeros(), + }, + confirmation_time: 42, + }), derivation_index: 1, }, ] diff --git a/crates/wallet/tests/common.rs b/crates/wallet/tests/common.rs index 375d680d1..a2870c807 100644 --- a/crates/wallet/tests/common.rs +++ b/crates/wallet/tests/common.rs @@ -1,5 +1,5 @@ #![allow(unused)] -use bdk_chain::{tx_graph, BlockId, ConfirmationBlockTime, ConfirmationTime, TxGraph}; +use bdk_chain::{tx_graph, BlockId, ChainPosition, ConfirmationBlockTime, TxGraph}; use bdk_wallet::{CreateParams, KeychainKind, LocalOutput, Update, Wallet}; use bitcoin::{ hashes::Hash, transaction, Address, Amount, BlockHash, FeeRate, Network, OutPoint, Transaction, @@ -89,20 +89,26 @@ pub fn get_funded_wallet_with_change(descriptor: &str, change: &str) -> (Wallet, insert_anchor_from_conf( &mut wallet, tx0.compute_txid(), - ConfirmationTime::Confirmed { - height: 1_000, - time: 100, - }, + ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: BlockId { + height: 1_000, + hash: BlockHash::all_zeros(), + }, + confirmation_time: 100, + }), ); wallet.insert_tx(tx1.clone()); insert_anchor_from_conf( &mut wallet, tx1.compute_txid(), - ConfirmationTime::Confirmed { - height: 2_000, - time: 200, - }, + ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: BlockId { + height: 2_000, + hash: BlockHash::all_zeros(), + }, + confirmation_time: 200, + }), ); (wallet, tx1.compute_txid()) @@ -205,19 +211,12 @@ pub fn feerate_unchecked(sat_vb: f64) -> FeeRate { /// Simulates confirming a tx with `txid` at the specified `position` by inserting an anchor /// at the lowest height in local chain that is greater or equal to `position`'s height, /// assuming the confirmation time matches `ConfirmationTime::Confirmed`. -pub fn insert_anchor_from_conf(wallet: &mut Wallet, txid: Txid, position: ConfirmationTime) { - if let ConfirmationTime::Confirmed { height, time } = position { - // anchor tx to checkpoint with lowest height that is >= position's height - let anchor = wallet - .local_chain() - .range(height..) - .last() - .map(|anchor_cp| ConfirmationBlockTime { - block_id: anchor_cp.block_id(), - confirmation_time: time, - }) - .expect("confirmation height cannot be greater than tip"); - +pub fn insert_anchor_from_conf( + wallet: &mut Wallet, + txid: Txid, + position: ChainPosition, +) { + if let ChainPosition::Confirmed(anchor) = position { wallet .apply_update(Update { tx_update: tx_graph::TxUpdate { diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 5914ce289..44b194a83 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -6,7 +6,7 @@ use std::str::FromStr; use anyhow::Context; use assert_matches::assert_matches; use bdk_chain::{tx_graph, COINBASE_MATURITY}; -use bdk_chain::{BlockId, ConfirmationTime}; +use bdk_chain::{BlockId, ChainPosition, ConfirmationBlockTime}; use bdk_wallet::coin_selection::{self, LargestFirstCoinSelection}; use bdk_wallet::descriptor::{calc_checksum, DescriptorError, IntoWalletDescriptor}; use bdk_wallet::error::CreateTxError; @@ -32,7 +32,11 @@ use rand::SeedableRng; mod common; use common::*; -fn receive_output(wallet: &mut Wallet, value: u64, height: ConfirmationTime) -> OutPoint { +fn receive_output( + wallet: &mut Wallet, + value: u64, + height: ChainPosition, +) -> OutPoint { let addr = wallet.next_unused_address(KeychainKind::External).address; receive_output_to_address(wallet, addr, value, height) } @@ -41,7 +45,7 @@ fn receive_output_to_address( wallet: &mut Wallet, addr: Address, value: u64, - height: ConfirmationTime, + height: ChainPosition, ) -> OutPoint { let tx = Transaction { version: transaction::Version::ONE, @@ -57,10 +61,10 @@ fn receive_output_to_address( wallet.insert_tx(tx); match height { - ConfirmationTime::Confirmed { .. } => { + ChainPosition::Confirmed { .. } => { insert_anchor_from_conf(wallet, txid, height); } - ConfirmationTime::Unconfirmed { last_seen } => { + ChainPosition::Unconfirmed(last_seen) => { insert_seen_at(wallet, txid, last_seen); } } @@ -72,9 +76,12 @@ fn receive_output_in_latest_block(wallet: &mut Wallet, value: u64) -> OutPoint { let latest_cp = wallet.latest_checkpoint(); let height = latest_cp.height(); let anchor = if height == 0 { - ConfirmationTime::Unconfirmed { last_seen: 0 } + ChainPosition::Unconfirmed(0) } else { - ConfirmationTime::Confirmed { height, time: 0 } + ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: latest_cp.block_id(), + confirmation_time: 0, + }) }; receive_output(wallet, value, anchor) } @@ -1209,14 +1216,11 @@ fn test_create_tx_add_utxo() { }; let txid = small_output_tx.compute_txid(); wallet.insert_tx(small_output_tx); - insert_anchor_from_conf( - &mut wallet, - txid, - ConfirmationTime::Confirmed { - height: 2000, - time: 200, - }, - ); + let chain_position = ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: wallet.latest_checkpoint().get(2000).unwrap().block_id(), + confirmation_time: 200, + }); + insert_anchor_from_conf(&mut wallet, txid, chain_position); let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") .unwrap() @@ -1259,14 +1263,11 @@ fn test_create_tx_manually_selected_insufficient() { }; let txid = small_output_tx.compute_txid(); wallet.insert_tx(small_output_tx.clone()); - insert_anchor_from_conf( - &mut wallet, - txid, - ConfirmationTime::Confirmed { - height: 2000, - time: 200, - }, - ); + let chain_position = ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: wallet.latest_checkpoint().get(2000).unwrap().block_id(), + confirmation_time: 200, + }); + insert_anchor_from_conf(&mut wallet, txid, chain_position); let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") .unwrap() @@ -1491,7 +1492,7 @@ fn test_create_tx_increment_change_index() { .create_wallet_no_persist() .unwrap(); // fund wallet - receive_output(&mut wallet, amount, ConfirmationTime::unconfirmed(0)); + receive_output(&mut wallet, amount, ChainPosition::Unconfirmed(0)); // create tx let mut builder = wallet.build_tx(); builder.add_recipient(recipient.clone(), Amount::from_sat(test.to_send)); @@ -1822,14 +1823,12 @@ fn test_bump_fee_confirmed_tx() { let txid = tx.compute_txid(); wallet.insert_tx(tx); - insert_anchor_from_conf( - &mut wallet, - txid, - ConfirmationTime::Confirmed { - height: 42, - time: 42_000, - }, - ); + + let chain_position = ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: wallet.latest_checkpoint().get(42).unwrap().block_id(), + confirmation_time: 42_000, + }); + insert_anchor_from_conf(&mut wallet, txid, chain_position); wallet.build_fee_bump(txid).unwrap().finish().unwrap(); } @@ -2101,16 +2100,12 @@ fn test_bump_fee_drain_wallet() { }], }; let txid = tx.compute_txid(); - let tip = wallet.latest_checkpoint().height(); wallet.insert_tx(tx.clone()); - insert_anchor_from_conf( - &mut wallet, - txid, - ConfirmationTime::Confirmed { - height: tip, - time: 42_000, - }, - ); + let chain_position = ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: wallet.latest_checkpoint().block_id(), + confirmation_time: 42_000, + }); + insert_anchor_from_conf(&mut wallet, txid, chain_position); let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX") .unwrap() @@ -2166,13 +2161,12 @@ fn test_bump_fee_remove_output_manually_selected_only() { value: Amount::from_sat(25_000), }], }; - let position: ConfirmationTime = wallet + let position: ChainPosition = wallet .transactions() .last() .unwrap() .chain_position - .cloned() - .into(); + .cloned(); wallet.insert_tx(init_tx.clone()); insert_anchor_from_conf(&mut wallet, init_tx.compute_txid(), position); @@ -2220,13 +2214,12 @@ fn test_bump_fee_add_input() { }], }; let txid = init_tx.compute_txid(); - let pos: ConfirmationTime = wallet + let pos: ChainPosition = wallet .transactions() .last() .unwrap() .chain_position - .cloned() - .into(); + .cloned(); wallet.insert_tx(init_tx); insert_anchor_from_conf(&mut wallet, txid, pos); @@ -2621,11 +2614,7 @@ fn test_bump_fee_unconfirmed_inputs_only() { let psbt = builder.finish().unwrap(); // Now we receive one transaction with 0 confirmations. We won't be able to use that for // fee bumping, as it's still unconfirmed! - receive_output( - &mut wallet, - 25_000, - ConfirmationTime::Unconfirmed { last_seen: 0 }, - ); + receive_output(&mut wallet, 25_000, ChainPosition::Unconfirmed(0)); let mut tx = psbt.extract_tx().expect("failed to extract tx"); let txid = tx.compute_txid(); for txin in &mut tx.input { @@ -2651,7 +2640,7 @@ fn test_bump_fee_unconfirmed_input() { .assume_checked(); // We receive a tx with 0 confirmations, which will be used as an input // in the drain tx. - receive_output(&mut wallet, 25_000, ConfirmationTime::unconfirmed(0)); + receive_output(&mut wallet, 25_000, ChainPosition::Unconfirmed(0)); let mut builder = wallet.build_tx(); builder.drain_wallet().drain_to(addr.script_pubkey()); let psbt = builder.finish().unwrap(); @@ -3855,12 +3844,11 @@ fn test_spend_coinbase() { .unwrap(); let confirmation_height = 5; - wallet - .insert_checkpoint(BlockId { - height: confirmation_height, - hash: BlockHash::all_zeros(), - }) - .unwrap(); + let confirmation_block_id = BlockId { + height: confirmation_height, + hash: BlockHash::all_zeros(), + }; + wallet.insert_checkpoint(confirmation_block_id).unwrap(); let coinbase_tx = Transaction { version: transaction::Version::ONE, lock_time: absolute::LockTime::ZERO, @@ -3877,14 +3865,11 @@ fn test_spend_coinbase() { }; let txid = coinbase_tx.compute_txid(); wallet.insert_tx(coinbase_tx); - insert_anchor_from_conf( - &mut wallet, - txid, - ConfirmationTime::Confirmed { - height: confirmation_height, - time: 30_000, - }, - ); + let chain_position = ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: confirmation_block_id, + confirmation_time: 30_000, + }); + insert_anchor_from_conf(&mut wallet, txid, chain_position); let not_yet_mature_time = confirmation_height + COINBASE_MATURITY - 1; let maturity_time = confirmation_height + COINBASE_MATURITY; @@ -4126,15 +4111,14 @@ fn test_keychains_with_overlapping_spks() { .last() .unwrap() .address; - let _outpoint = receive_output_to_address( - &mut wallet, - addr, - 8000, - ConfirmationTime::Confirmed { - height: 2000, - time: 0, + let chain_position = ChainPosition::Confirmed(ConfirmationBlockTime { + block_id: BlockId { + height: 8000, + hash: BlockHash::all_zeros(), }, - ); + confirmation_time: 0, + }); + let _outpoint = receive_output_to_address(&mut wallet, addr, 8000, chain_position); assert_eq!(wallet.balance().confirmed, Amount::from_sat(58000)); } @@ -4266,11 +4250,7 @@ fn single_descriptor_wallet_can_create_tx_and_receive_change() { .unwrap(); assert_eq!(wallet.keychains().count(), 1); let amt = Amount::from_sat(5_000); - receive_output( - &mut wallet, - 2 * amt.to_sat(), - ConfirmationTime::Unconfirmed { last_seen: 2 }, - ); + receive_output(&mut wallet, 2 * amt.to_sat(), ChainPosition::Unconfirmed(2)); // create spend tx that produces a change output let addr = Address::from_str("bcrt1qc6fweuf4xjvz4x3gx3t9e0fh4hvqyu2qw4wvxm") .unwrap() @@ -4297,7 +4277,7 @@ fn single_descriptor_wallet_can_create_tx_and_receive_change() { #[test] fn test_transactions_sort_by() { let (mut wallet, _txid) = get_funded_wallet_wpkh(); - receive_output(&mut wallet, 25_000, ConfirmationTime::unconfirmed(0)); + receive_output(&mut wallet, 25_000, ChainPosition::Unconfirmed(0)); // sort by chain position, unconfirmed then confirmed by descending block height let sorted_txs: Vec = From 74e19d198d6539670ac86d9744406bdd4656cf52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 9 Oct 2024 14:12:53 +0000 Subject: [PATCH 2/4] feat(wallet)!: change `insert_tx` behavior to include `last_seen` Previously, `Wallet::insert_tx` would insert a tx without a `last_seen` value. This meant that inserted transactions will not show up in the canonical history, nor be part of the UTXO set. Hence, new transactions created by the wallet may implicitly replace inserted transactions. This is somewhat unexpected for users. The new behavior of `insert_tx` inserts a tx with the current timestamp as `last_seen`. This, of course requires `std`. Therefore, we also introduced `insert_tx_at` which allows the caller to manually specify the `last_seen` timestamp. In addition, `test_insert_tx_balance_and_utxos` is removed since the behavior being tested is no longer expected. --- crates/wallet/src/wallet/mod.rs | 42 ++++++++++++++++++++++++--------- crates/wallet/tests/wallet.rs | 42 --------------------------------- 2 files changed, 31 insertions(+), 53 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 562e684c1..927c3efbb 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -1081,22 +1081,42 @@ impl Wallet { Ok(changed) } - /// Add a transaction to the wallet's internal view of the chain. This stages the change, - /// you must persist it later. + /// Insert a transaction into the wallet with the current timestamp and stages the changes. + /// Returns whether the transaction is newly introduced. /// - /// This method inserts the given `tx` and returns whether anything changed after insertion, - /// which will be false if the same transaction already exists in the wallet's transaction - /// graph. Any changes are staged but not committed. + /// This method is intended to be called after a transaction (which is created with this wallet) + /// is successfully broadcasted. This allows the newly broadcasted transaction to exist in the + /// wallet's canonical view of transactions and UTXO set. /// - /// # Note - /// - /// By default the inserted `tx` won't be considered "canonical" because it's not known - /// whether the transaction exists in the best chain. To know whether it exists, the tx - /// must be broadcast to the network and the wallet synced via a chain source. + /// To set the timestamp manually, use [`insert_tx_at`](Self::insert_tx_at). + #[cfg(feature = "std")] + #[cfg_attr(docsrs, doc(cfg(feature = "std")))] pub fn insert_tx>>(&mut self, tx: T) -> bool { + use std::time::*; + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("time now must surpass epoch anchor"); + self.insert_tx_at(tx, now.as_secs()) + } + + /// Insert a transaction into the wallet with the given `seen_at` and stages the changes. + /// Returns whether the transaction is newly introduced. + /// + /// This method is intended to be called after a transaction (which is created with this wallet) + /// is successfully broadcasted. This allows the newly broadcasted transaction to exist in the + /// wallet's canonical view of transactions and UTXO set. + /// + /// `seen_at` represents when the transaction is last seen by the mempool. This can be the time + /// when the transaction is broadcasted. To use the current timestamp, + /// [`insert_tx`](Self::insert_tx) can be called instead. + pub fn insert_tx_at>>(&mut self, tx: T, seen_at: u64) -> bool { + let tx: Arc = tx.into(); + let txid = tx.compute_txid(); + let mut changeset = ChangeSet::default(); changeset.merge(self.indexed_graph.insert_tx(tx).into()); - let ret = !changeset.is_empty(); + changeset.merge(self.indexed_graph.insert_seen_at(txid, seen_at).into()); + let ret = !changeset.tx_graph.txs.is_empty(); self.stage.merge(changeset); ret } diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 44b194a83..718db9fa7 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -4199,48 +4199,6 @@ fn test_thread_safety() { thread_safe::(); // compiles only if true } -#[test] -fn test_insert_tx_balance_and_utxos() { - // creating many txs has no effect on the wallet's available utxos - let (mut wallet, _) = get_funded_wallet(get_test_tr_single_sig_xprv()); - let addr = Address::from_str("bcrt1qc6fweuf4xjvz4x3gx3t9e0fh4hvqyu2qw4wvxm") - .unwrap() - .assume_checked(); - - let unspent: Vec<_> = wallet.list_unspent().collect(); - assert!(!unspent.is_empty()); - - let balance = wallet.balance().total(); - let fee = Amount::from_sat(143); - let amt = balance - fee; - - for _ in 0..3 { - let mut builder = wallet.build_tx(); - builder.add_recipient(addr.script_pubkey(), amt); - let mut psbt = builder.finish().unwrap(); - assert!(wallet.sign(&mut psbt, SignOptions::default()).unwrap()); - let tx = psbt.extract_tx().unwrap(); - let _ = wallet.insert_tx(tx); - } - assert_eq!(wallet.list_unspent().collect::>(), unspent); - assert_eq!(wallet.balance().confirmed, balance); - - // manually setting a tx last_seen will consume the wallet's available utxos - let addr = Address::from_str("bcrt1qfjg5lv3dvc9az8patec8fjddrs4aqtauadnagr") - .unwrap() - .assume_checked(); - let mut builder = wallet.build_tx(); - builder.add_recipient(addr.script_pubkey(), amt); - let mut psbt = builder.finish().unwrap(); - assert!(wallet.sign(&mut psbt, SignOptions::default()).unwrap()); - let tx = psbt.extract_tx().unwrap(); - let txid = tx.compute_txid(); - let _ = wallet.insert_tx(tx); - insert_seen_at(&mut wallet, txid, 2); - assert!(wallet.list_unspent().next().is_none()); - assert_eq!(wallet.balance().total().to_sat(), 0); -} - #[test] fn single_descriptor_wallet_can_create_tx_and_receive_change() { // create single descriptor wallet and fund it From 59e07586234c70bc6de80619a08ad1da088d7bb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 9 Oct 2024 14:19:58 +0000 Subject: [PATCH 3/4] revert(wallet)!: rm `Wallet::unbroadcast_transactions` This is no longer relevant as we direct callers to only insert tx into the wallet after a successful broadcast. --- crates/wallet/src/wallet/mod.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 927c3efbb..8f0b5ccdf 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -33,7 +33,7 @@ use bdk_chain::{ FullScanRequest, FullScanRequestBuilder, FullScanResult, SyncRequest, SyncRequestBuilder, SyncResult, }, - tx_graph::{CanonicalTx, TxGraph, TxNode, TxUpdate}, + tx_graph::{CanonicalTx, TxGraph, TxUpdate}, BlockId, ChainPosition, ConfirmationBlockTime, DescriptorExt, FullTxOut, Indexed, IndexedTxGraph, Merge, }; @@ -2380,14 +2380,6 @@ impl Wallet { self.indexed_graph.graph() } - /// Iterate over transactions in the wallet that are unseen and unanchored likely - /// because they haven't been broadcast. - pub fn unbroadcast_transactions( - &self, - ) -> impl Iterator, ConfirmationBlockTime>> { - self.tx_graph().txs_with_no_anchor_or_last_seen() - } - /// Get a reference to the inner [`KeychainTxOutIndex`]. pub fn spk_index(&self) -> &KeychainTxOutIndex { &self.indexed_graph.index From 4f73a770a789ce7b49c9f816291f7f7db9c8caf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 9 Oct 2024 14:36:27 +0000 Subject: [PATCH 4/4] feat(wallet)!: make `seen_at` mandatory for `Wallet::apply_update_at` Not including a `seen_at` alongside the update means the unconfirmed txs of the update will not be considered to be part of the canonical history. Therefore, transactions created with this wallet may replace the update's unconfirmed txs (which is unintuitive behavior). Also updated the docs. --- crates/wallet/src/wallet/mod.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 8f0b5ccdf..f749013f2 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/crates/wallet/src/wallet/mod.rs @@ -2311,10 +2311,10 @@ impl Wallet { let now = SystemTime::now() .duration_since(UNIX_EPOCH) .expect("time now must surpass epoch anchor"); - self.apply_update_at(update, Some(now.as_secs())) + self.apply_update_at(update, now.as_secs()) } - /// Applies an `update` alongside an optional `seen_at` timestamp and stages the changes. + /// Applies an `update` alongside a `seen_at` timestamp and stages the changes. /// /// `seen_at` represents when the update is seen (in unix seconds). It is used to determine the /// `last_seen`s for all transactions in the update which have no corresponding anchor(s). The @@ -2322,15 +2322,12 @@ impl Wallet { /// transactions (where the transaction with the lower `last_seen` value is omitted from the /// canonical history). /// - /// Not setting a `seen_at` value means unconfirmed transactions introduced by this update will - /// not be part of the canonical history of transactions. - /// /// Use [`apply_update`](Wallet::apply_update) to have the `seen_at` value automatically set to /// the current time. pub fn apply_update_at( &mut self, update: impl Into, - seen_at: Option, + seen_at: u64, ) -> Result<(), CannotConnectError> { let update = update.into(); let mut changeset = match update.chain { @@ -2345,7 +2342,7 @@ impl Wallet { changeset.merge(index_changeset.into()); changeset.merge( self.indexed_graph - .apply_update_at(update.tx_update, seen_at) + .apply_update_at(update.tx_update, Some(seen_at)) .into(), ); self.stage.merge(changeset);