Skip to content

docs(wallet): expand docs for apply_evicted_txs #270

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

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

Conversation

ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Jun 23, 2025

Description

Expand the API docs for Wallet::apply_evicted_txs.

This can help to fix #254 by having a section explaining the logic of mempool evictions.

Changelog notice

Checklists

All Submissions:

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

@coveralls
Copy link

coveralls commented Jun 23, 2025

Pull Request Test Coverage Report for Build 16274314345

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 44 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 84.69%

Files with Coverage Reduction New Missed Lines %
wallet/src/wallet/persisted.rs 44 36.16%
Totals Coverage Status
Change from base Build 16052072493: -0.04%
Covered Lines: 6577
Relevant Lines: 7766

💛 - Coveralls

@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet Jul 3, 2025
@ValuedMammal ValuedMammal added this to the Wallet 2.1.0 milestone Jul 3, 2025
@ValuedMammal ValuedMammal force-pushed the doc/apply-evicted-txs branch from 0121e0f to 9284767 Compare July 7, 2025 20:30
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Jul 7, 2025
@ValuedMammal ValuedMammal added the documentation Improvements or additions to documentation label Jul 7, 2025
@ValuedMammal ValuedMammal self-assigned this Jul 7, 2025
@ValuedMammal ValuedMammal marked this pull request as ready for review July 7, 2025 20:31
Copy link
Contributor

@tvpeter tvpeter left a comment

Choose a reason for hiding this comment

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

Thank you Mammal for the excellent documentation.

ACK 9284767

I left a suggestion for including an example if it won't become too lengthy.

/// influence the wallet's canonicalization logic.
///
/// [`transactions`]: Wallet::transactions
/// [`start_sync_with_revealed_spks`]: Wallet::start_sync_with_revealed_spks
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be great to also include an example here:

Suggested change
/// [`start_sync_with_revealed_spks`]: Wallet::start_sync_with_revealed_spks
/// [`start_sync_with_revealed_spks`]: Wallet::start_sync_with_revealed_spks
/// ## Example
///
/// ```rust
/// use bdk_wallet::{Wallet, bitcoin::Txid};
/// use std::collections::HashSet;
///
/// let unconfirmed_txids: HashSet<Txid> = wallet
/// .transactions()
/// .filter(|tx| tx.chain_position.is_unconfirmed())
/// .map(|tx| tx.tx_node.txid)
/// .collect();
///
/// let mut evicted_txs = Vec::new();
/// for txid in unconfirmed_txids {
/// let tx_node = wallet.tx_graph().full_txs().find(|tx| tx.txid == txid);
/// let wallet_tx = wallet.get_tx(txid);
/// let is_evicted = wallet_tx.map_or(true, |tx| {
/// !tx.chain_position.is_unconfirmed() && !tx.chain_position.is_confirmed()
/// });
/// if is_evicted {
/// let last_seen = tx_node.map_or(0, |tx| tx.last_seen.unwrap_or(0));
/// evicted_txs.push((txid, last_seen));
/// }
/// }
///
/// if !evicted_txs.is_empty() {
/// wallet.apply_evicted_txs(evicted_txs)?;
/// wallet.persist(&mut db)?;
/// }

///
/// ## Notes
///
/// - This function is particularly useful when syncing with a blockchain backend that provides
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me if or when a user needs to use this function. Don't the bdk_* chain clients already evict tx when they notice it missing from the mempool?

@ValuedMammal
Copy link
Collaborator Author

@tvpeter @notmandatory One usage example that I know of is in the example_wallet_rpc.

@ValuedMammal ValuedMammal moved this from Needs Review to In Progress in BDK Wallet Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

docs: Have a section explaining mempool eviction detection.
4 participants