Merge bitcoin#13968: [wallet] couple of walletcreatefundedpsbt fixes

faaac5c RPCTypeCheck bip32derivs arg in walletcreatefunded (Gregory Sanders)
1f0c428 QA: add basic walletcreatefunded optional arg test (Gregory Sanders)
1f18d7b walletcreatefundedpsbt: remove duplicate replaceable arg (Gregory Sanders)
2252ec5 Allow ConstructTransaction to not throw error with 0-input txn (Gregory Sanders)

Pull request description:

  1) Previously an empty input argument transaction that is marked for replaceability fails to pass the `SignalsOptInRBF` check right before funding it. Explicitly check for that condition before throwing an error.

  2) The rpc call had two separate `replaceable` arguments, each of which being used in mutually exclusive places. I preserved the `options` version to retain compatability with `fundtransaction`.

Tree-SHA512: 26eb0c9e2d38ea51d11f741d61100223253271a084adadeb7e78c6d4e9004636f089e4273c5bf64a41bd7e9ff795317acf30531cb36aeb0d8db9304b3c8270c3

src/rpc/client.cpp

@@ -113,9 +113,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "walletcreatefundedpsbt", 0, "inputs" },
     { "walletcreatefundedpsbt", 1, "outputs" },
     { "walletcreatefundedpsbt", 2, "locktime" },
-    { "walletcreatefundedpsbt", 3, "replaceable" },
-    { "walletcreatefundedpsbt", 4, "options" },
-    { "walletcreatefundedpsbt", 5, "bip32derivs" },
+    { "walletcreatefundedpsbt", 3, "options" },
+    { "walletcreatefundedpsbt", 4, "bip32derivs" },
     { "walletprocesspsbt", 1, "sign" },
     { "walletprocesspsbt", 3, "bip32derivs" },
     { "createpsbt", 0, "inputs" },

src/rpc/rawtransaction.cpp

@@ -436,7 +436,7 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
         }
     }
 
-    if (!rbf.isNull() && rbfOptIn != SignalsOptInRBF(rawTx)) {
+    if (!rbf.isNull() && rawTx.vin.size() > 0 && rbfOptIn != SignalsOptInRBF(rawTx)) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter combination: Sequence number(s) contradict replaceable option");
     }
 

src/wallet/rpcwallet.cpp

@@ -4643,7 +4643,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
         return NullUniValue;
     }
 
-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 6)
+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 5)
         throw std::runtime_error(
                             "walletcreatefundedpsbt [{\"txid\":\"id\",\"vout\":n},...] [{\"address\":amount},{\"data\":\"hex\"},...] ( locktime ) ( replaceable ) ( options bip32derivs )\n"
                             "\nCreates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"
@@ -4670,9 +4670,8 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
                             "                             accepted as second parameter.\n"
                             "   ]\n"
                             "3. locktime                  (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
-                            "4. replaceable               (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n"
                             "                             Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible.\n"
-                            "5. options                 (object, optional)\n"
+                            "4. options                 (object, optional)\n"
                             "   {\n"
                             "     \"changeAddress\"          (string, optional, default pool address) The bitcoin address to receive the change\n"
                             "     \"changePosition\"         (numeric, optional, default random) The index of the change output\n"
@@ -4694,7 +4693,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
                             "         \"ECONOMICAL\"\n"
                             "         \"CONSERVATIVE\"\n"
                             "   }\n"
-                            "6. bip32derivs                    (boolean, optiona, default=false) If true, includes the BIP 32 derivation paths for public keys if we know them\n"
+                            "5. bip32derivs                    (boolean, optiona, default=false) If true, includes the BIP 32 derivation paths for public keys if we know them\n"
                             "\nResult:\n"
                             "{\n"
                             "  \"psbt\": \"value\",        (string)  The resulting raw transaction (base64-encoded string)\n"
@@ -4710,15 +4709,15 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
         UniValue::VARR,
         UniValueType(), // ARR or OBJ, checked later
         UniValue::VNUM,
-        UniValue::VBOOL,
-        UniValue::VOBJ
+        UniValue::VOBJ,
+        UniValue::VBOOL
         }, true
     );
 
     CAmount fee;
     int change_position;
-    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]);
-    FundTransaction(pwallet, rawTx, fee, change_position, request.params[4]);
+    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]["replaceable"]);
+    FundTransaction(pwallet, rawTx, fee, change_position, request.params[3]);
 
     // Make a blank psbt
     PartiallySignedTransaction psbtx;
@@ -4735,7 +4734,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
     const CTransaction txConst(*psbtx.tx);
 
     // Fill transaction with out data but don't sign
-    bool bip32derivs = request.params[5].isNull() ? false : request.params[5].get_bool();
+    bool bip32derivs = request.params[4].isNull() ? false : request.params[5].get_bool();
     FillPSBT(pwallet, psbtx, &txConst, 1, false, bip32derivs);
 
     // Serialize the PSBT
@@ -4765,7 +4764,7 @@ static const CRPCCommand commands[] =
     //  --------------------- ------------------------          -----------------------         ----------
     { "rawtransactions",    "fundrawtransaction",               &fundrawtransaction,            {"hexstring","options","iswitness"} },
     { "wallet",             "walletprocesspsbt",                &walletprocesspsbt,             {"psbt","sign","sighashtype","bip32derivs"} },
-    { "wallet",             "walletcreatefundedpsbt",           &walletcreatefundedpsbt,        {"inputs","outputs","locktime","replaceable","options","bip32derivs"} },
+    { "wallet",             "walletcreatefundedpsbt",           &walletcreatefundedpsbt,        {"inputs","outputs","locktime","options","bip32derivs"} },
     { "hidden",             "resendwallettransactions",         &resendwallettransactions,      {} },
     { "wallet",             "abandontransaction",               &abandontransaction,            {"txid"} },
     { "wallet",             "abortrescan",                      &abortrescan,                   {} },

test/functional/rpc_psbt.py

@@ -11,6 +11,8 @@
 import json
 import os
 
+MAX_BIP125_RBF_SEQUENCE = 0xfffffffd
+
 # Create one-input, one-output, no-fee transaction:
 class PSBTTest(BitcoinTestFramework):
 
@@ -135,6 +137,33 @@ def run_test(self):
         self.nodes[0].generate(6)
         self.sync_all()
 
+        # Test additional args in walletcreatepsbt
+        # Make sure both pre-included and funded inputs
+        # have the correct sequence numbers based on
+        # replaceable arg
+        block_height = self.nodes[0].getblockcount()
+        unspent = self.nodes[0].listunspent()[0]
+        psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable":True})
+        decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
+        for tx_in in decoded_psbt["tx"]["vin"]:
+           assert_equal(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
+        assert_equal(decoded_psbt["tx"]["locktime"], block_height+2)
+
+        # Same construction with only locktime set
+        psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height)
+        decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
+        for tx_in in decoded_psbt["tx"]["vin"]:
+            assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
+        assert_equal(decoded_psbt["tx"]["locktime"], block_height)
+
+        # Same construction without optional arguments
+        psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}])
+        decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
+        for tx_in in decoded_psbt["tx"]["vin"]:
+            assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
+        assert_equal(decoded_psbt["tx"]["locktime"], 0)
+
+
         # BIP 174 Test Vectors
 
         # Check that unknown values are just passed through