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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 3 commits into from
+243 −6

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

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.

crates/chain/tests/test_indexed_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_indexed_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_indexed_tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_indexed_tx_graph.rs Show resolved Hide resolved
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
Draft
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buffrr buffrr buffrr left review comments

@evanlinjin evanlinjin evanlinjin requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@LagginTimes LagginTimes

Labels
bug Something isn't working
Projects
BDK
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Undetected double-spent
3 participants
@LagginTimes @evanlinjin @buffrr