Introduce `evicted-at`/`last-evicted` 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_evicted and evicted_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_evicted 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_evicted and evicted_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_evicted 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

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.

crates/chain/src/indexer/keychain_txout.rs

crates/chain/src/indexer/spk_txout.rs

crates/chain/src/tx_graph.rs

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.

After some discussion with @evanlinjin, it was decided that this was best done in a separate PR as it may be too breaking of a change.

Yes. On second thought, this introduces an invariant that last_evicted must exist only if we have had a seen_at value. Because of the existence of this invariant, this invariant must be reflected in tx_graph::ChangeSet by changing ChangeSet::seen_at to have values of MempoolTimestamps. This change breaks our policy of changes to changeset which was to only add new fields.

Edit: Maybe we can make it a user-facing API.

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?

but at the same time I wonder how useful are the seen-ats without the corresponding evictions?

@ValuedMammal what do you mean by this?

I think it makes sense to completely remove convertibility of TxGraph -> TxUpdate and TxUpdate -> TxGraph.

Just pointing out that we lose information going from TxGraph to TxUpdate. Since both last-seen and last-evicted can influence canonicalization, it might make sense to include both (or neither).

Maybe it depends on the use case for TxUpdate. If updating a tx graph from a spk_client based chain source, the seen_ats don't seem to be as important compared to treating graphs and updates as interchangeable. If we remove convertibility, would it make sense to change this From impl to a method on TxGraph that returns a TxUpdate? We could potentially develop different kinds of updates, like one that is only relevant to a subset of keychains.

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.

crates/chain/src/indexer/keychain_txout.rs

oleonardolima

A general ConceptACK, some small nits.

crates/chain/src/tx_graph.rs

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.

crates/chain/src/tx_graph.rs

crates/bitcoind_rpc/tests/test_emitter.rs

crates/chain/src/indexer/spk_txout.rs

evanlinjin · 2025-02-07T04:43:53Z

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


but at the same time I wonder how useful are the seen-ats without the corresponding evictions?

@ValuedMammal what do you mean by this?

I think it makes sense to completely remove convertibility of TxGraph -> TxUpdate and TxUpdate -> TxGraph.

crates/chain/src/tx_graph.rs

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.

evanlinjin · 2025-02-13T06:23:23Z

crates/esplora/tests/async_ext.rs

+
+    env.mine_blocks(101, None)?;
+
+    // Select a UTXO to use as an input for constructing our test transactions.


Which test transaction?

example-crates/example_bitcoind_rpc_polling/src/main.rs

example-crates/example_wallet_rpc/src/main.rs

crates/bitcoind_rpc/src/lib.rs

crates/bitcoind_rpc/tests/test_emitter.rs

example-crates/example_bitcoind_rpc_polling/src/main.rs

evanlinjin · 2025-02-16T10:15:00Z

crates/chain/src/tx_graph.rs

+        let chain_tip = chain.get_chain_tip().unwrap();
+
+        self.list_canonical_txs(chain, chain_tip)
+            .filter(|c| !c.chain_position.is_confirmed() && indexer.is_tx_relevant(&c.tx_node))


Any confirmed tx that is missing from the Electrum's spk history should also be evicted. Since this means that there is a reorg and the tx is not re-introduced to the mempool (evicted).

crates/chain/src/tx_graph.rs

Make this method work when the indexer is `KeychainTxOutIndex`. We reintroduce the ability to get the internal `SpkTxOutIndex` from `KeychainTxOutIndex` so that `SpkTxOutIndex::relevant_spks_of_tx` is callable from `KeychainTxOutIndex`. This commit renames `iter_spks_with_expected_txids` to `expected_unconfirmed_spk_txids` for `TxGraph`, `IndexedTxGraph` and `SyncRequestBuilder`. Docs are also improved to explain how these methods are useful. Remove unused `SyncRequestBuilder` methods.

* Remove duplicate paragraphs about `ChangeSet`s. * Add "Canonicalization" section which expands on methods that require canonicalization and the associated data types used in the canonicalization algorithm.

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 2 times, most recently 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

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

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

LagginTimes force-pushed the missing_marker branch 3 times, most recently from 7f3b6d4 to be60ada Compare February 6, 2025 08:47

LagginTimes closed this Feb 6, 2025

LagginTimes reopened this Feb 6, 2025

LagginTimes mentioned this pull request Feb 6, 2025

fix(chain): detect incoming transaction being replaced/canceled #1765

Closed

7 tasks

evanlinjin changed the title ~~Introduce missing-at/last-missing timestamps~~ Introduce evicted-at/last-evicted timestamps Feb 7, 2025

evanlinjin commented Feb 7, 2025

View reviewed changes

LagginTimes force-pushed the missing_marker branch 3 times, most recently from 25b005c to 203c97d Compare February 7, 2025 09:32

ValuedMammal reviewed Feb 8, 2025

View reviewed changes

crates/chain/src/tx_graph.rs Show resolved Hide resolved

LagginTimes force-pushed the missing_marker branch from 203c97d to 7568867 Compare February 10, 2025 07:36

LagginTimes and others added 7 commits February 11, 2025 23:25

feat(electrum): add SpkWithExpectedTxids support

f7bf232

feat(esplora): add SpkWithExpectedTxids support

08b6e88

feat(chain): Add SyncRequestExt::check_unconfirmed_statuses

9422275

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

5cf96fc

test(esplora): detect_receive_tx_cancel

a201446

feat(chain): add method iter_spks_with_expected_txids

f3a1eb1

LagginTimes force-pushed the missing_marker branch from 7568867 to c209d1a Compare February 11, 2025 15:25

evanlinjin force-pushed the missing_marker branch from c209d1a to 18fad27 Compare February 12, 2025 01:56

evanlinjin commented Feb 13, 2025

View reviewed changes

LagginTimes marked this pull request as draft February 13, 2025 11:35

LagginTimes force-pushed the missing_marker branch from abae44b to 9cc40f6 Compare February 13, 2025 12:13

evanlinjin commented Feb 14, 2025

View reviewed changes

LagginTimes force-pushed the missing_marker branch 5 times, most recently from 98af6cd to 75adeee Compare February 16, 2025 05:41

evanlinjin commented Feb 16, 2025

View reviewed changes

crates/bitcoind_rpc/src/lib.rs Outdated Show resolved Hide resolved

evanlinjin commented Feb 16, 2025

View reviewed changes

crates/bitcoind_rpc/tests/test_emitter.rs Outdated Show resolved Hide resolved

evanlinjin commented Feb 16, 2025

View reviewed changes

example-crates/example_bitcoind_rpc_polling/src/main.rs Outdated Show resolved Hide resolved

This was referenced Feb 16, 2025

Make full-scan/sync flow easier to reason about. #1838

Open

Temporary PR: Evicted at #1839

Draft

evanlinjin commented Feb 16, 2025

View reviewed changes

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

ValuedMammal and others added 4 commits February 18, 2025 01:46

test(rpc): add test_expect_tx_evicted

7a97d4b

docs(chain): Update tx_graph module docs

eba8fd6

* Remove duplicate paragraphs about `ChangeSet`s. * Add "Canonicalization" section which expands on methods that require canonicalization and the associated data types used in the canonicalization algorithm.

refactor(rpc)!: update mempool interface and test code WIP

ec90121

LagginTimes force-pushed the missing_marker branch from 75adeee to ec90121 Compare February 17, 2025 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Introduce `evicted-at`/`last-evicted` 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

ValuedMammal Feb 1, 2025

evanlinjin Feb 2, 2025

oleonardolima Feb 3, 2025

LagginTimes Feb 4, 2025

evanlinjin Feb 5, 2025 •

edited

Loading

ValuedMammal Feb 1, 2025

evanlinjin Feb 2, 2025

ValuedMammal Feb 2, 2025

evanlinjin Feb 7, 2025

ValuedMammal Feb 8, 2025

evanlinjin Feb 2, 2025

evanlinjin Feb 2, 2025

oleonardolima left a comment

oleonardolima Feb 3, 2025

evanlinjin Feb 7, 2025

evanlinjin Feb 13, 2025

evanlinjin Feb 16, 2025


		env.mine_blocks(101, None)?;

		// Select a UTXO to use as an input for constructing our test transactions.

Introduce evicted-at/last-evicted timestamps #1811

Are you sure you want to change the base?

Introduce evicted-at/last-evicted 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

evanlinjin Feb 5, 2025 • edited Loading

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

Choose a reason for hiding this comment

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

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

evanlinjin commented Jan 24, 2025 •

edited by LagginTimes

Loading

evanlinjin Feb 5, 2025 •

edited

Loading