Addresses and closes #22368
As per that issue (and its predecessor #14907), there seems to be some interest in allowing unspent outputs to be locked persistently. This PR does so by adding a flag to lockunspent to store the change in the wallet database. Defaults to false, so there is no change in default behaviour.
Edit: GUI commit changes default behaviour. UTXOs locked/unlocked via the GUI are now persistent.
2184@@ -2183,9 +2185,17 @@ static RPCHelpMan lockunspent()
2185
2186 bool fUnlock = request.params[0].get_bool();
2187
2188+ bool store_lock = false;
2189+ if (!request.params[2].isNull()) {
2190+ RPCTypeCheckArgument(request.params[2], UniValue::VBOOL);
2191+ store_lock = request.params[2].get_bool();
2192+ }
nit: Univalue get_* will already do type checking, so can drop RPCTypeCheckArgument.
0 const bool store_lock{request.params[2].isNull() ? false : request.params[2].get_bool()};
2262+ if (!pwallet->UnlockCoin(outpt, batch)) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed");
2263+ } else {
2264+ if (!pwallet->LockCoin(outpt, batch)) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed");
2265+ }
2266 }
2267+ if (batch) delete batch;
Concept ACK
Thanks a lot for improving privacy and working on this issue. Will test it in few minutes.
130+
131+ # Restarting the node should clear the lock
132+ self.restart_node(2)
133+ self.nodes[2].lockunspent(False, [unspent_0], True)
134+
135+ # Restarting the node now the lock is stored in memory should keep the lock
In 94b6c8db5010bf1f38f1458866575cb599d17326 “Update lockunspent tests for lock persistence”
nit:
0 # Restarting the node with the lock written to the wallet should keep the lock
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
2152@@ -2152,6 +2153,7 @@ static RPCHelpMan lockunspent()
2153 },
2154 },
2155 },
2156+ {"store_lock", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to write/erase this lock in the wallet database, or keep the change in memory only."},
2157 },
2158 RPCResult{
2159 RPCResult::Type::BOOL, "", "Whether the command was successful or not"
Tested on Pop!_OS and everything works as expected.
Steps that I followed for testing:
0bitcoin-cli -rpcwallet=W1 listunspent
1
2[
3 {
4 "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
5 "vout": 0,
6 "address": "bcrt1qa8xy0hlvlsr66yvv80h3qxp9stq68aef6pzvjt",
7 "label": "",
8 "scriptPubKey": "0014e9cc47dfecfc07ad118c3bef10182582c1a3f729",
9 "amount": 0.00010000,
10 "confirmations": 1003,
11 "spendable": true,
12 "solvable": true,
13 "desc": "wpkh([0ab0c859/0'/0'/2']03c65a6e4a66a4a6b4af981c5e81632bb2d7336bc75d7ccf83e055006a2693050f)#6z2c6spl",
14 "safe": true
15 }
16]
17
18$ bitcoin-cli -rpcwallet=W1 lockunspent false "[{\"txid\":\"752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff\",\"vout\":0}]"
19true
20
21$ bitcoin-cli -rpcwallet=W1 listlockunspent
22[
23 {
24 "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
25 "vout": 0
26 }
27]
28
29$ bitcoin-cli stop
30Bitcoin Core stopping
31
32$ bitcoind
33
34$ bitcoin-cli -rpcwallet=W1 listlockunspent
35[
36]
0$ bitcoin-cli -rpcwallet=W1 lockunspent false "[{\"txid\":\"752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff\",\"vout\":0}]" true
1true
2
3$ bitcoin-cli -rpcwallet=W1 listlockunspent
4[
5 {
6 "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
7 "vout": 0
8 }
9]
10
11$ bitcoin-cli stop
12Bitcoin Core stopping
13
14$ bitcoind
15
16$ bitcoin-cli -rpcwallet=W1 listlockunspent
17[
18 {
19 "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
20 "vout": 0
21 }
22]
0$ bitcoin-cli -rpcwallet=W1 lockunspent true "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]" true
1
2error code: -8
3error message:
4Invalid parameter, expected locked output
0$ bitcoin-cli -rpcwallet=Wallet1 lockunspent false "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]" true
1
2error code: -8
3error message:
4Invalid parameter, output already locked
:information_source: It also works for descriptor wallets
:warning: There are some issues with GUI but I guess they can be addressed in https://github.com/bitcoin-core/gui
bitcoin-cli its greyed out and there is an option to unlock it. This unlocked state won’t be saved in db and will remain until GUI is running.
@meshcollider Thanks for adding this option :) It will improve privacy and few projects were also dependent on this feature. Example: https://github.com/p2pderivatives/p2pderivatives-client#known-limitations
Thanks, addressed both review suggestions.
Happy to make GUI changes persistent too, it would be a very simple change. Can discuss if it is a useful change.
Happy to make GUI changes persistent too, it would be a very simple change. Can discuss if it is a useful change.
I think it is. People who use manual coin control will likely manually selected specfic UTXOs for single tx and lock unspent for longer term. And longer term should survive bitcoin-qt restarts. At least that looks to me more intuitive. Also, from privacy perspective, it is better to have some unwanted UTXO locked longer than spend it accidentally.
tACK https://github.com/bitcoin/bitcoin/pull/23065/commits/687d65f910755ae4c73db1cfe6b4ffaf351ce297
Changes since last review:
wallet_address_types.py –descriptors | ✖ Failed | 25 s
Not sure if this error in CI is related to PR: https://github.com/bitcoin/bitcoin/pull/23065/checks?check_run_id=3682608670
@prayank23 Not sure if this error in CI is related to PR
Looks unrelated, it looks like some random windows socket issue:
OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted
2152@@ -2152,6 +2153,7 @@ static RPCHelpMan lockunspent()
2153 },
2154 },
2155 },
2156+ {"store_lock", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to write/erase this lock in the wallet database, or keep the change in memory only."},
reACK https://github.com/bitcoin/bitcoin/pull/23065/commits/2d3ed88667eeaeb692581a11550f87e77d7ae6a1
Changes since last review:
Argument name changed which can be used to save the state of UTXO lock in db as suggested in #23065 (review)
0-store_lock
1+persistent
Concept ACK.
I have some questions though:
CWallet::AddToSpends we should remove persisted lock?
lock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?
I don’t think we should allow re-locking in any case, better to require explicitly unlocking then locking again.
unlock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?
I was thinking a lot about this while writing the PR. It does make sense to me that unlock would simply remove any locks, persistent or not. But I am not sure if there are some use cases that would benefit from the functionality - persistently lock a UTXO and then temporarily unlock it for a while for some reason? I’m happy to change this if people agree.
Good points about AddToSpends and the release notes, will fix.
lockunspent true after load.
Why not just always persist? What are the cons of that? If someone wants no locks then just call lockunspent true after load.
Not sure about this. It’s a much larger behavior change (downstream software probably relies on starting with a clean slate after restart), and if this behavior really turns out to be more popular it could always be done later.
Why not just always persist? What are the cons of that? If someone wants no locks then just call lockunspent true after load.
Not sure about this. It’s a much larger behavior change (downstream software probably relies on starting with a clean slate after restart), and if this behavior really turns out to be more popular it could always be done later.
Not only restart, even loadwallet after unloading.
How about a startup flag like --persist-unspent-locks?
If we keep this approach then it needs a way to select persist=true in all calls that eventually lock coins, like fundrawtransaction lockUnspents=true. Suddenly there’s a lot of options around.
I just think a (good) behavior change looks saner to reason about.
How about a startup flag like –persist-unspent-locks?
Please don’t add another startup option that subtly changes behavior over all wallets :smile: I was actually fearing this when I looked at this PR, and was pleasantly surprised it’s a flag per utxo.
fundrawtransaction/send RPCs and lock reasons for a follow-up PR, to keep this change simpler. Storing the reason should be easy, just using the value of the LOCKED_UTXO DB field which is currently just always 1.
130+
131+ # Restarting the node should clear the lock
132+ self.restart_node(2)
133+ self.nodes[2].lockunspent(False, [unspent_0], True)
134+
135+ # Restarting the node with the lock written to the wallet should keep the lock
2274-void CWallet::UnlockCoin(const COutPoint& output)
2275+bool CWallet::UnlockCoin(const COutPoint& output, WalletBatch* batch)
2276 {
2277 AssertLockHeld(cs_wallet);
2278 setLockedCoins.erase(output);
2279+ if (batch) {
false or you can avoid EraseLockedUTXO.
2259@@ -2260,22 +2260,36 @@ bool CWallet::DisplayAddress(const CTxDestination& dest)
2260 return signer_spk_man->DisplayAddress(scriptPubKey, signer);
2261 }
2262
2263-void CWallet::LockCoin(const COutPoint& output)
2264+bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch)
2265 {
2266 AssertLockHeld(cs_wallet);
2267 setLockedCoins.insert(output);
lock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?
I don’t think we should allow re-locking in any case, better to require explicitly unlocking then locking again.
If you want to implement this breaking change then use the return value of insert() to know if the output was already locked and return false.
Otherwise, the lock can be persisted but not in memory.
Otherwise, the lock can be persisted but not in memory.
Sorry, I’m not sure how this could happen. Upon thinking further, I think its fine to upgrade a memory-only lock to a persistent lock (this is useful if fundrawtransaction locked the spends and we want to persistently lock them afterward). But how could we end up with a persistent lock not in memory?
EDIT: I’ve updated the PR to allow persistently locking even if we already have a lock, to allow upgrading.
But how could we end up with a persistent lock not in memory?
Right, doesn’t happen now that unlock clears from memory and db.
But does it make sense to allow memory-only lock if it is already persisted?
EDIT: looks like this is not possible since lockunspent RPC checks if unspent is already locked.
Many changes since last review: https://github.com/bitcoin/bitcoin/compare/2d3ed88..d96b000, agree that changes about GUI should be mentioned in release notes and few things looked confusing in above discussion so tested again.
GUI only : Lock/Unlock no issues.
CLI:
2.1 Errors: Invalid parameter, output already locked this is not printed in one case when when you try to lock UTXO which is already locked with new parameter set as true⚠️
2.2 Locking UTXO without using new parameter works as expected and reset to unlocked on restart 2.3 Locking UTXO with new parameter set as true works as expected. Locked state is saved in db and remains even after restart.
2.4 Lock UTXO without new parameter. Unlock UTXO with new parameter set as true. Unlocked state remains after restart. 2.5 Lock UTXO with new parameter (db). Unlock UTXO without new parameter. Unlocked state remains after restart :x:
CLI + GUI:
3.1 Lock UTXO with and without new parameter. Stop bitcoind. Launch bitcoin-qt and check if state remains as expected. No issues.
3.2 Lock UTXO from CLI (with new param). Stop bitcoind. Unlock from GUI. Restart bitcoind. Check if unlocked state remains. No issues.
3.3 Unlock UTXO from CLI (with new param). Stop bitcoind. Lock from GUI. Restart bitcoind. Check if locked state remains. No issues.
3.4 Lock UTXO from CLI (without new param). Stop bitcoind. Lock from GUI. Restart bitcoind. Check if locked state remains. No issues.
3.5 Unlock UTXO from CLI (without new param). Stop bitcoind and check state in bitcoin-qt. It remains locked :x:
2.5 and 3.5 doesn’t look correct IMO. Also agree with @promag that very few people will be affected if this is made persistent by default in CLI and GUI. Infact people might start using this after this change.
Lock UTXO with new parameter (db). Unlock UTXO without new parameter. Unlocked state remains after restart x
No? There’s also a test for that case. Perhaps you are looking at a previous revision.
Thanks @prayank23 for your detailed testing!
2.1 Errors: Invalid parameter, output already locked this is not printed in one case when when you try to lock UTXO which is already locked with new parameter set as true⚠️
Yes, I decided it isn’t worth the overhead of checking if the persistent lock exists in the database, if someone wants to lock the same output twice persistently then I don’t see why that should error. It will, however, error if you try and lock un-persistently if the lock already exists – this is to avoid confusion about downgrading a persistent lock to non-persistent (not allowed).
2.5 and 3.5 doesn’t look correct IMO.
As @achow101 mentioned, this doesn’t seem to be the case. I manually tested 3.5 and cannot replicate the issue – the lock is definitely cleared persistently and does not remain locked for me. Could you check you built the latest version?
Tested ACK https://github.com/bitcoin/bitcoin/pull/23065/commits/d96b000e94d72d041689c5c47e374df2ebc0e011 on Ubuntu 20.04
2.5 and 3.5 doesn’t look correct IMO.
I also cannot replicate the issues. The steps mentioned in 2.5 and 3.5 worked as expected in my tests.
Steps that I did for 2.5:
0$ bitcoind --version
1Bitcoin Core version v22.99.0-d96b000e94d7
2
3$ bitcoind
4
5$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent
6[
7]
8
9$ bitcoin-cli -rpcwallet=Wallet1 lockunspent false "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]" true
10
11true
12
13$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent
14
15[
16 {
17 "txid": "f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7",
18 "vout": 1
19 }
20]
21
22$ bitcoin-cli -rpcwallet=Wallet1 lockunspent true "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]"
23
24true
25
26$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent
27
28[
29]
30
31$ bitcoin-cli stop
32
33$ bitcoind
34
35$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent
36
37[
38]
Oh, apologies @prayank23 , I misread your comment. That behaviour is intended:
I was thinking a lot about this while writing the PR. It does make sense to me that unlock would simply remove any locks, persistent or not.
We discussed this briefly on IRC at the wallet meeting and decided this approach made the most sense.
(By the way, if you start bitcoind with the -daemon flag you don’t need to use two terminal windows :) )
ACK https://github.com/bitcoin/bitcoin/pull/23065/commits/d96b000e94d72d041689c5c47e374df2ebc0e011
nit: I will be using persistence parameter set to true every time or GUI so 2.5 and 3.5 isn’t an issue for me personally. Can be confusing for some users.