Introduce `IntentTracker` #257

evanlinjin · 2025-06-06T09:55:22Z

Fixes #166
Fixes #40
Replaces #220

Description

Allows callers to spend from unbroadcasted transactions.

Notes to the reviewers

I think I may have done some overthinking for the BroadcastQueue implementation. This is the current implementation:

Wallet::add_tx_to_broadcast_queue will also remove conflicts (of the tx being inserted) from the broadcast queue.
Wallet::remove_tx_from_broadcast_queue will also remove descendants of the tx being removed.

However, I’m not convinced this feature is necessary, and it could lead to inconsistent behavior if callers sometimes use the BroadcastQueue, bypass it to broadcast transactions directly, or if multiple instances of the same wallet broadcast concurrently. In such cases—when intermediate transactions are missing from the queue—the logic described above will fail.

There is an argument for RBF, however, why would you need to RBF unbroadcasted transactions? It's better to empty the queue and start again.

Changelog notice

Checklists

To Get Out of Draft Status:

Have a section in the struct-level (Wallet) docs that explains the broadcast queue.
Better docs for each new method added.
Example: Wallet with single UTXO. Create x number of transactions sequentially. Broadcast all in one go. Sync.
Test persistence (sqlite).
More tests.

To Get This Merged:

User feedback (cc. @tnull ).

All Submissions:

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

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 · 2025-06-06T10:23:06Z

Pull Request Test Coverage Report for Build 15943002368

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

213 of 632 (33.7%) changed or added relevant lines in 5 files are covered.
14 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-4.9%) to 80.602%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
wallet/src/wallet/tx_builder.rs	2	10	20.0%
wallet/src/wallet/changeset.rs	25	41	60.98%
wallet/src/wallet/mod.rs	135	222	60.81%
wallet/src/wallet/intent_tracker.rs	49	357	13.73%

Files with Coverage Reduction	New Missed Lines	%
wallet/src/descriptor/dsl.rs	1	95.34%
wallet/src/wallet/changeset.rs	2	79.44%
wallet/src/descriptor/policy.rs	3	79.07%
wallet/src/descriptor/template.rs	4	98.04%
wallet/src/wallet/mod.rs	4	78.06%

Totals
Change from base Build 15476130196:	-4.9%
Covered Lines:	6644
Relevant Lines:	8243

💛 - Coveralls

If the caller wishes to avoid spending unbroadcasted UTXOs without double-spending any of the unbroadcasted transactions.

nymius · 2025-06-08T21:45:04Z

wallet/src/types.rs

@@ -73,6 +73,8 @@ pub struct LocalOutput {
    pub derivation_index: u32,
    /// The position of the output in the blockchain.
    pub chain_position: ChainPosition<ConfirmationBlockTime>,
+    /// Whether this output exists in a transaction that is yet to be broadcasted.
+    pub needs_broadcast: bool,


Maybe an enum state field with something like: UNSPENT, ON_QUEUE, SPENT, BROADCASTED will avoid keep adding new boolean fields here, and provide a better path for update on future occasions, taking advantage of non exhaustive patters. is_spent could be marked for deprecation and be used along the new field in the meantime.

I like this idea if done as a non-exhaustive enum to help reduce future API breaking changes. If we include a "LOCKED" variant could this also support #259?

nymius

However, I’m not convinced this feature is necessary, and it could lead to inconsistent behavior if callers sometimes use the BroadcastQueue, bypass it to broadcast transactions directly, or if multiple instances of the same wallet broadcast concurrently. In such cases—when intermediate transactions are missing from the queue—the logic described above will fail.

Can we enforce or support BroadcastQueue as the only way to broadcast transactions in bdk_wallet?
A user bypassing this mechanism should be considered? Are there reasons to not doing it?
Why would you keep broadcasting tx outside of the queue when you have an unbroadcasted tx in the queue?

I like the approach, and think is easy to reason about. Maybe we could leave the door open to implement other broadcast policies.
IMHO, the BroadcastQueue "profile" should ensure internal consistency, so I don't think is over engineered.

Why would you need to RBF unbroadcasted transactions?

Not a use case that I've needed, but maybe share multiple conflicting transactions offline looking for fee optimization in different scenarios.

thunderbiscuit · 2025-06-09T18:23:03Z

Concept ACK. I like the idea of the queue.

I took a look at the diff and here are some thoughts/questions, pardon me if some of them would have been answered by doing a code deep dive, I just know you wanted early feedback so decided to get moving on it sooner than later.

Simple is good in my mind. If one of the requirements of the queue is that it's always internally valid and could in theory be broadcast all at once in one go, that's an easier mental model than allowing conflicts in the queue.
If the queue can actually have conflicts, it's less of a queue and more of a "bag" of transactions. Again less easy to reason about, and now the naming is misleading from the point of view of the users (I mean it's not that bad, I just mean it's not as neat/pure)
I like that the queue purges itself automatically on syncs.
I am potentially drawn to the idea from @nymius that all transactions could need to go in the queue first to then be broadcast. I wonder if that's an elegant way to force clean setups and handle the fact that a ton of wallets probably don't do costly sync every time they build transactions. That way the queue would always be aware of what has been broadcast. Does that complicate things too much? It would just be important that the library not have any footguns that would for example have you forget about a tx in the queue, persist it, then weeks later you just do Wallet::broadcast_queue and bam you just sent more than you wanted.

tnull

Thanks for tackling this, took a first look.

I think I may have done some overthinking for the BroadcastQueue implementation. This is the current implementation:

Do we know how Core handles these things?

Also, when do we expect this queue to be processed? Would this happen manually or automatically in intervals?

tnull · 2025-06-10T10:17:57Z

wallet/src/wallet/broadcast_queue.rs

+    queue: VecDeque<Txid>,
+
+    /// Enforces that we do not have duplicates in `queue`.
+    dedup: HashSet<Txid>,


I wonder if it's worth having this separate set? How many unbroadcasted transactions are we expecting at any given time? Maybe it would just be quicker to simply iterate over the queue itself, also saving the heap allocations/memory footprint?

That is a good point. Maybe premature optimization here.

tnull · 2025-06-10T10:24:24Z

wallet/src/types.rs

@@ -73,6 +73,8 @@ pub struct LocalOutput {
    pub derivation_index: u32,
    /// The position of the output in the blockchain.
    pub chain_position: ChainPosition<ConfirmationBlockTime>,
+    /// Whether this output exists in a transaction that is yet to be broadcasted.
+    pub needs_broadcast: bool,


When do we expect this to be set/unset exactly? I guess it can only be unset once the transaction in question reaches threshold confirmations?

That is a good point, and it shows the limitations of the BroadcastQueue concept. In fact, I did some further thinking on this and the BroadcastQueue should really be an IntentTracker and should track txs even if they are "network canonical".

There should be a method such as .tracked_txs_which_are_not_network_canonical (better name needed) so that the caller can decide to either replace the tx, or explicitly forget about it. There are caveats to doing both since we don't want to create a sub-graph where intended payments are duplicated - BDK should handle these situations properly, or provide the required information so that the caller can make a safe decision.

notmandatory · 2025-06-12T20:42:00Z

Do we know how Core handles these things?

@tnull do you mean how does the Core wallet handle un-broadcasted Tx and building new Tx on those un-broadcasted Tx outputs? As far as I know there are no features in the Core wallet for this beyond you the user holding on to your signed and un-broadcast Tx and manually building on those Tx outputs with the commands:

createrawtransaction
signrawtransactionwithwallet
when you're ready to broadcast any of these Tx sendrawtransaction

notmandatory

Overall looks like a powerful new feature, I only have minor comments. Once you feel the API is ready I'd like to have a live chat to review it with L2 users like @tnull and @stevenroose to validate it meets their use cases.

notmandatory · 2025-06-12T20:54:27Z

wallet/src/types.rs

@@ -73,6 +73,8 @@ pub struct LocalOutput {
    pub derivation_index: u32,
    /// The position of the output in the blockchain.
    pub chain_position: ChainPosition<ConfirmationBlockTime>,
+    /// Whether this output exists in a transaction that is yet to be broadcasted.
+    pub needs_broadcast: bool,


I like this idea if done as a non-exhaustive enum to help reduce future API breaking changes. If we include a "LOCKED" variant could this also support #259?

wallet/src/wallet/broadcast_queue.rs

notmandatory · 2025-06-12T21:19:26Z

wallet/src/wallet/broadcast_queue.rs

+        let tx = match tx_graph.get_tx(txid) {
+            Some(tx) => tx,
+            None => {
+                debug_assert!(


Would it be better to throw and Err here instead of the panic? It seems possible a user could mistakenly try to queue a Txid not in the tx_graph. Or is this ment to warn app devs that they should never let this happen?

I also don't understand why you only panic if the txid is not in the tx_graph and not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

Sorry this was never meant to be in the public API. The idea is that we should only add txids into the BroadcastQueue which are also in TxGraph. If that is not the case, it is definitely an internal BDK error.

wallet/src/wallet/broadcast_queue.rs

evanlinjin · 2025-06-15T07:31:28Z

Can we enforce or support BroadcastQueue as the only way to broadcast transactions in bdk_wallet? A user bypassing this mechanism should be considered? Are there reasons to not doing it? Why would you keep broadcasting tx outside of the queue when you have unbroadcasted tx in the queue?

@nymius I've rethought about this problem. I think instead of a BroadcastQueue, it should really be an IntentTracker (refer to my comment here). Broadcast-ability can be evaluated on a trasaction-by-transaction basis. I do not think it is viable to enforce BroadcastQueue as the only way to broadcast transactions are BDK is not responsible for broadcasting directly to the mempool.

I like that the queue purges itself automatically on syncs.

@thunderbiscuit I agree that it is nice to reason with. However, I think it will introduce some footguns. Let me provide an example:

Transaction A (an intended payment) is broadcasted.
Transaction A gets evicted from the mempool so it disappears from the transaction list.
The user realizes this and creates a second transaction (B) to atone for the disappearance of transaction A. However, the coin selection puts A and B on non-conflicting subgraphs.

Now A and B can exist in the same history, and thus we have the potential birth of a double-payment situation.

My proposal right now, as mentioned above, is to have an IntentTracker which the user needs to explicitly forget or replace a "diverged" transaction.

The wallet will keep track of two consistent views of history:

The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
The canonical intent view. This is what the user intends to happen.

If these two views are the same, no action is required. If these two views diverge, the caller should be able to easily respond to it explicitly.

The tx merely needs a broadcast.
The tx is low fee so needs RBF/CPFP.
An input is no longer available. RBF?
Explicitly forgetting (this is safe if a conflict is x number of confirmations deep).

evanlinjin · 2025-06-17T00:01:47Z

When doing coin selection, we should use the "intent view" to obtain the UTXO set. This is to avoid accidentally double-spending intended-to-be-canonical transactions.

However, some intended-to-be-canonical transactions could not be canonical now (due to confirmed conflicts), or conflicts with mempool transactions (RBF, which may not go through in time).

So there should be some sort of filtering based on transactions in the IntentTracker:

Don't spend from transactions with confirmed conflicts.
Try to avoid spending from transactions with unconfirmed conflicts.
Try to avoid spending from unbroadcasted transactions.
Try to avoid spending from evicted transactions.

Of course, there are other filters that BDK does not do, but should really do (out of scope of this PR, but probably part of the same interface/structure):

Try to avoid spending from untrusted unconfirmed outputs (as they can be cancelled/replaced by another party).
Try to avoid spending from unconfirmed transactions in general.

tnull · 2025-06-18T10:27:33Z

Do we know how Core handles these things?

@tnull do you mean how does the Core wallet handle un-broadcasted Tx and building new Tx on those un-broadcasted Tx outputs? As far as I know there are no features in the Core wallet for this beyond you the user holding on to your signed and un-broadcast Tx and manually building on those Tx outputs with the commands:
1. [createrawtransaction](https://bitcoincore.org/en/doc/29.0.0/rpc/rawtransactions/createrawtransaction/)

2. [signrawtransactionwithwallet ](https://bitcoincore.org/en/doc/29.0.0/rpc/wallet/signrawtransactionwithwallet/)

3. when you're ready to broadcast any of these Tx [sendrawtransaction](https://bitcoincore.org/en/doc/29.0.0/rpc/rawtransactions/sendrawtransaction/)

Mh, right, regarding the UTXO locking usecase, it does feature a rather simple interface through lockunspent / listlockunspent though. As said on #166, that (mod maybe an auto-unlock feature) would likely be all we'd really need on our end for now, I think.

nymius · 2025-06-19T02:44:41Z

Thanks for modeling this, from my perspective, it resembles to React DOM and virtual DOM, and its reconciliation model.

The user realizes this and creates a second transaction (B) to atone for the disappearance of transaction A. However, the coin selection puts A and B on non-conflicting subgraphs.

A quick check: when you say non-conflicting subgraphs, it is implied B is not spending any inputs from A, but is spending to the same outputs, right?

My proposal right now, as mentioned above, is to have an IntentTracker which the user needs to explicitly forget or replace a "diverged" transaction.

Do you have in mind some diff method between this IntentTracker and the canonical to find these divergences?
For the updates, all these actions you mentioned (re-broadcast, rbf, cpfp, forget) will be implemented as IntentTrackers methods?

The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.

I'm confused here, Wallet.transactions docs say the following:

/// 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.

My guess is relevant transactions should be left out of the equation here.

github-project-automation bot added this to BDK Wallet Jun 6, 2025

This was referenced Jun 6, 2025

Introduce broadcast queue. #220

Closed

Don't double spend created transaction just because they haven't been broadcasted #40

Open

evanlinjin force-pushed the feature/broadcast-queue branch 2 times, most recently from a4fbb83 to a9efbcc Compare June 6, 2025 10:15

evanlinjin force-pushed the feature/broadcast-queue branch from a9efbcc to 808bbdf Compare June 6, 2025 10:23

feat!: Introduce BroadcastQueue

75bc892

evanlinjin force-pushed the feature/broadcast-queue branch from 808bbdf to 75bc892 Compare June 6, 2025 10:27

feat: Add TxBuilder::exclude_unbroadcasted

a65eabe

If the caller wishes to avoid spending unbroadcasted UTXOs without double-spending any of the unbroadcasted transactions.

evanlinjin self-assigned this Jun 7, 2025

notmandatory moved this to In Progress in BDK Wallet Jun 7, 2025

notmandatory added the new feature New feature or request label Jun 7, 2025

nymius reviewed Jun 8, 2025

View reviewed changes

nymius mentioned this pull request Jun 8, 2025

feat!: Persist utxo lock status #259

Open

5 tasks

tnull reviewed Jun 10, 2025

View reviewed changes

notmandatory reviewed Jun 12, 2025

View reviewed changes

notmandatory mentioned this pull request Jun 18, 2025

Add internal SPK filtering to SyncRequestBuilder for efficient mempool eviction handling #269

Open

notmandatory modified the milestone: Wallet 3.0.0 Jun 25, 2025

evanlinjin added 3 commits June 27, 2025 07:53

WIP: IntentTracker

68c0505

feat!: [WIP] More IntentTracker tweaks

8a2d04f

docs: [WIP] Initial work on intent tracker example

8e1b43c

evanlinjin changed the title ~~Introduce BroadcastQueue~~ Introduce IntentTracker Jun 28, 2025

This was referenced Jul 4, 2025

Detect new mempool txs bitcoindevkit/bdk#1988

Open

Address gap limit? #74

Open

Introduce IntentTracker #257

Are you sure you want to change the base?

Introduce IntentTracker #257

Conversation

evanlinjin commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

To Get Out of Draft Status:

To Get This Merged:

All Submissions:

New Features:

Bugfixes:

Uh oh!

coveralls commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15943002368

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

nymius Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nymius left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thunderbiscuit commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanlinjin commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nymius commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Introduce `IntentTracker` #257

Introduce `IntentTracker` #257

evanlinjin commented Jun 6, 2025 •

edited

Loading

coveralls commented Jun 6, 2025 •

edited

Loading

nymius Jun 8, 2025 •

edited

Loading

nymius left a comment •

edited

Loading

thunderbiscuit commented Jun 9, 2025 •

edited

Loading

notmandatory commented Jun 12, 2025 •

edited

Loading

evanlinjin commented Jun 15, 2025 •

edited

Loading

evanlinjin commented Jun 17, 2025 •

edited

Loading

tnull commented Jun 18, 2025 •

edited

Loading

nymius commented Jun 19, 2025 •

edited

Loading