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

transactions method should only return relevant transactions #1779

Merged
merged 2 commits into from
Dec 18, 2024
Merged

transactions method should only return relevant transactions #1779

merged 2 commits into from
Dec 18, 2024

Conversation

evanlinjin
Copy link
Member

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:

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

@evanlinjin evanlinjin added the bug Something isn't working label Dec 17, 2024
@evanlinjin evanlinjin added this to the 1.0.0-beta milestone Dec 17, 2024
@evanlinjin evanlinjin self-assigned this Dec 17, 2024
@evanlinjin evanlinjin force-pushed the fix/wallet-transactions-should-only-be-relevant branch from 2df73bf to 3fe2bb0 Compare December 17, 2024 06:38
oleonardolima
oleonardolima reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK 3fe2bb0

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Dec 17, 2024 •
edited
Loading

Concept ACK! Reminds me of #1239, and should probably close it once it gets in.

Edit: just noticed Evan you'd already linked the issue up. Cheers!

@notmandatory
Copy link
Member

notmandatory commented Dec 17, 2024 •
edited
Loading

I added 75fae3e to verify only relevant transactions are returned. If there's a better way to test this feel free to replace or change this commit.

I also re-based on master to pickup the CI fixes in #1776 and fixed typos that @oleonardolima found.

@notmandatory notmandatory force-pushed the fix/wallet-transactions-should-only-be-relevant branch 3 times, most recently from ca38f17 to 488c722 Compare December 17, 2024 22:22
notmandatory
notmandatory approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@notmandatory notmandatory 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.

tACK 75fae3e

@notmandatory notmandatory force-pushed the fix/wallet-transactions-should-only-be-relevant branch from 488c722 to 75fae3e Compare December 18, 2024 01:47
@oleonardolima
Copy link
Contributor

oleonardolima commented Dec 18, 2024 •
edited
Loading

[...] If there's a better way to test this feel free to replace or change this commit.

I wonder if another thorough testing approach would be explicitly creating a new tx sending from A -> B, RBF it, and see that it's not being returned as relevant now, or that would still be relevant ? Does that make sense, or am I missing a point here ? (I guess same is being achieved in a simpler approach)

oleonardolima
oleonardolima reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

tACK 75fae3e

@luisschwab
Copy link
Contributor

oleonardolima: wonder if another thorough testing approach would be explicitly creating a new tx sending from A -> B, RBF it, and see that it's not being returned as relevant now, or that would still be relevant ?

IMO, a transaction T and it's replacement T' are both relevant, until T' get's confirmed to the chain, then only T' is relevant. That's what I expect after reading this doc:

/// Iterate over relevant and canonical transactions in the wallet.
///
/// A transaction is relevant when it spends from or spends to at least one tracked output. A
/// transaction is canonical when it is confirmed in the best chain, or does not conflict
/// with any transaction confirmed in the best chain.

@notmandatory
Copy link
Member

I wonder if another thorough testing approach would be explicitly creating a new tx sending from A -> B, RBF it, and see that it's not being returned as relevant now, or that would still be relevant ? Does that make sense, or am I missing a point here ? (I guess same is being achieved in a simpler approach)

The scenario I had in mind is when we have a new version of electrum or esplora that also retrieves transactions that have double spent a utxo that we received in an unconfirmed tx. Those "foreign" tx don't have any scripts that are relevant to our wallet but still need to be in our wallet to invalidate our unconfirmed (double spent) tx.

My test doesn't repro all the details of this scenario but at least shows that a not relevant tx can be added to the tx_graph and will be excluded from Wallet::transactions.

@notmandatory
Copy link
Member

IMO, a transaction T and it's replacement T' are both relevant, until T' get's confirmed to the chain, then only T' is relevant. That's what I expect after reading this doc:

In this RBF scenario I think T and T' will both still be relevant since they both spend to or from a tracked SPK. But after T' is confirmed T is no longer canonical since it is not confirmed in the best chain.

@luisschwab
Copy link
Contributor

Is TxGraph monotonic? If so I agree that T remains relevant and T' becomes cannonical.

@luisschwab
Copy link
Contributor

tACK 75fae3e

Would be good to add a test fot the RBF scenario as well.

ValuedMammal
ValuedMammal approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 75fae3e

crates/wallet/src/wallet/mod.rs Show resolved Hide resolved
crates/wallet/tests/wallet.rs Show resolved Hide resolved
crates/wallet/tests/wallet.rs Show resolved Hide resolved
crates/wallet/src/wallet/mod.rs Show resolved Hide resolved
@notmandatory notmandatory merged commit 3e01d56 into bitcoindevkit:master Dec 18, 2024
21 checks passed
@oleonardolima
Copy link
Contributor

[...] Those "foreign" tx don't have any scripts that are relevant to our wallet but still need to be in our wallet to invalidate our unconfirmed (double spent) tx.

My test doesn't repro all the details of this scenario but at least shows that a not relevant tx can be added to the tx_graph and will be excluded from Wallet::transactions.

I had this scenario in mind when writing RBF (e.g RBF'ed by/to an external wallet/spk). Thanks, it's clearer now.

This was referenced Dec 19, 2024
Closed
Merged
ValuedMammal added a commit that referenced this pull request Dec 23, 2024
@ValuedMammal
Merge #1785: test(wallet): small cleanups to test_wallet_transactions…
a208144 
…_relevant

6346536 test(wallet): small cleanups to test_wallet_transactions_relevant (Steve Myers)

Pull request description:

  ### Description

  This cleans up the `test_wallet_transactions_relevant` test based on suggestions that didn't make it into #1779.

  ### Notes to the reviewers

  Also included a small use cleanup in wallet/mod.

  ### Changelog notice

  None.

  ### 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:
    ACK 6346536
  ValuedMammal:
    ACK 6346536

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

@oleonardolima oleonardolima oleonardolima left review comments

@notmandatory notmandatory notmandatory approved these changes

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@evanlinjin evanlinjin

Labels
bug Something isn't working
Projects
BDK
Archived in project
Milestone
1.0.0-beta
Development

Successfully merging this pull request may close these issues.

The fact that the Wallet.transactions() API can return more than wallet-related txs is not explicit
6 participants
@evanlinjin @thunderbiscuit @notmandatory @oleonardolima @luisschwab @ValuedMammal