Skip to content

feat!: Persist utxo lock status #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Jun 7, 2025

Added methods on Wallet to lock/unlock an outpoint, to query the locked outpoints, and updated the wallet ChangeSet to persist the lock status of an outpoint.

fixes #166
fixes #245

Changelog notice

Added these methods to Wallet

  • lock_outpoint
  • unlock_outpoint
  • list_locked_outpoints
  • list_locked_unspent
  • is_outpoint_locked

Added

  • New type wallet::locked_outpoints::ChangeSet
  • BREAKING: New member field wallet::ChangeSet::locked_outpoints

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • This pull request breaks the existing API
  • I'm linking the issue being fixed by this PR

@ValuedMammal ValuedMammal moved this to Todo in BDK Wallet Jun 7, 2025
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Jun 7, 2025
@ValuedMammal ValuedMammal self-assigned this Jun 7, 2025
@coveralls
Copy link

coveralls commented Jun 7, 2025

Pull Request Test Coverage Report for Build 16851016230

Details

  • 102 of 122 (83.61%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 84.936%

Changes Missing Coverage Covered Lines Changed/Added Lines %
wallet/src/wallet/changeset.rs 46 50 92.0%
wallet/src/wallet/persisted.rs 0 6 0.0%
wallet/src/wallet/mod.rs 50 60 83.33%
Files with Coverage Reduction New Missed Lines %
wallet/src/wallet/mod.rs 1 82.02%
Totals Coverage Status
Change from base Build 16786013135: 0.02%
Covered Lines: 6766
Relevant Lines: 7966

💛 - Coveralls

@ChidiChuks
Copy link

How do you handle UTXO locking in concurrent wallet instances?

Copy link
Contributor

@nymius nymius left a comment

Choose a reason for hiding this comment

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

Could you expand a little bit more on how this mechanism could be used in broadcast scenarios?
From the test I guess all the logic of why a UTxO becomes available again is kept by the lib user? Like unlock UTxO after a height in the future if there is not transaction spending it in the blockchain.

This is a competing solution for #257, right?

@evanlinjin
Copy link
Member

How do you handle UTXO locking in concurrent wallet instances?

@ChidiChuks could you be a bit more specific with this question? Do you mean sharing lock status between multiple instances of the same wallet?

@evanlinjin
Copy link
Member

This is a competing solution for #257, right?

@nymius My understanding is that this is NOT a competing solution with #257. UTXO-locking will not fully solve #166 as we want unbroadcasted outputs to be available for selection. In fact, UTXO-locking is already available by using TxBuilder::add_unspendable. The only difference here is that locked outpoints are persisted with BDK. So this is more of a convenience feature that users are requesting.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thanks for this work.

I propose adding the following methods:

  • list_locked_outpoints - Lists all locked outpoints.
  • list_locked_unspents - Lists locked outpoints that are unspent.

Imagine a situation where the caller broadcasts a tx and locks the prevouts. Then the tx gets evicted from the mempool. It will be helpful to list locked unspents at this point to recover those prevouts.

@notmandatory notmandatory mentioned this pull request Jun 12, 2025
14 tasks
@ValuedMammal ValuedMammal marked this pull request as ready for review June 16, 2025 17:29
@ValuedMammal ValuedMammal moved this from Todo to Needs Review in BDK Wallet Jun 16, 2025
@ValuedMammal
Copy link
Collaborator Author

ValuedMammal commented Jun 16, 2025

@evanlinjin I took all of your suggestions other than I think the proposed Wallet::list_locked_unspents is not yet implemented. edit: I have some ideas for this though.

@ValuedMammal ValuedMammal moved this from Needs Review to In Progress in BDK Wallet Jun 18, 2025
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Jun 23, 2025
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

I'm happy to see this feature moving along. In addition to helping L2 users like @tnull it also helps support one of my personal goals of improving feature parity with the bitcoin core wallet.

}

/// List unspent outpoints that are currently locked.
pub fn list_locked_unspent(&self) -> impl Iterator<Item = OutPoint> + '_ {
Copy link
Member

@notmandatory notmandatory Jun 25, 2025

Choose a reason for hiding this comment

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

⛏️ a nit, it's a little confusing to refer to the outpoints as "unspent" here and "outpoints" in the other new functions. Since we already have the "list_unspent" function would it make sense to use "unspent" for all the related functions (ie. locked_unspent, is_unspent_locked, lock_unspent, unlock_unspent)? Even though we all know what "outpoints" mean for those unfamiliar with bitcoin internals it's not a very descriptive term.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is okay since unspent here is short for unspent_outpoint whereas outpoint elsewhere is short for outpoints which can be unspent/spent.

@notmandatory notmandatory added new feature New feature or request api A breaking API change labels Jun 25, 2025
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Implementation looks good. Just need a bit more clarity on expiration_height before ACK.

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about an unlock_all_outpoints method? Just in case the caller needs to reset their state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since it can be expressed with the current APIs I don't think it's essential, but I'm not opposed to someone adding it in the future.

@ValuedMammal ValuedMammal moved this from Needs Review to In Progress in BDK Wallet Jun 27, 2025
@ValuedMammal ValuedMammal force-pushed the feat/lock-unspent branch 2 times, most recently from 37fac7d to ee0a88b Compare July 14, 2025 17:26
Comment on lines 2686 to 2693
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[non_exhaustive]
pub struct UtxoLock {
/// Outpoint.
pub outpoint: OutPoint,
/// Whether the outpoint is locked.
pub is_locked: bool,
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get away with removing this for now.

  • Internally, we can use locked_outpoints: Map<OutPoint, Option<bool>>.
  • We can have list_locked_outpoints() -> Iterator<OutPoint> instead of locked_outpoints() -> Map<OutPoint, UtxoLock>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point.

@ValuedMammal ValuedMammal force-pushed the feat/lock-unspent branch 2 times, most recently from 892ffd0 to a6381de Compare August 3, 2025 22:00
New APIs added for locking and unlocking a UTXO by outpoint
and to query the locked outpoints. Locking an outpoint
means that it is excluded from coin selection.

- Add `Wallet::lock_outpoint`
- Add `Wallet::unlock_outpoint`
- Add `Wallet::is_outpoint_locked`
- Add `Wallet::list_locked_outpoints`
- Add `Wallet::list_locked_unspent`

`test_lock_outpoint_persist` tests the lock/unlock
functionality and that the lock status is persistent.

BREAKING: Added `locked_outpoints` member field to ChangeSet.
A SQLite migration is included for adding the locked outpoints table.
..by `Box`ing the descriptor in `LoadMismatch` enum,
and by boxing the ChangeSet in `DataAlreadyExists` variant of
`CreateWithPersistError`.

We allow the large_enum_variant lint for `FileStoreError` for now,
as it is planned to be fixed in a future version of `bdk_file_store`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change new feature New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

ci: fix large enum and error types to clippy recommended sizes Let TxBuilder avoid previously used UTXOs (UTXO locking)
7 participants