Introduce `missing-at`/`last-missing` timestamps #1811

evanlinjin · 2025-01-24T06:54:21Z

Description

This PR replaces #1765. For context and the original discussion that led to this change, please refer to this comment.

This PR addresses a potential malicious double-spending issue by introducing improvements to unconfirmed transaction tracking. Key changes include the addition of TxUpdate::missing that tracks transactions that have been replaced and are no longer in the mempool, and the inclusion of last_missing and missing_at timestamps in TxGraph to track when a transaction was last deemed missing.

SpkWithExpectedTxids is introduced in SpkClient to track expected Txids for each spk. During a sync, if any Txids from SpkWithExpectedTxids are not in the current history of an spk obtained from the chain source, those Txids are considered missing. Support for SpkWithExpectedTxids has been added to both bdk_electrum and bdk_esplora chain source crates.

The canonicalization algorithm is updated to disregard transactions with a last_missing timestamp greater than or equal to their last_seen timestamp, except in cases where transitivity rules apply.

Changelog notice

TxUpdate::missing tracks transactions that have been replaced and are no longer present in mempool.
last_missing and missing_at timestamps in TxGraph track when a transaction was last marked as missing from the mempool.
The canonicalization algorithm now disregards transactions with a last_missing timestamp greater than or equal to it's last_seen timestamp, except when a canonical descendant exists due to rules of transitivity.
SpkWithExpectedTxids in SpkClient keeps track of expected Txids for each spk.
SpkWithExpectedTxids support added for bdk_electrum and bdk_esplora.
SyncRequestBuilder::expected_txids_of_spk adds an association between Txid and spk.
SyncRequestExt::check_unconfirmed_statuses adds unconfirmed transactions alongside their spks during sync.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

This may, if we introduce new fields to `TxUdpate`, they can be minor non-breaking updates.

This is a set of txids missing from the mempool.

notmandatory · 2025-01-24T16:18:17Z

Looks like only breaking changes are on core and chain, so we'll be able to roll this out in a non-breaking 1.x wallet release?

LLFourn · 2025-01-28T05:06:30Z

@notmandatory core breaking change is also a wallet crate breaking change unfortunately. It is a minor breaking change (some struct from core is changing that no one inspects directly in their code probably) but it technically is a breaking change so it would have to be in bdk_wallet v2. This is a pretty important improvement so I'd say it justifies a v2 release by itself when it's merged.

LLFourn

ConceptACK. Nice ideas here. I think chain API needs a clean up.

LLFourn · 2025-01-28T05:31:28Z

crates/chain/src/indexer/keychain_txout.rs

@@ -879,6 +881,23 @@ pub trait SyncRequestBuilderExt<K> {

    /// Add [`Script`](bitcoin::Script)s that are revealed by the `indexer` but currently unused.
    fn unused_spks_from_indexer(self, indexer: &KeychainTxOutIndex<K>) -> Self;
+


⛏️ I don't really see the attraction of a extension trait for this stuff. I think this should all be just member methods.

LLFourn · 2025-01-28T05:34:09Z

crates/chain/src/indexer/keychain_txout.rs

+    fn check_for_missing_txs<A, C>(
+        self,
+        indexer: &KeychainTxOutIndex<K>,
+        canonical_iter: CanonicalIter<A, C>,


This seems extremely awkward. What about ChainOracles that are not infallible? Why does it need be an KeychainTxOutIndex -- it just calls inner in the implementation.

How about you just put this method on the tx graph. Like:

impl<I, D: AsRef<SpkTxOutIndex<I>>> IndexedTxGraph<D> { fn sync_request_add_expected_unconfirmed_txs<O: ChainOracle>(&self, sync_request: &mut SyncRequestBuilder, chain_oracle: &O, range: impl RangeBounds<I>) -> Result<(), O::Error> { ... } } impl<K> IndexedTxGraph<KeychainTxOutIndex<K>> { fn keychain_sync_request_add_expected_unconfirmed_txs<O: ChainOracle>(&self, sync_request: &mut SyncRequestBuilder, chain_oracle: &O, range: impl RangeBounds<K>) -> Result<(), O::Error> { ... } }

Building on this comment I put some ideas for a IndexedTxGraph method in evanlinjin#14

Thanks @LLFourn, the infallible part was pure laziness.

I wanted to have the extension traits since bdk_core::spk_client types have a builder API. However, @ValuedMammal's has already provided a superior solution - I'm happy to go with that!

LLFourn · 2025-01-28T05:44:49Z

crates/chain/src/indexer/spk_txout.rs

@@ -334,4 +334,21 @@ impl<I: Clone + Ord + core::fmt::Debug> SpkTxOutIndex<I> {
            .any(|output| self.spk_indices.contains_key(&output.script_pubkey));
        input_matches || output_matches
    }
+
+    /// Find relevant script pubkeys associated with a transaction.


⛏️ doesn't define relevant here.

LLFourn · 2025-01-28T05:45:40Z

crates/chain/src/indexer/spk_txout.rs

+
+    /// Find relevant script pubkeys associated with a transaction.
+    ///
+    /// The script pubkeys of the transaction's outputs and previous outputs as scanned.


unhelpful sentence. Why. What do I use this method for.

Also needs to explain that inputs would not be detected as relevant unless the parents have been scanned.

LLFourn · 2025-01-28T06:25:12Z

crates/chain/src/tx_graph.rs

@@ -715,6 +717,30 @@ impl<A: Anchor> TxGraph<A> {
        changeset
    }

+    /// Inserts the given `missing_at` for `txid`


🔧 there is no explanation of missing_at.

ValuedMammal · 2025-02-01T16:08:16Z

crates/chain/src/tx_graph.rs

@@ -145,6 +145,7 @@ pub struct TxGraph<A = ConfirmationBlockTime> {
    spends: BTreeMap<OutPoint, HashSet<Txid>>,
    anchors: HashMap<Txid, BTreeSet<A>>,
    last_seen: HashMap<Txid, u64>,
+    last_missing: HashMap<Txid, u64>,


I think it would be nice if we had a data structure like this and then last_seen would become HashMap<Txid, LastSeen>

#[derive(Debug, Clone, Copy, Default, PartialEq)] struct LastSeen { /// seen at seen_at: u64, /// evicted at evicted_at: Option<u64>, }

I quite like this idea since there has been talks about adding more timestamps.

I.e. first-seen timestamp, which will help with tx ordering when listing transactions.

struct MempoolTimestamps { pub first_seen: u64, pub last_seen: u64, pub last_evicted: Option<u64>, }

I also like the term evicted as it's more descriptive than missing.

I agree, and +1 on this idea, it makes it clear, and easy to "derive" its lifecycle and it's and can be useful info to expose to the users.

ValuedMammal · 2025-02-01T16:11:11Z

crates/chain/src/tx_graph.rs

+            .flat_map(|(txid, anchors)| anchors.into_iter().map(move |a| (a, txid)))
+            .collect();
+        tx_update.seen_ats = graph.last_seen.into_iter().collect();
+        tx_update


It seems like TxUpdate would need another field evicted_at in order to maintain roundtrip convertibility between TxUpdate and TxGraph.

Do you think the convertibility between TxGraph and TxUpdate is important? I'm thinking maybe we should just get rid of it. One can just construct an empty TxGraph and apply an TxUpdate.

It's probably ok to forego complete fungibility between graphs and updates, but at the same time I wonder how useful are the seen-ats without the corresponding evictions?

crates/chain/src/tx_graph.rs

crates/chain/src/indexer/keychain_txout.rs

crates/core/src/spk_client.rs

evanlinjin · 2025-02-02T10:23:47Z

crates/chain/src/tx_graph.rs

@@ -145,6 +145,7 @@ pub struct TxGraph<A = ConfirmationBlockTime> {
    spends: BTreeMap<OutPoint, HashSet<Txid>>,
    anchors: HashMap<Txid, BTreeSet<A>>,
    last_seen: HashMap<Txid, u64>,
+    last_missing: HashMap<Txid, u64>,


I quite like this idea since there has been talks about adding more timestamps.

I.e. first-seen timestamp, which will help with tx ordering when listing transactions.

struct MempoolTimestamps { pub first_seen: u64, pub last_seen: u64, pub last_evicted: Option<u64>, }

I also like the term evicted as it's more descriptive than missing.

evanlinjin · 2025-02-02T10:26:16Z

crates/chain/src/tx_graph.rs

+            .flat_map(|(txid, anchors)| anchors.into_iter().map(move |a| (a, txid)))
+            .collect();
+        tx_update.seen_ats = graph.last_seen.into_iter().collect();
+        tx_update


Do you think the convertibility between TxGraph and TxUpdate is important? I'm thinking maybe we should just get rid of it. One can just construct an empty TxGraph and apply an TxUpdate.

evanlinjin · 2025-02-02T10:30:38Z

crates/chain/src/indexer/keychain_txout.rs

+    fn check_for_missing_txs<A, C>(
+        self,
+        indexer: &KeychainTxOutIndex<K>,
+        canonical_iter: CanonicalIter<A, C>,


Thanks @LLFourn, the infallible part was pure laziness.

I wanted to have the extension traits since bdk_core::spk_client types have a builder API. However, @ValuedMammal's has already provided a superior solution - I'm happy to go with that!

The evicted-at and last-evicted timestamp informs the `TxGraph` when the transaction was last deemed as missing from the mempool. The canonicalization algorithm is changed to disregard transactions with a last-missing timestamp greater or equal to it's last-seen timestamp. The exception is when we have a canonical descendant due to rules of transitivity.

This is for conveniently adding associations of txid <-> spk. We expect that these txids exist in the spk history. Otherwise, it means the tx is evicted from the mempool and we need to update the `missing_at` value in the sync response.

This is a convenience method for adding unconfirmed txs alongside their associated spks the the sync request. This way, we will be able to detect evictions of these transactions from the mempool.

Rename `SyncRequestBuilderExt::check_unconfirmed_statuses` to `SyncRequestBuilderExt::check_for_missing_txs` and update docs.

oleonardolima

A general ConceptACK, some small nits.

oleonardolima · 2025-02-02T23:40:05Z

crates/chain/src/tx_graph.rs

-    /// Transactions without last-seens are excluded.
-    pub fn txids_by_descending_last_seen(&self) -> impl ExactSizeIterator<Item = (u64, Txid)> + '_ {
-        self.txs_by_last_seen.iter().copied().rev()
+    /// Transactions without last-seens are excluded. Transactions with a last-missing timestamp


Suggested change

/// Transactions without last-seens are excluded. Transactions with a last-missing timestamp

/// Transactions without last-seens are excluded. Transactions with a last-evicted timestamp

oleonardolima · 2025-02-02T23:40:45Z

crates/chain/src/tx_graph.rs

@@ -1139,6 +1182,8 @@ pub struct ChangeSet<A = ()> {
    pub anchors: BTreeSet<(A, Txid)>,
    /// Added last-seen unix timestamps of transactions.
    pub last_seen: BTreeMap<Txid, u64>,
+    /// Added timestamps of when a transaction is last missing from the mempool.


Suggested change

/// Added timestamps of when a transaction is last missing from the mempool.

/// Added timestamps of when a transaction is last evicted from the mempool.

oleonardolima · 2025-02-03T00:26:13Z

crates/chain/src/tx_graph.rs

@@ -145,6 +145,7 @@ pub struct TxGraph<A = ConfirmationBlockTime> {
    spends: BTreeMap<OutPoint, HashSet<Txid>>,
    anchors: HashMap<Txid, BTreeSet<A>>,
    last_seen: HashMap<Txid, u64>,
+    last_missing: HashMap<Txid, u64>,


I agree, and +1 on this idea, it makes it clear, and easy to "derive" its lifecycle and it's and can be useful info to expose to the users.

oleonardolima · 2025-02-03T01:20:44Z

crates/chain/src/tx_graph.rs

@@ -765,6 +791,14 @@ impl<A: Anchor> TxGraph<A> {
                changeset.merge(self.insert_seen_at(txid, seen_at));
            }
        }
+        for txid in update.missing {


I think it'd help to update apply_update_at documentation in order to mention the evicted_at too, in lines.

feat(core)!: Make TxUpdate non-exhaustive

3b9f3c7

This may, if we introduce new fields to `TxUdpate`, they can be minor non-breaking updates.

evanlinjin force-pushed the missing_marker branch from 287b146 to fe09eab Compare January 24, 2025 09:03

evanlinjin changed the title ~~Introduce MissingMarker~~ Introduce missing-at/last-missing timestamps Jan 24, 2025

evanlinjin assigned evanlinjin and LagginTimes Jan 24, 2025

LagginTimes force-pushed the missing_marker branch from 7f09b70 to 49a64ba Compare January 24, 2025 10:58

feat(core): Add TxUpdate::missing

5b17c60

This is a set of txids missing from the mempool.

LagginTimes force-pushed the missing_marker branch from 0f52ead to 2e31bc7 Compare January 24, 2025 15:27

notmandatory added the bug Something isn't working label Jan 24, 2025

LagginTimes force-pushed the missing_marker branch from 2c37d06 to bbd6340 Compare January 27, 2025 09:37

LLFourn requested changes Jan 28, 2025

View reviewed changes

evanlinjin marked this pull request as ready for review January 30, 2025 12:33

ValuedMammal reviewed Feb 1, 2025

View reviewed changes

evanlinjin commented Feb 2, 2025

View reviewed changes

evanlinjin and others added 10 commits February 3, 2025 01:00

feat(core): add SpkWithExpectedTxids support for SpkClient

adecacc

feat(chain): Update rusqlite_impl to persist last_missing

2cc99cf

feat(electrum): add SpkWithExpectedTxids support

77c9af6

feat(esplora): add SpkWithExpectedTxids support

348bfd5

feat(chain): Add SyncRequestExt::check_unconfirmed_statuses

9c6245f

This is a convenience method for adding unconfirmed txs alongside their associated spks the the sync request. This way, we will be able to detect evictions of these transactions from the mempool.

test(electrum): detect_receive_tx_cancel

b77e479

test(esplora): detect_receive_tx_cancel

d4a81e7

refactor(chain): SyncRequestBuilderExt::check_for_missing_txs

8c0d5c3

Rename `SyncRequestBuilderExt::check_unconfirmed_statuses` to `SyncRequestBuilderExt::check_for_missing_txs` and update docs.

LagginTimes force-pushed the missing_marker branch from bbd6340 to 8c0d5c3 Compare February 2, 2025 17:02

oleonardolima reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `missing-at`/`last-missing` timestamps #1811

Introduce `missing-at`/`last-missing` timestamps #1811

evanlinjin commented Jan 24, 2025 •

edited by LagginTimes

Loading

notmandatory commented Jan 24, 2025

LLFourn commented Jan 28, 2025

LLFourn left a comment

LLFourn Jan 28, 2025

LLFourn Jan 28, 2025

ValuedMammal Feb 1, 2025

evanlinjin Feb 2, 2025

LLFourn Jan 28, 2025

LLFourn Jan 28, 2025

LLFourn Jan 28, 2025

ValuedMammal Feb 1, 2025

evanlinjin Feb 2, 2025

oleonardolima Feb 3, 2025

ValuedMammal Feb 1, 2025

evanlinjin Feb 2, 2025

ValuedMammal Feb 2, 2025

evanlinjin Feb 2, 2025

evanlinjin Feb 2, 2025

evanlinjin Feb 2, 2025

oleonardolima left a comment

oleonardolima Feb 2, 2025

oleonardolima Feb 2, 2025

oleonardolima Feb 3, 2025

oleonardolima Feb 3, 2025 •

edited

Loading

		@@ -879,6 +881,23 @@ pub trait SyncRequestBuilderExt<K> {

		/// Add [`Script`](bitcoin::Script)s that are revealed by the `indexer` but currently unused.
		fn unused_spks_from_indexer(self, indexer: &KeychainTxOutIndex<K>) -> Self;

	/// Transactions without last-seens are excluded. Transactions with a last-missing timestamp
	/// Transactions without last-seens are excluded. Transactions with a last-evicted timestamp

	/// Added timestamps of when a transaction is last missing from the mempool.
	/// Added timestamps of when a transaction is last evicted from the mempool.

Introduce missing-at/last-missing timestamps #1811

Are you sure you want to change the base?

Introduce missing-at/last-missing timestamps #1811

Conversation

evanlinjin commented Jan 24, 2025 • edited by LagginTimes Loading

Description

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

notmandatory commented Jan 24, 2025

LLFourn commented Jan 28, 2025

LLFourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Introduce `missing-at`/`last-missing` timestamps #1811

Introduce `missing-at`/`last-missing` timestamps #1811

evanlinjin commented Jan 24, 2025 •

edited by LagginTimes

Loading

oleonardolima Feb 3, 2025 •

edited

Loading