rpc: fundrawtransaction and walletcreatefundedpsbt also lock manually selected coins

Sjors commented at 5:31 pm on March 2, 2020: member

When using fundrawtransaction and walletcreatefundedpsbt with lockUnspents, it would only lock automatically selected coins, not manually selected coins. That doesn’t make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction.

Note that when creating a transaction, manually selected coins are automatic “unlocked” (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with lockUnspents) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling lockunspent and the subsequent spending RPC.

See #7518 for historical background.

DrahtBot added the label RPC/REST/ZMQ on Mar 2, 2020

DrahtBot added the label Tests on Mar 2, 2020

DrahtBot added the label Wallet on Mar 2, 2020

promag commented at 11:46 am on March 4, 2020: member

Concept ACK!

This made me think that we should fail funding if some coin is already locked?

in test/functional/rpc_psbt.py:110 in 0af7e9f480 outdated

88@@ -89,6 +89,15 @@ def run_test(self):
89         final_tx = self.nodes[0].finalizepsbt(signed_tx)['hex']
90         self.nodes[0].sendrawtransaction(final_tx)
91 
92+        # Manually selected inputs can be locked:
93+        utxo1 = self.nodes[0].listunspent()[0]
94+        psbtx1 = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():1}, 0,{"lockUnspents": True})["psbt"]
95+        assert_equal(len(self.nodes[0].listlockunspent()), 1)

promag commented at 2:33 pm on March 4, 2020:

assert_equal(len(self.nodes[0].listlockunspent()), 0) before walletcreatefundedpsbt?

promag commented at 2:36 pm on March 4, 2020: member

Add a release note maybe? Clients unaware of this change can have dangling utxos locked.

Sjors force-pushed on Mar 5, 2020

Sjors commented at 9:27 am on March 5, 2020: member

Added a release note.

This made me think that we should fail funding if some coin is already locked?

Maybe in a separate PR. I also think it’s safer for users to explicitly unlock coins before using them.

promag commented at 4:11 pm on March 5, 2020: member

I also think it’s safer for users to explicitly unlock coins before using them.

The unspent locking was done to deal with concurrent clients and so a client shouldn’t take the decision to unlock a coin so lightly - it might screw with a concurrent funding. This is a possible use case and for this reason the user should be able to lock all or nothing IMO.

Maybe in a separate PR

~~Yes sure!~~ Well kind of, the lock-an-already-locked-coin-and-ignore behavior is added here :thinking:.

Sjors commented at 10:28 am on March 6, 2020: member

the lock-an-already-locked-coin-and-ignore behavior

Not sure what you mean. This PR can potentially lock a coin that’s already locked, but why is that a problem? This PR does not ignore new things.

promag commented at 11:09 am on March 6, 2020: member

The problem is that you might screw with a concurrent funding. When you lock a coin you should expect that it won’t be used by others.

Sjors commented at 1:30 pm on March 6, 2020: member

If a concurrent wallet consumer uses manual coin selection, I don’t think this PR makes their problem worse; we were already ignoring any lock they had. If it uses automatic automatic coin selection then this PR has no impact.

Sjors force-pushed on Mar 6, 2020

Sjors renamed this:
~~rpc: have lockUnspents also lock manually selected coins~~
rpc: fundrawtransaction and walletcreatefundedpsbt respect locks even with manual coin selection
on Mar 6, 2020

Sjors commented at 3:11 pm on March 6, 2020: member

After IRC discussion I changed this PR to honour locks for manually selected coins.

in src/wallet/wallet.cpp:2462 in 2ad41557cd outdated

2457+    // is an atomic operation.
2458+    auto locked_chain = chain().lock();
2459+    LOCK(cs_wallet);
2460+
2461     for (const CTxIn& txin : tx.vin) {
2462+        if(IsLockedCoin(txin.prevout.hash, txin.prevout.n)) {

promag commented at 3:51 pm on March 6, 2020:

space

Sjors commented at 4:57 pm on March 6, 2020:

The final frontier.

in src/wallet/rpcwallet.cpp:2153 in 2ad41557cd outdated

2149@@ -2150,6 +2150,7 @@ static UniValue lockunspent(const JSONRPCRequest& request)
2150                 "Temporarily lock (unlock=false) or unlock (unlock=true) specified transaction outputs.\n"
2151                 "If no transaction outputs are specified when unlocking then all current locked transaction outputs are unlocked.\n"
2152                 "A locked transaction output will not be chosen by automatic coin selection, when spending bitcoins.\n"
2153+                "With manual coin selection a locked transaction output must unlocked before use."

promag commented at 4:14 pm on March 6, 2020:

“… must be …”

in test/functional/wallet_basic.py:143 in 2ad41557cd outdated

135@@ -136,11 +136,20 @@ def run_test(self):
136                                 self.nodes[2].lockunspent, False,
137                                 [{"txid": unspent_0["txid"], "vout": 999}])
138 
139-        # An output should be unlocked when spent
140+        # The lock on a manually selected output is honoured
141         unspent_0 = self.nodes[1].listunspent()[0]
142         self.nodes[1].lockunspent(False, [unspent_0])
143         tx = self.nodes[1].createrawtransaction([unspent_0], { self.nodes[1].getnewaddress() : 1 })
144-        tx = self.nodes[1].fundrawtransaction(tx)['hex']

promag commented at 4:17 pm on March 6, 2020:

Are you removing this case because it’s used elsewhere?

Sjors commented at 5:00 pm on March 6, 2020:

The original test was for auto-unlocking a coin, which now produces an error, so I check for that.

DrahtBot commented at 4:54 pm on March 6, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Sjors force-pushed on Mar 6, 2020

in test/functional/wallet_basic.py:148 in 991a38ce9e outdated

146+                                self.nodes[1].fundrawtransaction,tx,{"lockUnspents": True})
147+
148+        # fundrawtransaction can lock an input
149+        self.nodes[1].lockunspent(True, [unspent_0])
150+        assert_equal(len(self.nodes[1].listlockunspent()), 0)
151+        tx = self.nodes[1].fundrawtransaction(tx,{"lockUnspents": True})['hex']

promag commented at 3:41 pm on March 9, 2020:

nit, add space if you happen to push again.

promag commented at 3:42 pm on March 9, 2020: member

Code review ACK 991a38ce9e9c19ec78139d2988b2abd7c9983519.

instagibbs commented at 3:29 pm on March 25, 2020: member

concept ACK

meshcollider commented at 10:52 am on April 17, 2020: contributor

utACK 991a38ce9e9c19ec78139d2988b2abd7c9983519

DrahtBot added the label Needs rebase on Apr 27, 2020

Sjors force-pushed on Apr 27, 2020

Sjors commented at 2:24 pm on April 27, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Apr 27, 2020

DrahtBot added the label Needs rebase on May 1, 2020

Sjors force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

Sjors force-pushed on May 4, 2020

Sjors force-pushed on May 18, 2020

achow101 commented at 7:54 pm on June 11, 2020: member

ACK 74992fd8cbe6f409bd1dad6c064deb5428509472

Sjors requested review from meshcollider on Jun 12, 2020

promag commented at 9:28 pm on June 12, 2020: member

This made me think that we should fail funding if some coin is already locked?

After reading past discussions and some more careful analysis I now think this is not correct.

With the current approach, a client C1 must unlock the unspents to manually use them. But right after lockunspent true ... some other client C2 can fund/send and and take C1 unspents, and C1 will error.

So I think the “is locked” check should be used only for automatic coin selection, not for those explicitly set.

This means that: a) the client handles concurrency elsewhere and doesn’t use locked unspents b) the client locks unspents beforehand and then use them by funding transactions with those as inputs - and also sets lockUnspents to grab the new ones c) the user makes consecutive fundings with lockUnspents set.

In other words, we can assume that the client that funds a transaction with locked inputs is the one that locked them in the first place.

The c) case above is relevant - with the current approach it wouldn’t be possible because between each funding iteration all inputs would have to be unlocked.

Lastly, the option lockUnspents when true should always lock all inputs - the caller will “own” all inputs (existing inputs whether locked or not and the new ones). This was the original change.

Sorry @Sjors for the misdirection 😞

meshcollider added this to the "PRs" column in a project

in doc/release-notes-18244.md:5 in 74992fd8cb outdated

0@@ -0,0 +1,8 @@
1+Updated RPCs
2+------------
3+
4+- `fundrawtransaction` and `walletcreatefundedpsbt` now abort if a manually selected
5+  coin is locked. It must be manually unlocked with `lockunspent` before use.

jonatack commented at 10:10 am on June 23, 2020:

suggest stating lockunspent is an RPC, in contrast with the lockUnspents argument mentioned right after

0  coin is locked; coins must be manually unlocked with RPC `lockunspent` before use.

in doc/release-notes-18244.md:7 in 74992fd8cb outdated

0@@ -0,0 +1,8 @@
1+Updated RPCs
2+------------
3+
4+- `fundrawtransaction` and `walletcreatefundedpsbt` now abort if a manually selected
5+  coin is locked. It must be manually unlocked with `lockunspent` before use.
6+  Previously they would merely avoid using locked coins in automatic
7+  coin selection. When used with `lockUnspents`, these two RPC methods now lock manually

jonatack commented at 10:11 am on June 23, 2020:

suggest statinglockUnspents is an argument, in contrast to the lockunspent RPC mentioned right before

0  coin selection. When used with the `lockUnspents` argument, these two RPCs now lock manually

in src/wallet/rpcwallet.cpp:2125 in 74992fd8cb outdated

2121@@ -2122,6 +2122,7 @@ static UniValue lockunspent(const JSONRPCRequest& request)
2122                 "Temporarily lock (unlock=false) or unlock (unlock=true) specified transaction outputs.\n"
2123                 "If no transaction outputs are specified when unlocking then all current locked transaction outputs are unlocked.\n"
2124                 "A locked transaction output will not be chosen by automatic coin selection, when spending bitcoins.\n"
2125+                "With manual coin selection a locked transaction output must be unlocked before use."

jonatack commented at 10:24 am on June 23, 2020:

missing newline at end of string here

current output is a too-long line and missing space between sentences: before use.Locks are stored:

0With manual coin selection a locked transaction output must be unlocked before use.Locks are stored in memory only. Nodes start with zero locked outputs, and the locked output list
1is always cleared (by virtue of process exit) when a node stops or fails.

in src/wallet/rpcwallet.cpp:2053 in 74992fd8cb outdated

2121@@ -2122,6 +2122,7 @@ static UniValue lockunspent(const JSONRPCRequest& request)
2122                 "Temporarily lock (unlock=false) or unlock (unlock=true) specified transaction outputs.\n"
2123                 "If no transaction outputs are specified when unlocking then all current locked transaction outputs are unlocked.\n"
2124                 "A locked transaction output will not be chosen by automatic coin selection, when spending bitcoins.\n"
2125+                "With manual coin selection a locked transaction output must be unlocked before use."
2126                 "Locks are stored in memory only. Nodes start with zero locked outputs, and the locked output list\n"
2127                 "is always cleared (by virtue of process exit) when a node stops or fails.\n"
2128                 "Also see the listunspent call\n",

jonatack commented at 10:26 am on June 23, 2020:

while touching this help, can you add the missing full stop between “call” and the newline? e.g. call.\n

jonatack commented at 3:40 pm on June 23, 2020: member

Almost-ACK, review/tested, modulo I need to think about #18244#pullrequestreview-430047339.

Sjors commented at 5:48 pm on June 25, 2020: member

I’ll await the above discussion. Will address nits if that leads to a change.

[rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins

Previously only automatically selected coins were locked when lockUnspents is set.
It now also locks selected coins.

6d1f51343c

Sjors force-pushed on Aug 7, 2020

Sjors commented at 12:13 pm on August 7, 2020: member

Based on the above discussion I changed this PR again, it now does the following:

lock all inputs for a transaction, rather than just the automatically selected ones
unchanged behaviour: ignore lock on manually coins (with some tests)

I updated the description to clarify the discussion a bit.

Sjors renamed this:
~~rpc: fundrawtransaction and walletcreatefundedpsbt respect locks even with manual coin selection~~
rpc: fundrawtransaction and walletcreatefundedpsbt also lock manually selected coins
on Aug 7, 2020

promag commented at 12:19 pm on August 7, 2020: member

Thanks @Sjors will review again.

meshcollider commented at 9:06 pm on August 13, 2020: contributor

Code review ACK 6d1f51343cf11b07cd401fbd0c5bc3603e185a0e

MarcoFalke removed the label Tests on Aug 27, 2020

Sjors requested review from promag on Aug 28, 2020

in src/wallet/rpcwallet.cpp:2050 in 6d1f51343c

2046@@ -2047,6 +2047,7 @@ static UniValue lockunspent(const JSONRPCRequest& request)
2047                 "Temporarily lock (unlock=false) or unlock (unlock=true) specified transaction outputs.\n"
2048                 "If no transaction outputs are specified when unlocking then all current locked transaction outputs are unlocked.\n"
2049                 "A locked transaction output will not be chosen by automatic coin selection, when spending bitcoins.\n"
2050+                "Manually selected coins are automatically unlocked.\n"

promag commented at 11:33 am on August 29, 2020:

This is incorrect, or am I missing something? I mean, all spent coins are unlocked right?

Sjors commented at 12:08 pm on August 30, 2020:

Yes, but that’s not the point. Automatically selection avoids locks, manually selection causes an unlock. This remark protects a user who assumes locked coins can’t be accidentally spend by manually picking them. (it’s like the difference between a Japanese toilet with shoes in front of it indicating it’s occupied, and a Dutch toilets that’s locked like a vault because people just pull the door open to find out)

fjahr commented at 1:51 pm on August 30, 2020:

I think “Locks on manually selected coins are ignored.” would be a more intuitive description of what happens but I understand this to mean the same thing.

promag commented at 11:41 am on August 29, 2020: member

Just want to point out that it’s impossible to prevent 2 clients spend the same manually specified inputs.

One way to fix this is to make each client explicitly lock those inputs, and only proceed if the lock was granted.

in src/wallet/wallet.cpp:2616 in 6d1f51343c

2614-            }
2615         }
2616+        if (lockUnspents) {
2617+            LockCoin(txin.prevout);
2618+        }
2619+

fjahr commented at 11:52 am on August 30, 2020:

mirco-nit if you retouch: empty line seems misplaced

Sjors commented at 12:09 pm on August 30, 2020: member

Client-specific locks sounds fun, but not in this PR :-)

fjahr commented at 1:52 pm on August 30, 2020: member

Code review ACK 6d1f51343cf11b07cd401fbd0c5bc3603e185a0e

meshcollider merged this on Aug 31, 2020

meshcollider closed this on Aug 31, 2020

Sjors deleted the branch on Aug 31, 2020

meshcollider moved this from the "PRs" to the "Done" column in a project

sidhujag referenced this in commit 49daca74b8 on Aug 31, 2020

meshcollider referenced this in commit ffaac6e614 on Sep 15, 2020

sidhujag referenced this in commit 2d9d86f6b8 on Sep 15, 2020

deadalnix referenced this in commit 748b4d01fe on Sep 19, 2021

DrahtBot locked this on Feb 15, 2022

rpc: fundrawtransaction and walletcreatefundedpsbt also lock manually selected coins #18244

Conflicts