feat!: Persist utxo lock status #259
Conversation
Pull Request Test Coverage Report for Build 15780919952Details
💛 - Coveralls |
|
How do you handle UTXO locking in concurrent wallet instances? |
There was a problem hiding this comment.
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.
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.
wallet/src/wallet/mod.rs
Outdated
| /// Information about a wallet UTXO. | ||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] | ||
| pub struct UtxoInfo { |
There was a problem hiding this comment.
I originally called this UtxoInfo with the idea that it may be extended with more information, but since the structure itself doesn't qualify as a "sub-changeset", I think it should only contain the information related to the utxo lock. See a6b0f5d.
ValuedMammal
force-pushed
the
feat/lock-unspent
branch
from
b083dff to
a6b0f5d
Compare
|
@evanlinjin I took all of your suggestions other than I think the proposed |
wallet: - Add pub struct `UtxoLock` - Add `Wallet::lock_outpoint` - Add `Wallet::unlock_outpoint` - Add `Wallet::list_locked_outpoints` - Add `Wallet::is_outpoint_locked` changeset: - Add member to ChangeSet `locked_outpoints: BTreeMap<OutPoint, UtxoLock>` `tests/wallet.rs`: - Add test `test_lock_outpoint_persist`
Since the changeset grew in size, the old values were insufficient. The recommended solution long-term is to place the affected types behind a Box pointer.
ValuedMammal
force-pushed
the
feat/lock-unspent
branch
from
a6b0f5d to
13e024d
Compare
| txid TEXT NOT NULL, \ | ||
| vout INTEGER NOT NULL, \ | ||
| is_locked INTEGER, \ | ||
| expiration_height INTEGER, \ |
There was a problem hiding this comment.
What would be the usecases for this expiration_height feature?
If the only goal is to forget about locked outpoints if a transaction fails to be included in a block or is dropped from the mempool due to insufficient feerates, it may result in the caller creating a second transaction that is intended to replace the original payment which does not conflict the original. Now we have a situation where the original and replacement can belong in the same history resulting in double-payment. As suggested in #257, it is only safe to forget about a tx if there is a conflict which is x-confirmations deep (where x is defined by the caller, based on their assumptions).
Let me know if there is any other usecase that I am unaware of. Otherwise, I recommend removing this feature as it seems dangerous.
cc. @tnull
References:
- Original feature suggestion: Let
TxBuilderavoid previously used UTXOs (UTXO locking) #166 (comment) - Proposed no-footgun API: Introduce
BroadcastQueue#257 (comment). Note that this does not automatically "untrack" transactions if a conflict has reached x-confirmations. If automation is important, it can be implemented in a separate PR. The API would be: "automatically forget if a conflicting tx reasons x-confirmations deep". For transactions that are evicted from the mempool due to insufficient fee, the safe thing to do would be to explicitly replace, or cancel with a replacement transaction.
There was a problem hiding this comment.
Yes, I agree with everything you said. It's just that if you don't offer that API, the user will implement such a thing themselves, as at some point the user would need unlock previously-locked UTXOs for which the spend 'failed'. And usually that means that they'd do a worse job as they have less context, less access to wallet internals, and presumably less time spent thinking through the issues. So while we often tend to leave things we can't decide on to our users, it unfortunately also means that we set them up to do a worse job than us.
All that said, I agree that it might be tricky to get the API right to avoid any footguns, and maybe it could happen in a follow-up/separate PR, especially if it's undecided yet and would block this PR.
There was a problem hiding this comment.
The use case I had in mind was to prevent a utxo from being selected for a specific duration of time measured in blocks. As mentioned, it frees the user from having to explicitly unlock it in the future.
| /// into account any timelocks directly encoded by the descriptor. | ||
| #[derive( | ||
| Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, serde::Serialize, serde::Deserialize, | ||
| )] |
There was a problem hiding this comment.
Since the caller does not need to create this struct, what do you think about sneaking in a #[non_exhaustive] so we can add fields in minor releases?
There was a problem hiding this comment.
I made a comment #259 (comment) about why I don't think the struct in its present form should be extended, in short because it's not a true changeset. That said, I can change the implementation, by making a proper sub-changeset, such that it is more conducive to being extended.
Description
Added methods on
Walletto lock/unlock an outpoint, to query the locked outpoints, and updated the walletChangeSetto persist information about wallet UTXOs including the lock status of an outpoint. You can optionally set an expiration height for the outpoint to become unlocked once the local tip reaches the specified block height.may resolve #166.
Changelog notice
Added the following methods to
Walletlock_outpointunlock_outpointlist_locked_outpointsis_outpoint_lockedAdded
ChangeSet::locked_outpointsChecklists
All Submissions:
New Features:
Bugfixes: