Skip to content

feat(wallet): WalletTx now supports non-canonical txs#480

Open
noahjoeris wants to merge 2 commits into
bitcoindevkit:masterfrom
noahjoeris:feat/wallet-tx
Open

feat(wallet): WalletTx now supports non-canonical txs#480
noahjoeris wants to merge 2 commits into
bitcoindevkit:masterfrom
noahjoeris:feat/wallet-tx

Conversation

@noahjoeris
Copy link
Copy Markdown

@noahjoeris noahjoeris commented May 9, 2026

Description

WalletTx was an alias for CanonicalTx, so wallet.transactions() and wallet.get_tx(txid) only returned canonical wallet-relevant txs. Evicted/replaced txs disappeared tho still wallet relevant.

This PR changes WalletTx to TransactionInfo and makes it a struct that represents any wallet-relevant tx:

/// A wallet-relevant transaction and its metadata.
pub struct TransactionInfo<'a> {
    /// Wallet-specific amounts, fees, and canonicality.
    pub details: TxDetails,
    /// Graph metadata such as anchors, first_seen, and last_seen.
    pub tx_node: TxNode<'a, Arc<Transaction>, ConfirmationBlockTime>,
    /// Latest backend observation that this tx was absent from the mempool.
    pub last_evicted: Option<u64>,
    /// Direct conflicts spending the same inputs.
    pub conflicts: Vec<Txid>,
}

/// The canonicality of a wallet-relevant transaction.
pub enum TxCanonicality {
    /// The transaction is currently canonical.
    Canonical(ChainPosition<ConfirmationBlockTime>),
    /// The transaction is currently not canonical.
    NonCanonical,
}

Canonical status lives in details.canonicality.

Fixes #295

Supported use cases

  • Keep dropped txs visible in wallet history.
  • Show whether an evicted tx has conflicts.
  • Let users decide when it is safe to forget an old dropped tx.
  • Give users enough information to warn before accidentally paying twice.

Surface now including non-canonical txs

  • TransactionInfo, TxCanonicality, TxDetails, transactions(), get_tx()

Notes to the reviewers

  • Performance of transactions() takes a small hit. Worth confirming whether it's an issue in practice.
  • Still need to add tests.

Changelog notice

  • Changed: WalletTxTransactionInfo struct with details, tx_node, last_evicted, conflicts; WalletTxStatusTxCanonicality.
  • Changed: transactions() / get_tx() also return non-canonical wallet-relevant txs.
  • Changed: TxDetails::chain_positioncanonicality.
  • Removed: Wallet::tx_details(txid), use wallet.get_tx(txid).details.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran just p before pushing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • This pull request breaks the existing API

@codecov
Copy link
Copy Markdown

codecov Bot commented May 9, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.29%. Comparing base (4b612f5) to head (fe447ad).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/wallet/mod.rs 78.40% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
- Coverage   80.30%   80.29%   -0.02%     
==========================================
  Files          24       24              
  Lines        5417     5566     +149     
  Branches      245      246       +1     
==========================================
+ Hits         4350     4469     +119     
- Misses        989     1020      +31     
+ Partials       78       77       -1     
Flag Coverage Δ
rust 80.29% <80.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/wallet/mod.rs Outdated
Comment thread src/wallet/mod.rs Outdated
@ValuedMammal
Copy link
Copy Markdown
Collaborator

Concept ACK

@noahjoeris
Copy link
Copy Markdown
Author

After some ideas by ValuedMammal I simplified the model a bit:

  • Renamed WalletTxTransactionInfo, WalletTxStatusTxCanonicality.
  • Replaced canonical_blockers() with a direct-only conflicts: Vec<Txid> field; moved last_evicted to a top-level field on TransactionInfo.

PR description updated accordingly.

@ValuedMammal
Copy link
Copy Markdown
Collaborator

canonical_blockers was a nice idea because it could tell you if a tx is indirectly conflicted through one of its ancestors, even if it has no direct conflicts of its own. How can we capture that information so it's clear why a wallet tx isn't canonical? Maybe include the tx's unconfirmed ancestor and descendant sets somewhere so we can trace its ancestry. But as mentioned before we may not want to bake a lot of txgraph traversal into every wallet.transactions() call

@noahjoeris
Copy link
Copy Markdown
Author

Yep I agree. I added canonical_blockers() back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Preserve evicted outgoing transactions in wallet / Improve "WalletTx"

3 participants