Introduce IntentTracker

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.

[evanlinjin]

Fixes#166
Fixes#40
Fixed#295
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]

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

[nymius]

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.

[notmandatory]

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]

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]

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?

[evanlinjin]

That is a good point. Maybe premature optimization here.

[tnull]

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?

[evanlinjin]

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]

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
2. signrawtransactionwithwallet
3. 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]

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?

[notmandatory]

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_graphand not in the dedup set. Isn't not having the Txid in the graph enough to panic due to it being invalid?

[evanlinjin]

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.

[evanlinjin]

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:

1. The canonical network view. This is what BDK assumes to be what the network sees. Currently wallet.transactions returns this.
2. 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]

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]

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]

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.

[ovitrif]

Hi guys, Bitkit team dev here, needing this to unlock:

• Add user-facing events for applying updates after syncing #6

Why?

• Add user-facing events for applying updates after syncing #6 (comment)

EDIT: nvm, #6 is now fixed by #310. Thank you for your attention and collaboration 🙏🏻.