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

6d1f513 [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost)

Pull request description:

  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.

ACKs for top commit:
  meshcollider:
    Code review ACK 6d1f513
  fjahr:
    Code review ACK 6d1f513

Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d

Author: meshcollider

doc/release-notes-18244.md

@@ -0,0 +1,7 @@
+Updated RPCs
+------------
+
+- `fundrawtransaction` and `walletcreatefundedpsbt` when used with the `lockUnspents`
+   argument now lock manually selected coins, in addition to automatically selected
+   coins. Note that locked coins are never used in automatic coin selection, but
+   can still be manually selected.

src/wallet/rpcwallet.cpp

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

src/wallet/wallet.cpp

@@ -2582,10 +2582,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
         if (!coinControl.IsSelected(txin.prevout)) {
             tx.vin.push_back(txin);
 
-            if (lockUnspents) {
-                LockCoin(txin.prevout);
-            }
         }
+        if (lockUnspents) {
+            LockCoin(txin.prevout);
+        }
+
     }
 
     return true;

test/functional/rpc_psbt.py

@@ -103,7 +103,16 @@ def run_test(self):
         final_tx = self.nodes[0].finalizepsbt(signed_tx)['hex']
         self.nodes[0].sendrawtransaction(final_tx)
 
-        # Get pubkeys
+        # Manually selected inputs can be locked:
+        assert_equal(len(self.nodes[0].listlockunspent()), 0)
+        utxo1 = self.nodes[0].listunspent()[0]
+        psbtx1 = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():1}, 0,{"lockUnspents": True})["psbt"]
+        assert_equal(len(self.nodes[0].listlockunspent()), 1)
+
+        # Locks are ignored for manually selected inputs
+        self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():1}, 0)
+
+        # Create p2sh, p2wpkh, and p2wsh addresses
         pubkey0 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())['pubkey']
         pubkey1 = self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress())['pubkey']
         pubkey2 = self.nodes[2].getaddressinfo(self.nodes[2].getnewaddress())['pubkey']

test/functional/wallet_basic.py

@@ -135,11 +135,19 @@ def run_test(self):
                                 self.nodes[2].lockunspent, False,
                                 [{"txid": unspent_0["txid"], "vout": 999}])
 
-        # An output should be unlocked when spent
+        # The lock on a manually selected output is ignored
         unspent_0 = self.nodes[1].listunspent()[0]
         self.nodes[1].lockunspent(False, [unspent_0])
         tx = self.nodes[1].createrawtransaction([unspent_0], { self.nodes[1].getnewaddress() : 1 })
-        tx = self.nodes[1].fundrawtransaction(tx)['hex']
+        self.nodes[1].fundrawtransaction(tx,{"lockUnspents": True})
+
+        # fundrawtransaction can lock an input
+        self.nodes[1].lockunspent(True, [unspent_0])
+        assert_equal(len(self.nodes[1].listlockunspent()), 0)
+        tx = self.nodes[1].fundrawtransaction(tx,{"lockUnspents": True})['hex']
+        assert_equal(len(self.nodes[1].listlockunspent()), 1)
+
+        # Send transaction
         tx = self.nodes[1].signrawtransactionwithwallet(tx)["hex"]
         self.nodes[1].sendrawtransaction(tx)
         assert_equal(len(self.nodes[1].listlockunspent()), 0)