Merge #17264: rpc: set default bip32derivs to true for psbt methods

5bad792 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)

Pull request description:

  In bitcoin/bitcoin#13557 (review) I recommended not including bip32 deriviation by default in PSBTs:

  > _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
  >
  > Adding `bip32_derivs` should probably be opt-in.

  In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.

  More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).

ACKs for top commit:
  instagibbs:
    utACK bitcoin/bitcoin@5bad792
  jonatack:
    ACK 5bad792 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
  meshcollider:
    utACK 5bad792

Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd

Author: meshcollider

doc/release-notes-17264.md

@@ -0,0 +1,4 @@
+Updated RPCs
+------------
+
+- `walletprocesspsbt` and `walletcreatefundedpsbt` now include BIP 32 derivation paths by default for public keys if we know them. This can be disabled by setting `bip32derivs` to `false`.

src/wallet/psbtwallet.h

@@ -27,6 +27,6 @@ NODISCARD TransactionError FillPSBT(const CWallet* pwallet,
               bool& complete,
               int sighash_type = 1 /* SIGHASH_ALL */,
               bool sign = true,
-              bool bip32derivs = false);
+              bool bip32derivs = true);
 
 #endif // BITCOIN_WALLET_PSBTWALLET_H

src/wallet/rpcwallet.cpp

@@ -4066,7 +4066,7 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request)
             "       \"ALL|ANYONECANPAY\"\n"
             "       \"NONE|ANYONECANPAY\"\n"
             "       \"SINGLE|ANYONECANPAY\""},
-                    {"bip32derivs", RPCArg::Type::BOOL, /* default */ "false", "If true, includes the BIP 32 derivation paths for public keys if we know them"},
+                    {"bip32derivs", RPCArg::Type::BOOL, /* default */ "true", "Include BIP 32 derivation paths for public keys if we know them"},
                 },
                 RPCResult{
             "{                            (json object)\n"
@@ -4093,7 +4093,7 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request)
 
     // Fill transaction with our data and also sign
     bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();
-    bool bip32derivs = request.params[3].isNull() ? false : request.params[3].get_bool();
+    bool bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool();
     bool complete = true;
     const TransactionError err = FillPSBT(pwallet, psbtx, complete, nHashType, sign, bip32derivs);
     if (err != TransactionError::OK) {
@@ -4176,7 +4176,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
                             "         \"CONSERVATIVE\""},
                         },
                         "options"},
-                    {"bip32derivs", RPCArg::Type::BOOL, /* default */ "false", "If true, includes the BIP 32 derivation paths for public keys if we know them"},
+                    {"bip32derivs", RPCArg::Type::BOOL, /* default */ "true", "Include BIP 32 derivation paths for public keys if we know them"},
                 },
                 RPCResult{
                             "{\n"
@@ -4215,7 +4215,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
     PartiallySignedTransaction psbtx(rawTx);
 
     // Fill transaction with out data but don't sign
-    bool bip32derivs = request.params[4].isNull() ? false : request.params[4].get_bool();
+    bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool();
     bool complete = true;
     const TransactionError err = FillPSBT(pwallet, psbtx, complete, 1, false, bip32derivs);
     if (err != TransactionError::OK) {

test/functional/rpc_psbt.py

@@ -193,12 +193,20 @@ def run_test(self):
         psbt_orig = self.nodes[0].createpsbt([{"txid":txid1,  "vout":vout1}, {"txid":txid2, "vout":vout2}], {self.nodes[0].getnewaddress():25.999})
 
         # Update psbts, should only have data for one input and not the other
-        psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig)['psbt']
+        psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig, False, "ALL")['psbt']
         psbt1_decoded = self.nodes[0].decodepsbt(psbt1)
         assert psbt1_decoded['inputs'][0] and not psbt1_decoded['inputs'][1]
-        psbt2 = self.nodes[2].walletprocesspsbt(psbt_orig)['psbt']
+        # Check that BIP32 path was added
+        assert "bip32_derivs" in psbt1_decoded['inputs'][0]
+        psbt2 = self.nodes[2].walletprocesspsbt(psbt_orig, False, "ALL", False)['psbt']
         psbt2_decoded = self.nodes[0].decodepsbt(psbt2)
         assert not psbt2_decoded['inputs'][0] and psbt2_decoded['inputs'][1]
+        # Check that BIP32 paths were not added
+        assert "bip32_derivs" not in psbt2_decoded['inputs'][1]
+
+        # Sign PSBTs (workaround issue #18039)
+        psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig)['psbt']
+        psbt2 = self.nodes[2].walletprocesspsbt(psbt_orig)['psbt']
 
         # Combine, finalize, and send the psbts
         combined = self.nodes[0].combinepsbt([psbt1, psbt2])
@@ -231,16 +239,18 @@ def run_test(self):
         # 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"]:
+        for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
             assert_equal(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
+            assert "bip32_derivs" in psbt_in
         assert_equal(decoded_psbt["tx"]["locktime"], 0)
 
         # Same construction without optional arguments, for a node with -walletrbf=0
         unspent1 = self.nodes[1].listunspent()[0]
         psbtx_info = self.nodes[1].walletcreatefundedpsbt([{"txid":unspent1["txid"], "vout":unspent1["vout"]}], [{self.nodes[2].getnewaddress():unspent1["amount"]+1}], block_height)
         decoded_psbt = self.nodes[1].decodepsbt(psbtx_info["psbt"])
-        for tx_in in decoded_psbt["tx"]["vin"]:
+        for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
             assert_greater_than(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
+            assert "bip32_derivs" in psbt_in
 
         # Make sure change address wallet does not have P2SH innerscript access to results in success
         # when attempting BnB coin selection