Merge #18654: rpc: separate bumpfee's psbt creation function into psbtbumpfee

79d6332e9e moveonly: Fix indentation in bumpfee RPC (Andrew Chow)
431071c28a Hide bumpfee's psbt creation behavior behind -deprecatedrpc (Andrew Chow)
4638224f64 Add psbtbumpfee RPC (Andrew Chow)

Pull request description:

  Adds a new RPC `psbtbumpfee` which always creates a psbt. `bumpfee` will then only be able to create and broadcast fee bumping transactions instead of changing its behavior based on `IsWalletSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)`.

  Split from #18627

ACKs for top commit:
  Sjors:
    re-utACK 79d6332
  meshcollider:
    utACK 79d6332e9e
  fjahr:
    Code review ACK 79d6332e9e

Tree-SHA512: 1c92c4b4461bb30e78be3ee73165f624398ef33996ce36043b61a8931be667030d0fca12fd0b30097b78c56e4e9092c69582b237cbdac51d56f6be23d8c0f1bb

src/rpc/client.cpp

@@ -151,6 +151,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "getmempoolancestors", 1, "verbose" },
     { "getmempooldescendants", 1, "verbose" },
     { "bumpfee", 1, "options" },
+    { "psbtbumpfee", 1, "options" },
     { "logging", 0, "include" },
     { "logging", 1, "exclude" },
     { "disconnectnode", 1, "nodeid" },

src/wallet/rpcwallet.cpp

@@ -3222,59 +3222,73 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request)
 
 static UniValue bumpfee(const JSONRPCRequest& request)
 {
-            RPCHelpMan{"bumpfee",
+    bool want_psbt = request.strMethod == "psbtbumpfee";
-                "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
+
-                "An opt-in RBF transaction with the given txid must be in the wallet.\n"
+    RPCHelpMan{request.strMethod,
-                "The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist.\n"
+        "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
-                "All inputs in the original transaction will be included in the replacement transaction.\n"
+        + std::string(want_psbt ? "Returns a PSBT instead of creating and signing a new transaction.\n" : "") +
-                "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
+        "An opt-in RBF transaction with the given txid must be in the wallet.\n"
-                "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
+        "The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist.\n"
-                "The user can specify a confirmation target for estimatesmartfee.\n"
+        "All inputs in the original transaction will be included in the replacement transaction.\n"
-                "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction.\n"
+        "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
-                "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
+        "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
-                "returned by getnetworkinfo) to enter the node's mempool.\n",
+        "The user can specify a confirmation target for estimatesmartfee.\n"
+        "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction.\n"
+        "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
+        "returned by getnetworkinfo) to enter the node's mempool.\n",
+        {
+            {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The txid to be bumped"},
+            {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
                 {
-                    {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The txid to be bumped"},
+                    {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"},
-                    {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
+                    {"fee_rate", RPCArg::Type::NUM, /* default */ "fall back to 'conf_target'", "fee rate (NOT total fee) to pay, in " + CURRENCY_UNIT + " per kB\n"
-                        {
+    "                         Specify a fee rate instead of relying on the built-in fee estimator.\n"
-                            {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"},
+                             "Must be at least 0.0001 " + CURRENCY_UNIT + " per kB higher than the current transaction fee rate.\n"},
-                            {"fee_rate", RPCArg::Type::NUM, /* default */ "fall back to 'conf_target'", "fee rate (NOT total fee) to pay, in " + CURRENCY_UNIT + " per kB\n"
+                    {"replaceable", RPCArg::Type::BOOL, /* default */ "true", "Whether the new transaction should still be\n"
-            "                         Specify a fee rate instead of relying on the built-in fee estimator.\n"
+    "                         marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
-                                     "Must be at least 0.0001 " + CURRENCY_UNIT + " per kB higher than the current transaction fee rate.\n"},
+    "                         be left unchanged from the original. If false, any input sequence numbers in the\n"
-                            {"replaceable", RPCArg::Type::BOOL, /* default */ "true", "Whether the new transaction should still be\n"
+    "                         original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"
-            "                         marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
+    "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
-            "                         be left unchanged from the original. If false, any input sequence numbers in the\n"
+    "                         still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
-            "                         original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"
+    "                         are replaceable)."},
-            "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
+                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
-            "                         still be replaceable in practice, for example if it has unconfirmed ancestors which\n"
+    "         \"" + FeeModes("\"\n\"") + "\""},
-            "                         are replaceable)."},
-                            {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
-            "         \"" + FeeModes("\"\n\"") + "\""},
-                        },
-                        "options"},
                 },
-                RPCResult{
+                "options"},
-                    RPCResult::Type::OBJ, "", "", {
+        },
-                        {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private keys are disabled."},
+        RPCResult{
-                        {RPCResult::Type::STR_HEX, "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."},
+            RPCResult::Type::OBJ, "", "", Cat(Cat<std::vector<RPCResult>>(
-                        {RPCResult::Type::STR_AMOUNT, "origfee", "The fee of the replaced transaction."},
+            {
-                        {RPCResult::Type::STR_AMOUNT, "fee", "The fee of the new transaction."},
+                {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction." + std::string(want_psbt ? "" : " Only returned when wallet private keys are disabled. (DEPRECATED)")},
-                        {RPCResult::Type::ARR, "errors", "Errors encountered during processing (may be empty).",
+            },
-                        {
+            want_psbt ? std::vector<RPCResult>{} : std::vector<RPCResult>{{RPCResult::Type::STR_HEX, "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."}}
-                            {RPCResult::Type::STR, "", ""},
+            ),
-                        }},
+            {
-                    }
+                {RPCResult::Type::STR_AMOUNT, "origfee", "The fee of the replaced transaction."},
-                },
+                {RPCResult::Type::STR_AMOUNT, "fee", "The fee of the new transaction."},
-                RPCExamples{
+                {RPCResult::Type::ARR, "errors", "Errors encountered during processing (may be empty).",
-            "\nBump the fee, get the new transaction\'s txid\n" +
+                {
-                    HelpExampleCli("bumpfee", "<txid>")
+                    {RPCResult::Type::STR, "", ""},
-                },
+                }},
-            }.Check(request);
+            })
+        },
+        RPCExamples{
+    "\nBump the fee, get the new transaction\'s" + std::string(want_psbt ? "psbt" : "txid") + "\n" +
+            HelpExampleCli(request.strMethod, "<txid>")
+        },
+    }.Check(request);
 
     std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
     if (!wallet) return NullUniValue;
     CWallet* const pwallet = wallet.get();
 
+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !want_psbt) {
+        if (!pwallet->chain().rpcEnableDeprecated("bumpfee")) {
+            throw JSONRPCError(RPC_METHOD_DEPRECATED, "Using bumpfee with wallets that have private keys disabled is deprecated. Use psbtbumpfee instead or restart bitcoind with -deprecatedrpc=bumpfee. This functionality will be removed in 0.22");
+        }
+        want_psbt = true;
+    }
+
     RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VOBJ});
     uint256 hash(ParseHashV(request.params[0], "txid"));
 
@@ -3359,7 +3373,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
 
     // If wallet private keys are enabled, return the new transaction id,
     // otherwise return the base64-encoded unsigned PSBT of the new transaction.
-    if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+    if (!want_psbt) {
         if (!feebumper::SignTransaction(*pwallet, mtx)) {
             throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
         }
@@ -3392,6 +3406,11 @@ static UniValue bumpfee(const JSONRPCRequest& request)
     return result;
 }
 
+static UniValue psbtbumpfee(const JSONRPCRequest& request)
+{
+    return bumpfee(request);
+}
+
 UniValue rescanblockchain(const JSONRPCRequest& request)
 {
             RPCHelpMan{"rescanblockchain",
@@ -4137,6 +4156,7 @@ static const CRPCCommand commands[] =
     { "wallet",             "addmultisigaddress",               &addmultisigaddress,            {"nrequired","keys","label","address_type"} },
     { "wallet",             "backupwallet",                     &backupwallet,                  {"destination"} },
     { "wallet",             "bumpfee",                          &bumpfee,                       {"txid", "options"} },
+    { "wallet",             "psbtbumpfee",                      &psbtbumpfee,                   {"txid", "options"} },
     { "wallet",             "createwallet",                     &createwallet,                  {"wallet_name", "disable_private_keys", "blank", "passphrase", "avoid_reuse", "descriptors"} },
     { "wallet",             "dumpprivkey",                      &dumpprivkey,                   {"address"}  },
     { "wallet",             "dumpwallet",                       &dumpwallet,                    {"filename"} },

test/functional/rpc_deprecated.py

@@ -4,13 +4,13 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test deprecation of RPC calls."""
 from test_framework.test_framework import BitcoinTestFramework
-# from test_framework.util import assert_raises_rpc_error
+from test_framework.util import assert_raises_rpc_error, find_vout_for_address
 
 class DeprecatedRpcTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
         self.setup_clean_chain = True
-        self.extra_args = [[], []]
+        self.extra_args = [[], ['-deprecatedrpc=bumpfee']]
 
     def run_test(self):
         # This test should be used to verify correct behaviour of deprecated
@@ -23,7 +23,38 @@ class DeprecatedRpcTest(BitcoinTestFramework):
         # self.log.info("Test generate RPC")
         # assert_raises_rpc_error(-32, 'The wallet generate rpc method is deprecated', self.nodes[0].rpc.generate, 1)
         # self.nodes[1].generate(1)
-        self.log.info("No tested deprecated RPC methods")
+
+        if self.is_wallet_compiled():
+            self.log.info("Test bumpfee RPC")
+            self.nodes[0].generate(101)
+            self.nodes[0].createwallet(wallet_name='nopriv', disable_private_keys=True)
+            noprivs0 = self.nodes[0].get_wallet_rpc('nopriv')
+            w0 = self.nodes[0].get_wallet_rpc('')
+            self.nodes[1].createwallet(wallet_name='nopriv', disable_private_keys=True)
+            noprivs1 = self.nodes[1].get_wallet_rpc('nopriv')
+
+            address = w0.getnewaddress()
+            desc = w0.getaddressinfo(address)['desc']
+            change_addr = w0.getrawchangeaddress()
+            change_desc = w0.getaddressinfo(change_addr)['desc']
+            txid = w0.sendtoaddress(address=address, amount=10)
+            vout = find_vout_for_address(w0, txid, address)
+            self.nodes[0].generate(1)
+            rawtx = w0.createrawtransaction([{'txid': txid, 'vout': vout}], {w0.getnewaddress(): 5}, 0, True)
+            rawtx = w0.fundrawtransaction(rawtx, {'changeAddress': change_addr})
+            signed_tx = w0.signrawtransactionwithwallet(rawtx['hex'])['hex']
+
+            noprivs0.importmulti([{'desc': desc, 'timestamp': 0}, {'desc': change_desc, 'timestamp': 0, 'internal': True}])
+            noprivs1.importmulti([{'desc': desc, 'timestamp': 0}, {'desc': change_desc, 'timestamp': 0, 'internal': True}])
+
+            txid = w0.sendrawtransaction(signed_tx)
+            self.sync_all()
+
+            assert_raises_rpc_error(-32, 'Using bumpfee with wallets that have private keys disabled is deprecated. Use psbtbumpfee instead or restart bitcoind with -deprecatedrpc=bumpfee. This functionality will be removed in 0.22', noprivs0.bumpfee, txid)
+            bumped_psbt = noprivs1.bumpfee(txid)
+            assert 'psbt' in bumped_psbt
+        else:
+            self.log.info("No tested deprecated RPC methods")
 
 if __name__ == '__main__':
     DeprecatedRpcTest().main()

test/functional/wallet_bumpfee.py

@@ -123,13 +123,19 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
     self.sync_mempools((rbf_node, peer_node))
     assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
     if mode == "fee_rate":
+        bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"fee_rate": NORMAL})
         bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": NORMAL})
     else:
+        bumped_psbt = rbf_node.psbtbumpfee(rbfid)
         bumped_tx = rbf_node.bumpfee(rbfid)
     assert_equal(bumped_tx["errors"], [])
     assert bumped_tx["fee"] > -rbftx["fee"]
     assert_equal(bumped_tx["origfee"], -rbftx["fee"])
     assert "psbt" not in bumped_tx
+    assert_equal(bumped_psbt["errors"], [])
+    assert bumped_psbt["fee"] > -rbftx["fee"]
+    assert_equal(bumped_psbt["origfee"], -rbftx["fee"])
+    assert "psbt" in bumped_psbt
     # check that bumped_tx propagates, original tx was evicted and has a wallet conflict
     self.sync_mempools((rbf_node, peer_node))
     assert bumped_tx["txid"] in rbf_node.getrawmempool()
@@ -391,7 +397,7 @@ def test_watchonly_psbt(self, peer_node, rbf_node, dest_address):
     assert_equal(len(watcher.decodepsbt(psbt)["tx"]["vin"]), 1)
 
     # Bump fee, obnoxiously high to add additional watchonly input
-    bumped_psbt = watcher.bumpfee(original_txid, {"fee_rate": HIGH})
+    bumped_psbt = watcher.psbtbumpfee(original_txid, {"fee_rate": HIGH})
     assert_greater_than(len(watcher.decodepsbt(bumped_psbt['psbt'])["tx"]["vin"]), 1)
     assert "txid" not in bumped_psbt
     assert_equal(bumped_psbt["origfee"], -watcher.gettransaction(original_txid)["fee"])