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

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

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

This is a set of txids evicted from the mempool.

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.

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.

crates/bitcoind_rpc/src/lib.rs

evanlinjin · 2025-02-21T12:54:33Z

crates/bitcoind_rpc/src/lib.rs

+    /// Given an iterator of expected [`Txid`]s, return those that are missing from the mempool.
+    pub fn evicted_txids(
+        &self,
+        expected_unconfirmed_txids: impl IntoIterator<Item = Txid>,


For spk-based syncing we want to use all canonical txids (including confirmed) to determine evicted txs. The reason is because spk-based APIs returns both confirmed and unconfirmed under a given spk and that a missing previously-confirmed tx is also evicted from the mempool.

For rpc-based, we obviously want to only input unconfirmed txs as block (confirmed events) are handled separately...

We can add an include_confirmed: bool param on TxGraph/IndexedTxGraph methods that lists expected txs... but that does feel seen like it's easy to screw up.

Another idea is to compare the MempoolEvent txids to the previous MempoolEvent txids and the difference that does not end up in a block is evicted... However, when we initiate the Emitter, we still need the wallet state of unconfirmed txs as we no longer have all the data from the last MempoolEvent emission.

Maybe it's best to combine those two ideas.

This way we don't have to get something from our receiving structures (expected_unconfirmed_txids) before applying the update.

@notmandatory

800f358 feat!: Improve spk-based syncing flow (志宇) ee52745 feat(core)!: Make `TxUpdate` non-exhaustive (志宇) Pull request description: ### Description #1811 introduces the `evicted-at` concept. While adding this to `TxUpdate`, I realized there were some shortcomings in our full-scan & sync flow: * Chain sources that use `TxUpdate` to convey updates cannot choose to add transactions without a `seen_at` timestamp as the `TxGraph::apply_update_at` logic adds this timestamp for all unanchored txs if the `seen_at` parameter is specified. Purposefully adding uncanonical txs is useful for wallets that want to know about replaced txs. * The `TxGraph::apply_update_at` logic is hard to reason about. `TxUpdate::seen_ats` already has the `seen_at` timestamp per tx, but `apply_update_at()` also takes in a `seen_at` timestamp`. * `TxUpdate::seen_ats` currently forces us to only have one `seen-at` per tx as it's a map of `txid -> seen_at`. However, in the future we want to add a `first-seen` timestamp to `TxGraph` which is basically the earliest `seen-at` timestamp introduced so we may want to have multiple `seen_at`s per tx. The other issue is if we merge `TxUpdate`s, we will loose data. I.e. we can only keep the `last_seen` or the `first_seen`. These problems were exacerbated when introducing `evicted-at`. In the old design, the chain-source has no concept of sync time (as sync time was introduced after-the-fact when applying the `TxUpdate`). Therefore the introduced `TxUpdate::evicted` field was a `HashSet<Txid>` (no timestamps) and relied on the `TxGraph::apply_upate_at` logic to introduce the `evicted-at` timestamp. All this makes the sync logic harder to understand. What happens if the `seen_at` input is `None`? What happens if updates were applied out of order? What happens when we merge `TxUpdates` before applying? The following changes are made in this PR to simplify the sync/full-scan logic to be easier to understand and robust: * The `sync_time` is added to the `SyncRequest` and `FullScanRequest`. `sync_time` is mandatory and is added as an input of `builder()`. If the `std` feature is enabled, the `builder_now()` is available which uses the current timestamp. The chain source is now responsible to add this `sync_time` timestamp as `seen_at` for mempool txs. Non-canonical and non-anchored txs can be added without the `seen_at` timestamp. * `TxUpdate::seen_ats` is now a `HashSet` of `(Txid, u64)`. This allows multiple `seen_at`s per tx. This is also a much easier to use API as the chain source can just insert into this `HashSet` without checking previous data. * `TxGraph::apply_update_at` is removed as we no longer needs to introduce a fallback `seen_at` timestamp after-the-fact. `TxGraph::apply_update` is no longer `std`-only and the logic of applying updates is greatly simplified. Additionally, `TxUpdate` is updated to be `#[non_exhaustive]` for backwards-compatibility protection. ### Notes to the reviewers This is based on #1837. Merge that first. These are breaking changes to `bdk_core`. It needs to be breaking to properly fix all the issues. As mentioned by @notmandatory, `bdk_wallet` changes will be added to a new PR once the new `bdk_wallet` repo is ready. But the PR won't be merged until we're ready for a `bdk_wallet` 2.0. ### Changelog notice * Change `FullScanRequest::builder` and `SyncRequest::builder` methods to depend on `feature = "std"`. This is because requests now have a `start_time`, instead of specifying a `seen_at` when applying the update. * Add `FullScanRequest::builder_at` and `SyncRequest::builder_at` methods which are the non-std version of the `..Request::builder` methods. * Change `TxUpdate::seen_ats` field to be a `HashSet` of `(Txid, u64)`. This allows a single update to have multiple `seen_at`s per tx. * Change `TxUpdate` to be `non-exhaustive`. * Remove `apply_update_at` as we no longer need to apply with a timestamp after-the-fact. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: ~* [ ] I've added tests for the new feature~ No tests needed as it's more of a refactor. * [x] I've added docs for the new feature ACKs for top commit: notmandatory: utACK 800f358 Tree-SHA512: 85af8452ac60c4a8087962403fd10c5c67592d3711f7665ae09a2d9c48a868583a41e54f28d639e47bd264ccf95d9254efc8d0d6248c8bcc9343c8290502e061

evanlinjin · 2025-02-28T04:00:58Z

Replaced by #1839

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

This was referenced Feb 16, 2025

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

Merged

Introduce evicted-at/last-evicted timestamps #1839

Open

evanlinjin commented Feb 16, 2025

View reviewed changes

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

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

evanlinjin and others added 15 commits February 21, 2025 00:40

feat(core)!: Make TxUpdate non-exhaustive

b1461f2

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

feat(core): Add TxUpdate::evicted

ec25b10

This is a set of txids evicted from the mempool.

feat(core): add SpkWithExpectedTxids support for SpkClient

40ceef1

feat(chain): Update rusqlite_impl to persist last_evicted

5b7f851

feat(electrum): add SpkWithExpectedTxids support

b3d17ad

feat(esplora): add SpkWithExpectedTxids support

241dd7d

feat(chain): Add SyncRequestExt::check_unconfirmed_statuses

5e2f242

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

4b979c6

test(esplora): detect_receive_tx_cancel

3be6c2b

feat(chain): add method iter_spks_with_expected_txids

5f6dfe7

test(rpc): add test_expect_tx_evicted

ef45337

docs(chain): Update tx_graph module docs

dc452a1

* 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.

LagginTimes force-pushed the missing_marker branch from ec90121 to db61c76 Compare February 20, 2025 16:48

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

66fa256

LagginTimes force-pushed the missing_marker branch from db61c76 to 66fa256 Compare February 20, 2025 16:55

LagginTimes marked this pull request as ready for review February 20, 2025 16:56

evanlinjin commented Feb 21, 2025

View reviewed changes

LLFourn mentioned this pull request Feb 24, 2025

refactor(chain)! removed IndexedTxGraph 🗑 #1510

Open

4 tasks

LagginTimes mentioned this pull request Feb 26, 2025

feat(rpc)!: Update Emitter::mempool to support evicted_at #1857

Draft

8 tasks

evanlinjin closed this Feb 28, 2025

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 16, 2025

evanlinjin Feb 21, 2025

evanlinjin Feb 21, 2025

evanlinjin commented Feb 28, 2025

Introduce evicted-at/last-evicted timestamps #1811

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

Choose a reason for hiding this comment

evanlinjin commented Feb 28, 2025

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