Merge #16378: The ultimate send RPC

92326d89766155a792254d30a9962251b8fc7799 [rpc] add send method (Sjors Provoost)
2c2a1445dc9d22c9d729b8301c8b3f54195bcfcf [rpc] add snake case aliases for transaction methods (Sjors Provoost)
1bc8d0fd5906bc9637d513cd193a1f47ad94da28 [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost)

Pull request description:

  `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have:
  * manual coin selection
  * outputting a PSBT (it was controversial to add this, see #18201)
  * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`)

  At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases.

  This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format.

  Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546).

  For `bitcoin-cli` users, it tries to keep the simplest use case easy to use:

  ```sh
  bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
  ```

  This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address.

  Depends on:
  - [x] #16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`)
  - [x] #11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`)
  - [x] #18244 (`[rpc] have lockUnspents also lock manually selected coins`)

ACKs for top commit:
  meshcollider:
    Light re-utACK 92326d89766155a792254d30a9962251b8fc7799
  achow101:
    ACK 92326d89766155a792254d30a9962251b8fc7799 Reviewed code and test, ran tests.
  kallewoof:
    utACK 92326d89766155a792254d30a9962251b8fc7799

Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f

4 files changed, 344 insertions, 1 deletions

diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
index 2a0971b808..6dcbec2714 100755
--- a/test/functional/rpc_fundrawtransaction.py
+++ b/test/functional/rpc_fundrawtransaction.py
@@ -224,7 +224,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
-        assert_raises_rpc_error(-5, "changeAddress must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'})
+        assert_raises_rpc_error(-5, "Change address must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'})
 
     def test_valid_change_address(self):
         self.log.info("Test fundrawtxn with a provided change address")
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 1c7dc98d16..781a49dfac 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -94,6 +94,9 @@ class PSBTTest(BitcoinTestFramework):
         psbtx1 = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():90}, 0, {"add_inputs": True})['psbt']
         assert_equal(len(self.nodes[0].decodepsbt(psbtx1)['tx']['vin']), 2)
 
+        # Inputs argument can be null
+        self.nodes[0].walletcreatefundedpsbt(None, {self.nodes[2].getnewaddress():10})
+
         # Node 1 should not be able to add anything to it but still return the psbtx same as before
         psbtx = self.nodes[1].walletprocesspsbt(psbtx1)['psbt']
         assert_equal(psbtx1, psbtx)
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 578afe5f30..a3e160f12e 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -225,6 +225,7 @@ BASE_SCRIPTS = [
     'rpc_estimatefee.py',
     'rpc_getblockstats.py',
     'wallet_create_tx.py',
+    'wallet_send.py',
     'p2p_fingerprint.py',
     'feature_uacomment.py',
     'wallet_coinbase_category.py',
diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py
new file mode 100755
index 0000000000..b64d2030a4
--- /dev/null
+++ b/test/functional/wallet_send.py
@@ -0,0 +1,339 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test the send RPC command."""
+
+from decimal import Decimal, getcontext
+from test_framework.authproxy import JSONRPCException
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+    assert_fee_amount,
+    assert_greater_than,
+    assert_raises_rpc_error
+)
+
+class WalletSendTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        # whitelist all peers to speed up tx relay / mempool sync
+        self.extra_args = [
+            ["-whitelist=127.0.0.1","-walletrbf=1"],
+            ["-whitelist=127.0.0.1","-walletrbf=1"],
+        ]
+        getcontext().prec = 8 # Satoshi precision for Decimal
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
+                  arg_conf_target=None, arg_estimate_mode=None,
+                  conf_target=None, estimate_mode=None, add_to_wallet=None,psbt=None,
+                  inputs=None,add_inputs=None,change_address=None,change_position=None,change_type=None,
+                  include_watching=None,locktime=None,lock_unspents=None,replaceable=None,subtract_fee_from_outputs=None,
+                  expect_error=None):
+        assert (amount is None) != (data is None)
+
+        from_balance_before = from_wallet.getbalance()
+        if to_wallet is None:
+            assert amount is None
+        else:
+            to_untrusted_pending_before = to_wallet.getbalances()["mine"]["untrusted_pending"]
+
+        if amount:
+            dest = to_wallet.getnewaddress()
+            outputs = {dest: amount}
+        else:
+            outputs = {"data": data}
+
+        # Construct options dictionary
+        options = {}
+        if add_to_wallet is not None:
+            options["add_to_wallet"] = add_to_wallet
+        else:
+            if psbt:
+                add_to_wallet = False
+            else:
+                add_to_wallet = from_wallet.getwalletinfo()["private_keys_enabled"] # Default value
+        if psbt is not None:
+            options["psbt"] = psbt
+        if conf_target is not None:
+            options["conf_target"] = conf_target
+        if estimate_mode is not None:
+            options["estimate_mode"] = estimate_mode
+        if inputs is not None:
+            options["inputs"] = inputs
+        if add_inputs is not None:
+            options["add_inputs"] = add_inputs
+        if change_address is not None:
+            options["change_address"] = change_address
+        if change_position is not None:
+            options["change_position"] = change_position
+        if change_type is not None:
+            options["change_type"] = change_type
+        if include_watching is not None:
+            options["include_watching"] = include_watching
+        if locktime is not None:
+            options["locktime"] = locktime
+        if lock_unspents is not None:
+            options["lock_unspents"] = lock_unspents
+        if replaceable is None:
+            replaceable = True # default
+        else:
+            options["replaceable"] = replaceable
+        if subtract_fee_from_outputs is not None:
+            options["subtract_fee_from_outputs"] = subtract_fee_from_outputs
+
+        if len(options.keys()) == 0:
+            options = None
+
+        if expect_error is None:
+            res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options)
+        else:
+            try:
+                assert_raises_rpc_error(expect_error[0],expect_error[1],from_wallet.send,
+                                        outputs=outputs,conf_target=arg_conf_target,estimate_mode=arg_estimate_mode,options=options)
+            except AssertionError:
+                # Provide debug info if the test fails
+                self.log.error("Unexpected successful result:")
+                self.log.error(options)
+                res = from_wallet.send(outputs=outputs,conf_target=arg_conf_target,estimate_mode=arg_estimate_mode,options=options)
+                self.log.error(res)
+                if "txid" in res and add_to_wallet:
+                    self.log.error("Transaction details:")
+                    try:
+                        tx = from_wallet.gettransaction(res["txid"])
+                        self.log.error(tx)
+                        self.log.error("testmempoolaccept (transaction may already be in mempool):")
+                        self.log.error(from_wallet.testmempoolaccept([tx["hex"]]))
+                    except JSONRPCException as exc:
+                        self.log.error(exc)
+
+                raise
+
+            return
+
+        if locktime:
+            return res
+
+        if from_wallet.getwalletinfo()["private_keys_enabled"] and not include_watching:
+            assert_equal(res["complete"], True)
+            assert "txid" in res
+        else:
+            assert_equal(res["complete"], False)
+            assert not "txid" in res
+            assert "psbt" in res
+
+        if add_to_wallet and not include_watching:
+            # Ensure transaction exists in the wallet:
+            tx = from_wallet.gettransaction(res["txid"])
+            assert tx
+            assert_equal(tx["bip125-replaceable"], "yes" if replaceable else "no")
+            # Ensure transaction exists in the mempool:
+            tx = from_wallet.getrawtransaction(res["txid"],True)
+            assert tx
+            if amount:
+                if subtract_fee_from_outputs:
+                    assert_equal(from_balance_before - from_wallet.getbalance(), amount)
+                else:
+                    assert_greater_than(from_balance_before - from_wallet.getbalance(), amount)
+            else:
+                assert next((out for out in tx["vout"] if out["scriptPubKey"]["asm"] == "OP_RETURN 35"), None)
+        else:
+            assert_equal(from_balance_before, from_wallet.getbalance())
+
+        if to_wallet:
+            self.sync_mempools()
+            if add_to_wallet:
+                if not subtract_fee_from_outputs:
+                    assert_equal(to_wallet.getbalances()["mine"]["untrusted_pending"], to_untrusted_pending_before + Decimal(amount if amount else 0))
+            else:
+                assert_equal(to_wallet.getbalances()["mine"]["untrusted_pending"], to_untrusted_pending_before)
+
+        return res
+
+    def run_test(self):
+        self.log.info("Setup wallets...")
+        # w0 is a wallet with coinbase rewards
+        w0 = self.nodes[0].get_wallet_rpc("")
+        # w1 is a regular wallet
+        self.nodes[1].createwallet(wallet_name="w1")
+        w1 = self.nodes[1].get_wallet_rpc("w1")
+        # w2 contains the private keys for w3
+        self.nodes[1].createwallet(wallet_name="w2")
+        w2 = self.nodes[1].get_wallet_rpc("w2")
+        # w3 is a watch-only wallet, based on w2
+        self.nodes[1].createwallet(wallet_name="w3",disable_private_keys=True)
+        w3 = self.nodes[1].get_wallet_rpc("w3")
+        for _ in range(3):
+            a2_receive = w2.getnewaddress()
+            a2_change = w2.getrawchangeaddress() # doesn't actually use change derivation
+            res = w3.importmulti([{
+                "desc": w2.getaddressinfo(a2_receive)["desc"],
+                "timestamp": "now",
+                "keypool": True,
+                "watchonly": True
+            },{
+                "desc": w2.getaddressinfo(a2_change)["desc"],
+                "timestamp": "now",
+                "keypool": True,
+                "internal": True,
+                "watchonly": True
+            }])
+            assert_equal(res, [{"success": True}, {"success": True}])
+
+        w0.sendtoaddress(a2_receive, 10) # fund w3
+        self.nodes[0].generate(1)
+        self.sync_blocks()
+
+        # w4 has private keys enabled, but only contains watch-only keys (from w2)
+        self.nodes[1].createwallet(wallet_name="w4",disable_private_keys=False)
+        w4 = self.nodes[1].get_wallet_rpc("w4")
+        for _ in range(3):
+            a2_receive = w2.getnewaddress()
+            res = w4.importmulti([{
+                "desc": w2.getaddressinfo(a2_receive)["desc"],
+                "timestamp": "now",
+                "keypool": False,
+                "watchonly": True
+            }])
+            assert_equal(res, [{"success": True}])
+
+        w0.sendtoaddress(a2_receive, 10) # fund w4
+        self.nodes[0].generate(1)
+        self.sync_blocks()
+
+        self.log.info("Send to address...")
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1)
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=True)
+
+        self.log.info("Don't broadcast...")
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False)
+        assert(res["hex"])
+
+        self.log.info("Return PSBT...")
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, psbt=True)
+        assert(res["psbt"])
+
+        self.log.info("Create transaction that spends to address, but don't broadcast...")
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False)
+        # conf_target & estimate_mode can be set as argument or option
+        res1 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical", add_to_wallet=False)
+        res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=1, estimate_mode="economical", add_to_wallet=False)
+        assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"],
+                     self.nodes[1].decodepsbt(res2["psbt"])["fee"])
+        # but not at the same time
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical",
+                       conf_target=1, estimate_mode="economical", add_to_wallet=False, expect_error=(-8,"Use either conf_target and estimate_mode or the options dictionary to control fee rate"))
+
+        self.log.info("Create PSBT from watch-only wallet w3, sign with w2...")
+        res = self.test_send(from_wallet=w3, to_wallet=w1, amount=1)
+        res = w2.walletprocesspsbt(res["psbt"])
+        assert res["complete"]
+
+        self.log.info("Create PSBT from wallet w4 with watch-only keys, sign with w2...")
+        self.test_send(from_wallet=w4, to_wallet=w1, amount=1, expect_error=(-4, "Insufficient funds"))
+        res = self.test_send(from_wallet=w4, to_wallet=w1, amount=1, include_watching=True, add_to_wallet=False)
+        res = w2.walletprocesspsbt(res["psbt"])
+        assert res["complete"]
+
+        self.log.info("Create OP_RETURN...")
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1)
+        self.test_send(from_wallet=w0, data="Hello World", expect_error=(-8, "Data must be hexadecimal string (not 'Hello World')"))
+        self.test_send(from_wallet=w0, data="23")
+        res = self.test_send(from_wallet=w3, data="23")
+        res = w2.walletprocesspsbt(res["psbt"])
+        assert res["complete"]
+
+        self.log.info("Set fee rate...")
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=2, estimate_mode="sat/b", add_to_wallet=False)
+        fee = self.nodes[1].decodepsbt(res["psbt"])["fee"]
+        assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002"))
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode="sat/b",
+                       expect_error=(-3, "Amount out of range"))
+        # Fee rate of 0.1 satoshi per byte should throw an error
+        # TODO: error should say 1.000 sat/b
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="sat/b",
+                       expect_error=(-4, "Fee rate (0.00000100 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)"))
+
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.000001, estimate_mode="BTC/KB",
+                       expect_error=(-4, "Fee rate (0.00000100 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)"))
+
+        # TODO: Return hex if fee rate is below -maxmempool
+        # res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="sat/b", add_to_wallet=False)
+        # assert res["hex"]
+        # hex = res["hex"]
+        # res = self.nodes[0].testmempoolaccept([hex])
+        # assert not res[0]["allowed"]
+        # assert_equal(res[0]["reject-reason"], "...") # low fee
+        # assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.000001"))
+
+        self.log.info("If inputs are specified, do not automatically add more...")
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[], add_to_wallet=False)
+        assert res["complete"]
+        utxo1 = w0.listunspent()[0]
+        assert_equal(utxo1["amount"], 50)
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[utxo1],
+                       expect_error=(-4, "Insufficient funds"))
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[utxo1], add_inputs=False,
+                       expect_error=(-4, "Insufficient funds"))
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=51, inputs=[utxo1], add_inputs=True, add_to_wallet=False)
+        assert res["complete"]
+
+        self.log.info("Manual change address and position...")
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, change_address="not an address",
+                       expect_error=(-5, "Change address must be a valid bitcoin address"))
+        change_address = w0.getnewaddress()
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, change_address=change_address)
+        assert res["complete"]
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, change_address=change_address, change_position=0)
+        assert res["complete"]
+        assert_equal(self.nodes[0].decodepsbt(res["psbt"])["tx"]["vout"][0]["scriptPubKey"]["addresses"], [change_address])
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, change_type="legacy", change_position=0)
+        assert res["complete"]
+        change_address = self.nodes[0].decodepsbt(res["psbt"])["tx"]["vout"][0]["scriptPubKey"]["addresses"][0]
+        assert change_address[0] == "m" or change_address[0] == "n"
+
+        self.log.info("Set lock time...")
+        height = self.nodes[0].getblockchaininfo()["blocks"]
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, locktime=height + 1)
+        assert res["complete"]
+        assert res["txid"]
+        txid = res["txid"]
+        # Although the wallet finishes the transaction, it can't be added to the mempool yet:
+        hex = self.nodes[0].gettransaction(res["txid"])["hex"]
+        res = self.nodes[0].testmempoolaccept([hex])
+        assert not res[0]["allowed"]
+        assert_equal(res[0]["reject-reason"], "non-final")
+        # It shouldn't be confirmed in the next block
+        self.nodes[0].generate(1)
+        assert_equal(self.nodes[0].gettransaction(txid)["confirmations"], 0)
+        # The mempool should allow it now:
+        res = self.nodes[0].testmempoolaccept([hex])
+        assert res[0]["allowed"]
+        # Don't wait for wallet to add it to the mempool:
+        res = self.nodes[0].sendrawtransaction(hex)
+        self.nodes[0].generate(1)
+        assert_equal(self.nodes[0].gettransaction(txid)["confirmations"], 1)
+
+        self.log.info("Lock unspents...")
+        utxo1 = w0.listunspent()[0]
+        assert_greater_than(utxo1["amount"], 1)
+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1], add_to_wallet=False, lock_unspents=True)
+        assert res["complete"]
+        locked_coins = w0.listlockunspent()
+        assert_equal(len(locked_coins), 1)
+        # Locked coins are automatically unlocked when manually selected
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False)
+
+        self.log.info("Replaceable...")
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, replaceable=True)
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, replaceable=False)
+
+        self.log.info("Subtract fee from output")
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, subtract_fee_from_outputs=[0])
+
+
+if __name__ == '__main__':
+    WalletSendTest().main()