frost/bdk
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(tx_graph)!: Change tx_last_seen to Option<u64>

Browse Source 
Also fixup `test_list_owned_txouts` to check that the right
outputs, utxos, and balance are returned at different local
chain heights.

This fixes an issue where unbroadcast and otherwise non-canonical
transactions were returned from methods `list_chain_txs` and
`Wallet::transactions` because every tx inserted had a last_seen
of 0 making it appear unconfirmed.

Note this commit changes the way `Balance` is represented due to
new logic in `try_get_chain_position` that no longer considers
txs with non-canonical anchors. Before this change, a tx anchored
to a block that is reorged out had a permanent effect on the
pending balance, and now only txs with a last_seen time or an
anchor confirmed in the best chain will return a `ChainPosition`.
This commit is contained in:
valued mammal 2024-05-23 17:33:45 -04:00
parent 324eeb3eb4
commit bbc19c3536
No known key found for this signature in database
7 changed files with 100 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
crates/bitcoind_rpc/tests/test_emitter.rs
View File

@ -392,7 +392,6 @@ fn tx_can_become_unconfirmed_after_reorg() -> anyhow::Result<()> {
get_balance(&recv_chain, &recv_graph)?,
Balance {
confirmed: SEND_AMOUNT * (ADDITIONAL_COUNT - reorg_count) as u64,
trusted_pending: SEND_AMOUNT * reorg_count as u64,
..Balance::default()
},
"reorg_count: {}",

38
crates/chain/src/tx_graph.rs
View File

@ -110,7 +110,7 @@ use core::{
#[derive(Clone, Debug, PartialEq)]
pub struct TxGraph<A = ()> {
// all transactions that the graph is aware of in format: `(tx_node, tx_anchors, tx_last_seen)`
txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, u64)>,
txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, Option<u64>)>,
spends: BTreeMap<OutPoint, HashSet<Txid>>,
anchors: BTreeSet<(A, Txid)>,
@ -140,7 +140,7 @@ pub struct TxNode<'a, T, A> {
/// The blocks that the transaction is "anchored" in.
pub anchors: &'a BTreeSet<A>,
/// The last-seen unix timestamp of the transaction as unconfirmed.
pub last_seen_unconfirmed: u64,
pub last_seen_unconfirmed: Option<u64>,
}
impl<'a, T, A> Deref for TxNode<'a, T, A> {
@ -504,7 +504,7 @@ impl<A: Clone + Ord> TxGraph<A> {
(
TxNodeInternal::Partial([(outpoint.vout, txout)].into()),
BTreeSet::new(),
0,
None,
),
);
self.apply_update(update)
@ -518,7 +518,7 @@ impl<A: Clone + Ord> TxGraph<A> {
let mut update = Self::default();
update.txs.insert(
tx.compute_txid(),
(TxNodeInternal::Whole(tx), BTreeSet::new(), 0),
(TxNodeInternal::Whole(tx), BTreeSet::new(), None),
);
self.apply_update(update)
}
@ -560,7 +560,7 @@ impl<A: Clone + Ord> TxGraph<A> {
pub fn insert_seen_at(&mut self, txid: Txid, seen_at: u64) -> ChangeSet<A> {
let mut update = Self::default();
let (_, _, update_last_seen) = update.txs.entry(txid).or_default();
*update_last_seen = seen_at;
*update_last_seen = Some(seen_at);
self.apply_update(update)
}
@ -669,7 +669,7 @@ impl<A: Clone + Ord> TxGraph<A> {
None => {
self.txs.insert(
txid,
(TxNodeInternal::Whole(wrapped_tx), BTreeSet::new(), 0),
(TxNodeInternal::Whole(wrapped_tx), BTreeSet::new(), None),
);
}
}
@ -696,8 +696,8 @@ impl<A: Clone + Ord> TxGraph<A> {
for (txid, new_last_seen) in changeset.last_seen {
let (_, _, last_seen) = self.txs.entry(txid).or_default();
if new_last_seen > *last_seen {
*last_seen = new_last_seen;
if Some(new_last_seen) > *last_seen {
*last_seen = Some(new_last_seen);
}
}
}
@ -710,10 +710,10 @@ impl<A: Clone + Ord> TxGraph<A> {
let mut changeset = ChangeSet::<A>::default();
for (&txid, (update_tx_node, _, update_last_seen)) in &update.txs {
let prev_last_seen: u64 = match (self.txs.get(&txid), update_tx_node) {
let prev_last_seen = match (self.txs.get(&txid), update_tx_node) {
(None, TxNodeInternal::Whole(update_tx)) => {
changeset.txs.insert(update_tx.clone());
0
None
}
(None, TxNodeInternal::Partial(update_txos)) => {
changeset.txouts.extend(
@ -721,7 +721,7 @@ impl<A: Clone + Ord> TxGraph<A> {
.iter()
.map(|(&vout, txo)| (OutPoint::new(txid, vout), txo.clone())),
);
0
None
}
(Some((TxNodeInternal::Whole(_), _, last_seen)), _) => *last_seen,
(
@ -746,7 +746,10 @@ impl<A: Clone + Ord> TxGraph<A> {
};
if *update_last_seen > prev_last_seen {
changeset.last_seen.insert(txid, *update_last_seen);
changeset.last_seen.insert(
txid,
update_last_seen.expect("checked is greater, so we must have a last_seen"),
);
}
}
@ -798,6 +801,13 @@ impl<A: Anchor> TxGraph<A> {
}
}
// If no anchors are in best chain and we don't have a last_seen, we can return
// early because by definition the tx doesn't have a chain position.
let last_seen = match last_seen {
Some(t) => *t,
None => return Ok(None),
};
// The tx is not anchored to a block in the best chain, which means that it
// might be in mempool, or it might have been dropped already.
// Let's check conflicts to find out!
@ -884,7 +894,7 @@ impl<A: Anchor> TxGraph<A> {
if conflicting_tx.last_seen_unconfirmed > tx_last_seen {
return Ok(None);
}
if conflicting_tx.last_seen_unconfirmed == *last_seen
if conflicting_tx.last_seen_unconfirmed == Some(last_seen)
&& conflicting_tx.as_ref().compute_txid() > tx.as_ref().compute_txid()
{
// Conflicting tx has priority if txid of conflicting tx > txid of original tx
@ -893,7 +903,7 @@ impl<A: Anchor> TxGraph<A> {
}
}
Ok(Some(ChainPosition::Unconfirmed(*last_seen)))
Ok(Some(ChainPosition::Unconfirmed(last_seen)))
}
/// Get the position of the transaction in `chain` with tip `chain_tip`.

4
crates/chain/tests/common/tx_template.rs
View File

@ -131,9 +131,7 @@ pub fn init_graph<'a, A: Anchor + Clone + 'a>(
for anchor in tx_tmp.anchors.iter() {
let _ = graph.insert_anchor(tx.compute_txid(), anchor.clone());
}
if let Some(seen_at) = tx_tmp.last_seen {
let _ = graph.insert_seen_at(tx.compute_txid(), seen_at);
}
let _ = graph.insert_seen_at(tx.compute_txid(), tx_tmp.last_seen.unwrap_or(0));
}
(graph, spk_index, tx_ids)
}

47
crates/chain/tests/test_indexed_tx_graph.rs
View File

@ -116,8 +116,8 @@ fn insert_relevant_txs() {
/// tx1: A Coinbase, sending 70000 sats to "trusted" address. [Block 0]
/// tx2: A external Receive, sending 30000 sats to "untrusted" address. [Block 1]
/// tx3: Internal Spend. Spends tx2 and returns change of 10000 to "trusted" address. [Block 2]
/// tx4: Mempool tx, sending 20000 sats to "trusted" address.
/// tx5: Mempool tx, sending 15000 sats to "untested" address.
/// tx4: Mempool tx, sending 20000 sats to "untrusted" address.
/// tx5: Mempool tx, sending 15000 sats to "trusted" address.
/// tx6: Complete unrelated tx. [Block 3]
///
/// Different transactions are added via `insert_relevant_txs`.
@ -160,7 +160,7 @@ fn test_list_owned_txouts() {
let mut untrusted_spks: Vec<ScriptBuf> = Vec::new();
{
// we need to scope here to take immutanble reference of the graph
// we need to scope here to take immutable reference of the graph
for _ in 0..10 {
let ((_, script), _) = graph
.index
@ -226,7 +226,7 @@ fn test_list_owned_txouts() {
..common::new_tx(0)
};
// tx5 is spending tx3 and receiving change at trusted keychain, unconfirmed.
// tx5 is an external transaction receiving at trusted keychain, unconfirmed.
let tx5 = Transaction {
output: vec![TxOut {
value: Amount::from_sat(15000),
@ -239,7 +239,7 @@ fn test_list_owned_txouts() {
let tx6 = common::new_tx(0);
// Insert transactions into graph with respective anchors
// For unconfirmed txs we pass in `None`.
// Insert unconfirmed txs with a last_seen timestamp
let _ =
graph.batch_insert_relevant([&tx1, &tx2, &tx3, &tx6].iter().enumerate().map(|(i, tx)| {
@ -291,9 +291,6 @@ fn test_list_owned_txouts() {
|_, spk: &Script| trusted_spks.contains(&spk.to_owned()),
);
assert_eq!(txouts.len(), 5);
assert_eq!(utxos.len(), 4);
let confirmed_txouts_txid = txouts
.iter()
.filter_map(|(_, full_txout)| {
@ -359,29 +356,25 @@ fn test_list_owned_txouts() {
balance,
) = fetch(0, &graph);
// tx1 is a confirmed txout and is unspent
// tx4, tx5 are unconfirmed
assert_eq!(confirmed_txouts_txid, [tx1.compute_txid()].into());
assert_eq!(
unconfirmed_txouts_txid,
[
tx2.compute_txid(),
tx3.compute_txid(),
tx4.compute_txid(),
tx5.compute_txid()
]
.into()
[tx4.compute_txid(), tx5.compute_txid()].into()
);
assert_eq!(confirmed_utxos_txid, [tx1.compute_txid()].into());
assert_eq!(
unconfirmed_utxos_txid,
[tx3.compute_txid(), tx4.compute_txid(), tx5.compute_txid()].into()
[tx4.compute_txid(), tx5.compute_txid()].into()
);
assert_eq!(
balance,
Balance {
immature: Amount::from_sat(70000), // immature coinbase
trusted_pending: Amount::from_sat(25000), // tx3 + tx5
trusted_pending: Amount::from_sat(15000), // tx5
untrusted_pending: Amount::from_sat(20000), // tx4
confirmed: Amount::ZERO // Nothing is confirmed yet
}
@ -405,23 +398,26 @@ fn test_list_owned_txouts() {
);
assert_eq!(
unconfirmed_txouts_txid,
[tx3.compute_txid(), tx4.compute_txid(), tx5.compute_txid()].into()
[tx4.compute_txid(), tx5.compute_txid()].into()
);
// tx2 doesn't get into confirmed utxos set
assert_eq!(confirmed_utxos_txid, [tx1.compute_txid()].into());
// tx2 gets into confirmed utxos set
assert_eq!(
confirmed_utxos_txid,
[tx1.compute_txid(), tx2.compute_txid()].into()
);
assert_eq!(
unconfirmed_utxos_txid,
[tx3.compute_txid(), tx4.compute_txid(), tx5.compute_txid()].into()
[tx4.compute_txid(), tx5.compute_txid()].into()
);
assert_eq!(
balance,
Balance {
immature: Amount::from_sat(70000), // immature coinbase
trusted_pending: Amount::from_sat(25000), // tx3 + tx5
trusted_pending: Amount::from_sat(15000), // tx5
untrusted_pending: Amount::from_sat(20000), // tx4
confirmed: Amount::ZERO // Nothing is confirmed yet
confirmed: Amount::from_sat(30_000) // tx2 got confirmed
}
);
}
@ -477,6 +473,7 @@ fn test_list_owned_txouts() {
balance,
) = fetch(98, &graph);
// no change compared to block 2
assert_eq!(
confirmed_txouts_txid,
[tx1.compute_txid(), tx2.compute_txid(), tx3.compute_txid()].into()
@ -502,14 +499,14 @@ fn test_list_owned_txouts() {
immature: Amount::from_sat(70000), // immature coinbase
trusted_pending: Amount::from_sat(15000), // tx5
untrusted_pending: Amount::from_sat(20000), // tx4
confirmed: Amount::from_sat(10000) // tx1 got matured
confirmed: Amount::from_sat(10000) // tx3 is confirmed
}
);
}
// AT Block 99
{
let (_, _, _, _, balance) = fetch(100, &graph);
let (_, _, _, _, balance) = fetch(99, &graph);
// Coinbase maturity hits
assert_eq!(

24
crates/chain/tests/test_tx_graph.rs
View File

@ -977,16 +977,6 @@ fn test_chain_spends() {
}))
);
// 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.block_id(),
OutPoint::new(tx_0.compute_txid(), 1)
),
Some((ChainPosition::Unconfirmed(0), tx_2.compute_txid())),
);
// Mark the unconfirmed as seen and check correct ObservedAs status is returned.
let _ = graph.insert_seen_at(tx_2.compute_txid(), 1234567);
@ -1099,10 +1089,10 @@ fn update_last_seen_unconfirmed() {
let txid = tx.compute_txid();
// insert a new tx
// initially we have a last_seen of 0, and no anchors
// initially we have a last_seen of None and no anchors
let _ = graph.insert_tx(tx);
let tx = graph.full_txs().next().unwrap();
assert_eq!(tx.last_seen_unconfirmed, 0);
assert_eq!(tx.last_seen_unconfirmed, None);
assert!(tx.anchors.is_empty());
// higher timestamp should update last seen
@ -1117,7 +1107,15 @@ fn update_last_seen_unconfirmed() {
let _ = graph.insert_anchor(txid, ());
let changeset = graph.update_last_seen_unconfirmed(4);
assert!(changeset.is_empty());
assert_eq!(graph.full_txs().next().unwrap().last_seen_unconfirmed, 2);
assert_eq!(
graph
.full_txs()
.next()
.unwrap()
.last_seen_unconfirmed
.unwrap(),
2
);
}
#[test]

7
crates/electrum/tests/test_electrum.rs
View File

@ -198,17 +198,14 @@ fn tx_can_become_unconfirmed_after_reorg() -> anyhow::Result<()> {
.apply_update(update.chain_update)
.map_err(|err| anyhow::anyhow!("LocalChain update error: {:?}", err))?;
// Check to see if a new anchor is added during current reorg.
if !initial_anchors.is_superset(update.graph_update.all_anchors()) {
println!("New anchor added at reorg depth {}", depth);
}
// Check that no new anchors are added during current reorg.
assert!(initial_anchors.is_superset(update.graph_update.all_anchors()));
let _ = recv_graph.apply_update(update.graph_update);
assert_eq!(
get_balance(&recv_chain, &recv_graph)?,
Balance {
confirmed: SEND_AMOUNT * (REORG_COUNT - depth) as u64,
trusted_pending: SEND_AMOUNT * depth as u64,
..Balance::default()
},
"reorg_count: {}",

50
crates/wallet/tests/wallet.rs
View File

@ -1187,7 +1187,16 @@ fn test_create_tx_add_utxo() {
version: transaction::Version::non_standard(0),
lock_time: absolute::LockTime::ZERO,
};
wallet.insert_tx(small_output_tx.clone());
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 addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX")
.unwrap()
@ -1195,10 +1204,7 @@ fn test_create_tx_add_utxo() {
let mut builder = wallet.build_tx();
builder
.add_recipient(addr.script_pubkey(), Amount::from_sat(30_000))
.add_utxo(OutPoint {
txid: small_output_tx.compute_txid(),
vout: 0,
})
.add_utxo(OutPoint { txid, vout: 0 })
.unwrap();
let psbt = builder.finish().unwrap();
let sent_received =
@ -1231,8 +1237,16 @@ fn test_create_tx_manually_selected_insufficient() {
version: transaction::Version::non_standard(0),
lock_time: absolute::LockTime::ZERO,
};
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 addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX")
.unwrap()
@ -1240,10 +1254,7 @@ fn test_create_tx_manually_selected_insufficient() {
let mut builder = wallet.build_tx();
builder
.add_recipient(addr.script_pubkey(), Amount::from_sat(30_000))
.add_utxo(OutPoint {
txid: small_output_tx.compute_txid(),
vout: 0,
})
.add_utxo(OutPoint { txid, vout: 0 })
.unwrap()
.manually_selected_only();
builder.finish().unwrap();
@ -1278,7 +1289,9 @@ fn test_create_tx_policy_path_no_csv() {
value: Amount::from_sat(50_000),
}],
};
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let external_policy = wallet.policies(KeychainKind::External).unwrap().unwrap();
let root_id = external_policy.id;
@ -1670,6 +1683,7 @@ fn test_bump_fee_irreplaceable_tx() {
let tx = psbt.extract_tx().expect("failed to extract tx");
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
wallet.build_fee_bump(txid).unwrap().finish().unwrap();
}
@ -1713,6 +1727,7 @@ fn test_bump_fee_low_fee_rate() {
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder.fee_rate(FeeRate::BROADCAST_MIN);
@ -1744,6 +1759,7 @@ fn test_bump_fee_low_abs() {
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder.fee_absolute(Amount::from_sat(10));
@ -1764,6 +1780,7 @@ fn test_bump_fee_zero_abs() {
let tx = psbt.extract_tx().expect("failed to extract tx");
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder.fee_absolute(Amount::ZERO);
@ -1788,6 +1805,7 @@ fn test_bump_fee_reduce_change() {
let tx = psbt.extract_tx().expect("failed to extract tx");
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let feerate = FeeRate::from_sat_per_kwu(625); // 2.5 sat/vb
let mut builder = wallet.build_fee_bump(txid).unwrap();
@ -1884,6 +1902,7 @@ fn test_bump_fee_reduce_single_recipient() {
let original_fee = check_fee!(wallet, psbt);
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let feerate = FeeRate::from_sat_per_kwu(625); // 2.5 sat/vb
let mut builder = wallet.build_fee_bump(txid).unwrap();
@ -1930,6 +1949,7 @@ fn test_bump_fee_absolute_reduce_single_recipient() {
let original_sent_received = wallet.sent_and_received(&tx);
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder
@ -2004,6 +2024,7 @@ fn test_bump_fee_drain_wallet() {
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
assert_eq!(original_sent_received.0, Amount::from_sat(25_000));
// for the new feerate, it should be enough to reduce the output, but since we specify
@ -2068,6 +2089,7 @@ fn test_bump_fee_remove_output_manually_selected_only() {
let original_sent_received = wallet.sent_and_received(&tx);
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
assert_eq!(original_sent_received.0, Amount::from_sat(25_000));
let mut builder = wallet.build_fee_bump(txid).unwrap();
@ -2114,6 +2136,7 @@ fn test_bump_fee_add_input() {
let original_details = wallet.sent_and_received(&tx);
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder.fee_rate(FeeRate::from_sat_per_vb_unchecked(50));
@ -2169,6 +2192,7 @@ fn test_bump_fee_absolute_add_input() {
let original_sent_received = wallet.sent_and_received(&tx);
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder.fee_absolute(Amount::from_sat(6_000));
@ -2233,6 +2257,7 @@ fn test_bump_fee_no_change_add_input_and_change() {
let tx = psbt.extract_tx().expect("failed to extract tx");
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
// Now bump the fees, the wallet should add an extra input and a change output, and leave
// the original output untouched.
@ -2301,6 +2326,7 @@ fn test_bump_fee_add_input_change_dust() {
assert_eq!(tx.output.len(), 2);
let txid = tx.compute_txid();
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
// We set a fee high enough that during rbf we are forced to add
@ -2370,6 +2396,7 @@ fn test_bump_fee_force_add_input() {
txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature
}
wallet.insert_tx(tx.clone());
wallet.insert_seen_at(txid, 0);
// the new fee_rate is low enough that just reducing the change would be fine, but we force
// the addition of an extra input with `add_utxo()`
let mut builder = wallet.build_fee_bump(txid).unwrap();
@ -2435,6 +2462,7 @@ fn test_bump_fee_absolute_force_add_input() {
txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature
}
wallet.insert_tx(tx.clone());
wallet.insert_seen_at(txid, 0);
// the new fee_rate is low enough that just reducing the change would be fine, but we force
// the addition of an extra input with `add_utxo()`
@ -2512,6 +2540,7 @@ fn test_bump_fee_unconfirmed_inputs_only() {
txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature
}
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder.fee_rate(FeeRate::from_sat_per_vb_unchecked(25));
builder.finish().unwrap();
@ -2543,6 +2572,7 @@ fn test_bump_fee_unconfirmed_input() {
txin.witness.push([0x00; P2WPKH_FAKE_WITNESS_SIZE]); // fake signature
}
wallet.insert_tx(tx);
wallet.insert_seen_at(txid, 0);
let mut builder = wallet.build_fee_bump(txid).unwrap();
builder