Skip to content
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

Introduce missing-at/last-missing timestamps #1811

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Jan 24, 2025

Fixes #1740.

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.

SpkWithHistory is introduced in SpkClient to track expected Txids for each spk. During a sync, if any Txids from SpkWithHistory are not in the current history of an spk obtained from the chain source, those Txids are considered missing. Support for SpkWithHistory 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.
  • SpkWithHistory in SpkClient keeps track of expected Txids for each spk.
  • SpkWithHistory support added for bdk_electrum and bdk_esplora.
  • SyncRequestBuilder::expected_txs 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.
@evanlinjin evanlinjin changed the title Introduce MissingMarker Introduce missing-at/last-missing timestamps Jan 24, 2025
evanlinjin and others added 10 commits January 24, 2025 23:21
This is a set of txids missing from the mempool.
The missing-at and last-missing timestamp informs the `TxGraph` when the
transaction was last deemed as missing from the mempool (evicted).

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.
@notmandatory notmandatory added the bug Something isn't working label Jan 24, 2025
@notmandatory
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Undetected double-spent
3 participants