Skip to content

Introduce evicted-at/last-evicted timestamps #1839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 15, 2025

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Feb 16, 2025

Partially Fixes #1740.
Replaces #1765.
Replaces #1811.

Description

This PR allows the receiving structures (bdk_chain, bdk_wallet) to detect and evict incoming transactions that are double spent (cancelled).

We add a new field to TxUpdate (TxUpdate::evicted_ats), which in turn, updates the last_evicted timestamps that are tracked/persisted by TxGraph. This is similar to how TxUpdate::seen_ats update the last_seen timestamp in TxGraph. Transactions with a last_evicted timestamp higher than their last_seen timestamp are considered evicted.

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.

Notes to the reviewers

This PR does not fix #1740 for block-by-block chain source (such as bdk_bitcoind_rpc). This work is done in a separate PR (#1857).

Changelog notice

  • Add TxUpdate::evicted_ats which tracks transactions that have been replaced and are no longer present in mempool.
  • Change TxGraph to track last_evicted timestamps. This is when a transaction is 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.
  • Add SpkWithExpectedTxids in spk_client which keeps track of expected Txids for each spk.
  • Change bdk_electrum and bdk_esplora to understand SpkWithExpectedTxids.
  • Add SyncRequestBuilder::expected_txids_of_spk method which adds an association between txids and spks.

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

@evanlinjin evanlinjin force-pushed the evicted_at branch 5 times, most recently from 6dc26c4 to 0fcc097 Compare February 21, 2025 10:48
@LagginTimes LagginTimes force-pushed the evicted_at branch 2 times, most recently from 540a83f to 5107b3a Compare February 26, 2025 14:26
@evanlinjin evanlinjin force-pushed the evicted_at branch 6 times, most recently from 5718a3e to 07903a8 Compare February 28, 2025 03:34
@evanlinjin evanlinjin changed the title Temporary PR: Evicted at Introduce evicted-at/last-evicted timestamps Feb 28, 2025
@evanlinjin evanlinjin marked this pull request as ready for review February 28, 2025 04:00
@evanlinjin evanlinjin added bug Something isn't working api A breaking API change labels Feb 28, 2025
@notmandatory notmandatory moved this to Needs Review in BDK Wallet Feb 28, 2025
@evanlinjin evanlinjin requested a review from notmandatory March 2, 2025 06:13
@notmandatory notmandatory removed this from BDK Wallet Mar 4, 2025
This is a set of evicted txs from the mempool.
@evanlinjin evanlinjin force-pushed the evicted_at branch 2 times, most recently from 0f10b55 to 806a7d0 Compare March 14, 2025 00:40
@evanlinjin evanlinjin requested a review from nymius March 14, 2025 00:41
evanlinjin and others added 7 commits March 14, 2025 13:15
The evicted-at and last-evicted timestamp informs the `TxGraph` when the
transaction was last deemed as missing (evicted) from the mempool.

The canonicalization algorithm is changed to disregard transactions with
a last-evicted timestamp greater or equal to it's last-seen timestamp.
The exception is when we have a canonical descendant due to rules of
transitivity.

Update rusqlite_impl to persist `last_evicted`.

Also update docs:
* 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.
The spk history returned from Electrum should have these txs present.
Any missing tx will be considered evicted from the mempool.
* `TxGraph::try_list_expected_spk_txids`
* `TxGraph::list_expected_spk_txids`
* `IndexedTxGraph::try_list_expected_spk_txids`
* `IndexedTxGraph::list_expected_spk_txids`

Co-authored-by: valued mammal <[email protected]>
Co-authored-by: Wei Chen <[email protected]>
Also add `detect_receive_tx_cancel` test.
Also rm `miniscript` dependency.
Update ci to rm `miniscript/no-std` for `bdk_esplora`.

Co-authored-by: Wei Chen <[email protected]>
Methods `next_spk` and `iter_spks` are removed. These are superceded
with `next_spk_with_expected_txids` and `iter_spks_with_expected_txids`.
@evanlinjin
Copy link
Member Author

evanlinjin commented Mar 14, 2025

ut ACK 1eb0c32

Thanks for the fix and all the new docs, looks like a solid solution. Before (or after) this goes in it would be nice to have some confirmation from @ErikDeSmedt that it fixes #1740 for him (at least using electrum and esplora).

@notmandatory

The tests that we introduce in this PR are based on @ErikDeSmedt's tests (but we target for esplora/electrum in our PR here).

This PR does not fix evicted-tx-detection for bdk_bitcoind_rpc. That work is in a separate PR: #1857.

So I think it is okay to merge this without Erik's confirmation.

Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0a20724

Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last nit that I saw.

///
/// This is used to fill [`SyncRequestBuilder::expected_spk_txids`](bdk_core::spk_client::SyncRequestBuilder::expected_spk_txids).
///
/// The spk index range can be contrained with `range`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The spk index range can be contrained with `range`.
/// The spk index range can be constrained with `range`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how easy it would be to set up CI to check for spelling mistakes. Might save time.

@evanlinjin evanlinjin merged commit f0faff6 into bitcoindevkit:master Mar 15, 2025
22 of 23 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Chain Mar 15, 2025
@ValuedMammal
Copy link
Collaborator

ACK 0a20724

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Undetected double-spent
7 participants