-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 17143814147Details
💛 - Coveralls |
How do you handle UTXO locking in concurrent wallet instances? |
There was a problem hiding this 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?
@ChidiChuks could you be a bit more specific with this question? Do you mean sharing lock status between multiple instances of the same wallet? |
@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 |
There was a problem hiding this 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.
b083dff
to
a6b0f5d
Compare
@evanlinjin I took all of your suggestions other than I think the proposed |
a6b0f5d
to
13e024d
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
37fac7d
to
ee0a88b
Compare
wallet/src/wallet/mod.rs
Outdated
#[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, | ||
} |
There was a problem hiding this comment.
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 oflocked_outpoints() -> Map<OutPoint, UtxoLock>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
892ffd0
to
a6381de
Compare
a6381de
to
d020559
Compare
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`.
977034c
to
80157d6
Compare
The example crates contain code that isn't accepted by the 1.63.0 version of the compiler (e.g. `is_multiple_of`). Since the MSRV of the wallet library is currently 1.63.0, and clippy is configured to adhere to that, we exclude the example crates from the clippy check enforced by CI.
80157d6
to
1be014f
Compare
@@ -109,6 +110,7 @@ pub struct Wallet { | |||
stage: ChangeSet, | |||
network: Network, | |||
secp: SecpCtx, | |||
locked_outpoints: BTreeMap<OutPoint, Option<bool>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using a HashSet<OutPoint>
here be more appropriate?
- I don't think ordering is important when returning
OutPoint
s. Option<false>
andNone
don’t seem to be distinguished in the current logic.
Other than my comment above, everything looks good! |
This PR adds methods to
Wallet
to lock and unlock a UTXO by outpoint, to query the locked outpoints, and updates the walletChangeSet
to persist the lock status of an outpoint.fixes #166
fixes #245
Considerations for broadcast scenarios:
When a UTXO is locked, it won't be selected for a transaction. This is useful in cases where the user needs to permanently exclude an output from being spent, or reserve it until some time in the future and continue making transactions in the meantime. To eventually spend the coin the user has to explicitly unlock it. This process could perhaps be facilitated by implementing an automatic lock expiry based on block height.
If you submit a transaction to the network and then lock the inputs, then they won't be re-selected in subsequent transactions. This prevents us from inadvertently "double-spending" ourselves. Once broadcast, the lock effect depends on the fate of the transaction: If it confirms and the outpoint is still locked, then the lock is no longer useful, because the input can't be double spent anyway by virtue of consensus. If a conflict confirms which spends the same output, the lock is also irrelevant. If a conflict confirms but doesn't spend the already locked output then the lock remains in effect.
If the transaction is somehow dropped from the mempool but otherwise still valid, we can either rebroadcast the tx or create a replacement. As far as I can tell the implementation doesn't prevent us from creating a Replace-By-Fee transaction regardless of the lock status, since the inputs to the RBF are considered "manually selected".
To avoid the risk of double payment (sending two txs to the same payment invoice) one needs to examine the details of the second transaction to ensure that it actually replaces the first one, otherwise we've simply made a donation to our counterparty.
Changelog notice
Checklists
All Submissions:
just p
before pushingNew Features: