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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Dec 9, 2024 •
edited
Loading

Fixes #1740.

Description

This PR addresses a potential double-spending issue by implementing the following changes:

Redefine Transaction Relevancy in IndexedTxGraph

Transactions are now considered relevant if they:

  • Reference the wallet's spks in their inputs/outputs.
  • Share inputs with any transaction already in the TxGraph.

Include Unconfirmed Spends in Chain Sync Requests

  • Unconfirmed outputs can now be provided via SyncRequestExt::unconfirmed_spends, which enables the chain source to detect replaced or canceled transactions.
  • Deprecated Wallet::start_sync_with_revealed_spks in favor of start_sync, which supports unconfirmed spends.

Notes to the reviewers

TODO:

  • Add esplora test.

Changelog notice

  • Redefine what a relevant transaction is in IndexedTxGraph.
  • Providing unconfirmed spends in chain sync requests, enabling detection of replaced or canceled transactions.
  • Deprecated Wallet::start_sync_with_revealed_spks in favor of the more comprehensive Wallet::start_sync.

Checklists

All Submissions:

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

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

@LagginTimes LagginTimes added the bug Something isn't working label Dec 9, 2024
@LagginTimes LagginTimes self-assigned this Dec 9, 2024
@LagginTimes LagginTimes marked this pull request as draft December 9, 2024 15:13
@LagginTimes LagginTimes force-pushed the double_spend branch 3 times, most recently from 0dc826d to 43bca86 Compare December 12, 2024 10:51
@LagginTimes LagginTimes marked this pull request as ready for review December 13, 2024 05:42
buffrr
buffrr reviewed Dec 14, 2024
View reviewed changes
Copy link

@buffrr buffrr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This behaves as expected! However, I think including the conflicting transaction in the wallet's Tx graph may create unnecessary noise/confusion. Since these conflicts don't represent actual wallet activity.

Perhaps irrelevant transactions could be filtered out either directly in list_canonical_txs or in the wallet crate. Alternatively, marking the existing wallet tx as conflicting without inserting the external conflicting transactions into the graph.

@evanlinjin
Copy link
Member

Perhaps irrelevant transactions could be filtered out either directly in list_canonical_txs or in the wallet crate. Alternatively, marking the existing wallet tx as conflicting without inserting the external conflicting transactions into the graph.

That's a good point. It's a one line change to have Wallet::transactions filter out irrelevant txs. For bdk_chain, I think having docs should be sufficient?

@evanlinjin evanlinjin mentioned this pull request Dec 17, 2024
Merged
3 tasks
evanlinjin
evanlinjin requested changes Dec 17, 2024
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ConceptACK

I'm requesting the following changes:

  • Better clarity with the tests.
  • Update docs of the methods that are changed (since we changed the definition of what is relevant).

I expect a follow-up PR which tests against a block-by-block chain source (bdk_bitcoind_rpc). I think this test can exist in bdk_bitcoind_rpc.

notmandatory added a commit that referenced this pull request Dec 18, 2024
@notmandatory
Merge #1779: transactions method should only return relevant transa…
3e01d56 
…ctions

75fae3e test(wallet): verify Wallet::transactions method only returns relevant txs (Steve Myers)
3e1fd2b fix(wallet): `transactions` method should only return relevant txs (志宇)

Pull request description:

  Fixes #1239

  ### Description

  Currently the behavior of `Wallet::transactions` is not well defined and unintuitive.

  The approach taken in this PR is to make `Wallet::transactions` return what most wallets would like the caller to see (i.e. transactions that are part of the canonical history and spend from/to a tracked spk). A.k.a make the method more restrictive.

  Documentation is updated to refer the caller to the underlying `bdk_chain` structures for any over usecase.

  After #1765 gets merged, the behavior of `Wallet::transactions` will become even more unintuitive. Refer to #1765 (review).

  ### Notes to the reviewers

  **Why not have multiple methods in `Wallet` that return different sets of transactions?**

  I think it's better to only provide common usecase histories from `Wallet` and I can only think of one.

  ### Changelog notice

  * Change `Wallet::transactions` to only include "relevant" transactions.

  ### 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

ACKs for top commit:
  luisschwab:
    tACK 75fae3e
  notmandatory:
    tACK 75fae3e
  oleonardolima:
    tACK 75fae3e
  ValuedMammal:
    ACK 75fae3e

Tree-SHA512: abf159e0c5d44842d7e0fc5ebc6829d34646fbc45d07bb145ce327f368db0e571ab7c5731a12e63258dfc74abb9d4ff1b841842de8341e0f21b5cbb2becc5e5f
@buffrr buffrr mentioned this pull request Dec 18, 2024
Closed
3 tasks
@evanlinjin
Copy link
Member

@LagginTimes I rebased and forced push. The last commit I added provides unconfirmed spends to spk-based chain sources. This should be enough to fully fix #1740 but we need to test.

I expect a detect_receive_tx_cancel test for all chain sources (defined in their respective chain source crates). Also the PR description/title needs to be updated.

@LagginTimes LagginTimes changed the title fix(chain): redefine relevant txs in IndexedTxGraph fix(chain): updating tx relevancy and unconfirmed spends Dec 24, 2024
@evanlinjin
Copy link
Member

evanlinjin commented Dec 24, 2024 •
edited
Loading

The title needs to highlight what is being fixed, not what is changed to do the fix.

Suggestion: Detect incoming transaction being replaced/canceled.

@LagginTimes LagginTimes changed the title fix(chain): updating tx relevancy and unconfirmed spends fix(chain): detect incoming transaction being replaced/canceled. Dec 24, 2024
@LagginTimes LagginTimes changed the title fix(chain): detect incoming transaction being replaced/canceled. fix(chain): detect incoming transaction being replaced/canceled Dec 24, 2024
LagginTimes and others added 3 commits December 24, 2024 22:07
@LagginTimes
fix(chain): redefine relevant txs in IndexedTxGraph
9caa5e0 
A transaction's relevancy was originally only determined by the spks
referenced by the tx's inputs and outputs. A new rule is added where if
a tx shares inputs with anything contained in TxGraph, then it should
also be considered relevant. This fixes a potential double spending
problem.
@LagginTimes
test(chain): batch_insert_relevant inserts conflicting tx
82c8e33
@evanlinjin @LagginTimes
fix(chain,wallet): Provide unconfirmed spends to chain src sync request
58a6704 
Unconfirmed outputs can now be easily provided to the `SyncRequest` via
`SyncRequestExt::unconfirmed_spends`. This allows the chain src to
detect receiving txs being replaced/cancelled.

`Wallet::start_sync_with_revealed_spks` has been deprecated in favor of
`start_sync` which included unconfirmed spends.
@ValuedMammal
Copy link
Contributor

Since we can reliably determine the "unconfirmed outpoints" of relevant txs, would it not suffice to have a method apply_block_relevant_spends that applies both relevant txs along with any spends given a list of semi-relevant outpoints. This wouldn't change the definition of relevant and the other batch-insert methods could continue to do what they say.

@LagginTimes LagginTimes marked this pull request as draft January 17, 2025 10:29
@evanlinjin evanlinjin mentioned this pull request Jan 23, 2025
Open
@LagginTimes LagginTimes marked this pull request as ready for review January 23, 2025 19:11
@evanlinjin
Copy link
Member

After reviewing comments from @buffrr and @ValuedMammal, and learning about perspectives from @LLFourn and Yuval Kogman, I realized the approach in this PR is wrong.

We failed to take into consideration that the malicious replacement can be replacing an ancestor/parent of the incoming transaction. To fix the problem with our current approach of including the conflicting transaction(s), we will need to traverse all unconfirmed ancestors (and find their conflicts). This is obviously a DoS vector and very bandwidth-intensive in general.

The better solution would be to introduce a MissingMarker which informs about the latest timestamp that a transaction was deemed "missing" from the mempool.

When requesting a sync from spk-based chain sources, we also include a list of "expected txids" per spk. Anything missing from the list will result in a MissingMarker being added to the SyncResponse.

For block-based chain sources, can send the txid list of unconfirmed to it and it can return those that are missing from mempool.

@evanlinjin evanlinjin mentioned this pull request Jan 24, 2025
Closed
8 tasks
@LagginTimes
Copy link
Contributor Author

Closed, replaced by #1811.

@LagginTimes LagginTimes closed this Feb 6, 2025
@evanlinjin evanlinjin mentioned this pull request Feb 28, 2025
Merged
8 tasks
evanlinjin added a commit that referenced this pull request Mar 15, 2025
@evanlinjin
Merge #1839: Introduce evicted-at/last-evicted timestamps
f0faff6 
0a20724 feat(examples): Update example crates to use `expected_spk_txids` (志宇)
1f8fc17 feat(core)!: Remove redundant `SyncRequest` methods (志宇)
f42d5a8 feat(esplora): Handle spks with expected txids (志宇)
3ab4994 feat(electrum): Handle spks with expected txids (志宇)
64e4100 feat(chain): Add `TxGraph` methods that handle expected spk txids (志宇)
b38569f feat(core): Add expected txids to `SyncRequest` spks (Wei Chen)
a2dfcb9 feat(chain)!: Change `TxGraph` to understand evicted-at & last-evicted (志宇)
ae0336b feat(core): Add `TxUpdate::evicted_ats` (志宇)

Pull request description:

  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 `Txid`s for each `spk`. During a sync, if any `Txid`s from `SpkWithExpectedTxids` are not in the current history of an `spk` obtained from the chain source, those `Txid`s 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 `Txid`s for each `spk`.
  * Change `bdk_electrum` and `bdk_esplora` to understand `SpkWithExpectedTxids`.
  * Add `SyncRequestBuilder::expected_txids_of_spk` method which adds an association between `txid`s and `spk`s.

  ### 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:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

  #### Bugfixes:

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

ACKs for top commit:
  LLFourn:
    ACK 0a20724

Tree-SHA512: 29ef964e4597aa9f4cf02e9997b0d17cb91ec2f0f1187b0e9ade3709636b873c2a7cbe803facbc4686a3050a2abeb3e9cc40f9308f8cded9c9353734dcc5755b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlinjin evanlinjin evanlinjin requested changes

@buffrr buffrr buffrr left review comments

Assignees

@LagginTimes LagginTimes

Labels
bug Something isn't working
Projects
BDK Wallet
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Undetected double-spent
4 participants
@LagginTimes @evanlinjin @ValuedMammal @buffrr