From 3ac60775644be1a0f24f4fea69bec19957f8105c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 4 Jul 2025 15:38:13 +0200 Subject: [PATCH 01/66] wallet: add option to avoid script path spends --- src/common/types.h | 6 ++++++ src/psbt.cpp | 2 +- src/script/sign.cpp | 8 ++++++++ src/script/sign.h | 3 +++ src/wallet/wallet.h | 1 + 5 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/common/types.h b/src/common/types.h index 1ffcd392d609..efcebc9062df 100644 --- a/src/common/types.h +++ b/src/common/types.h @@ -48,6 +48,12 @@ struct PSBTFillOptions { * Whether to fill in bip32 derivation information if available. */ bool bip32_derivs{true}; + + /** + * Only add new Taproot key-path data, and only sign and finalize Taproot + * inputs using the key path. Existing script-path data is left intact. + */ + bool taproot_keypath_only{false}; }; } // namespace common diff --git a/src/psbt.cpp b/src/psbt.cpp index 586fded2a293..f26e619372a1 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -722,7 +722,7 @@ util::Expected SignPSBTInput(const SigningProvider& provider, P if (txdata == nullptr) { sig_complete = ProduceSignature(provider, DUMMY_SIGNATURE_CREATOR, utxo.scriptPubKey, sigdata); } else { - MutableTransactionSignatureCreator creator(tx, index, utxo.nValue, txdata, {.sighash_type = sighash}); + MutableTransactionSignatureCreator creator(tx, index, utxo.nValue, txdata, {.sighash_type = sighash, .taproot_keypath_only = options.taproot_keypath_only}); sig_complete = ProduceSignature(provider, creator, utxo.scriptPubKey, sigdata); } // Verify that a witness signature was produced in case one was required. diff --git a/src/script/sign.cpp b/src/script/sign.cpp index efee5a35fe01..f96f86c91371 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -559,9 +559,13 @@ static bool SignTaproot(const SigningProvider& provider, const BaseSignatureCrea { TaprootSpendData spenddata; TaprootBuilder builder; + const bool taproot_keypath_only{creator.Options().taproot_keypath_only}; // Gather information about this output. if (provider.GetTaprootSpendData(output, spenddata)) { + // Avoid merging newly provided script path data. Existing taproot + // script path fields in sigdata (e.g. from a PSBT) are left intact. + if (taproot_keypath_only) spenddata.scripts.clear(); sigdata.tr_spenddata.Merge(spenddata); } if (provider.GetTaprootBuilder(output, builder)) { @@ -613,6 +617,10 @@ static bool SignTaproot(const SigningProvider& provider, const BaseSignatureCrea } } + // Key path signing failed. In keypath-only mode, stop here instead of + // attempting a script path signature. + if (taproot_keypath_only) return false; + // Try script path spending. std::vector> smallest_result_stack; for (const auto& [key, control_blocks] : sigdata.tr_spenddata.scripts) { diff --git a/src/script/sign.h b/src/script/sign.h index 107abb9e8492..558d7749f3f8 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -33,6 +33,7 @@ struct SignatureData; struct SignOptions { int sighash_type{SIGHASH_DEFAULT}; + bool taproot_keypath_only{false}; }; /** Interface for signature creators. */ @@ -40,6 +41,7 @@ class BaseSignatureCreator { public: virtual ~BaseSignatureCreator() = default; virtual const BaseSignatureChecker& Checker() const =0; + virtual SignOptions Options() const { return {}; } /** Create a singular (non-script) signature. */ virtual bool CreateSig(const SigningProvider& provider, std::vector& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const =0; @@ -65,6 +67,7 @@ class MutableTransactionSignatureCreator : public BaseSignatureCreator MutableTransactionSignatureCreator(const CMutableTransaction& tx LIFETIMEBOUND, unsigned int input_idx, const CAmount& amount, const SignOptions& options); MutableTransactionSignatureCreator(const CMutableTransaction& tx LIFETIMEBOUND, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, const SignOptions& options); const BaseSignatureChecker& Checker() const override { return checker; } + SignOptions Options() const override { return m_options; } bool CreateSig(const SigningProvider& provider, std::vector& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override; bool CreateSchnorrSig(const SigningProvider& provider, std::vector& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const override; std::vector CreateMuSig2Nonce(const SigningProvider& provider, const CPubKey& aggregate_pubkey, const CPubKey& script_pubkey, const CPubKey& part_pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion, const SignatureData& sigdata) const override; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 61ba2dea2634..594f56fcf899 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -133,6 +133,7 @@ inline constexpr bool DEFAULT_WALLET_RBF = true; inline constexpr bool DEFAULT_WALLETBROADCAST = true; inline constexpr bool DEFAULT_DISABLE_WALLET = false; inline constexpr bool DEFAULT_WALLETCROSSCHAIN = false; +inline constexpr bool DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY{false}; //! -maxtxfee default inline constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10}; //! Discourage users to set fees higher than this amount (in satoshis) per kB From a8734b210199d72cad0cb368ca1e2d79f2365b57 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 3 Jul 2025 14:37:15 +0200 Subject: [PATCH 02/66] rpc: add keypath_only to walletprocesspsbt --- src/rpc/client.cpp | 1 + src/wallet/rpc/spend.cpp | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 21ed8795304f..41230a0b56da 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -214,6 +214,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletprocesspsbt", 2, "sighashtype", ParamFormat::STRING }, { "walletprocesspsbt", 3, "bip32derivs" }, { "walletprocesspsbt", 4, "finalize" }, + { "walletprocesspsbt", 5, "keypath_only"}, { "descriptorprocesspsbt", 0, "psbt", ParamFormat::STRING }, { "descriptorprocesspsbt", 1, "descriptors"}, { "descriptorprocesspsbt", 2, "sighashtype", ParamFormat::STRING }, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 1cc5e0445cb2..d85081810824 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1607,6 +1607,7 @@ RPCMethod walletprocesspsbt() " \"SINGLE|ANYONECANPAY\""}, {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"}, {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"}, + {"keypath_only", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY}, "Only add Taproot key-path data, and only sign and finalize Taproot inputs using the key path. Existing script-path data is not signed or finalized."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -1643,11 +1644,13 @@ RPCMethod walletprocesspsbt() bool sign = request.params[1].isNull() ? true : request.params[1].get_bool(); bool bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool(); bool finalize = request.params[4].isNull() ? true : request.params[4].get_bool(); + bool keypath_only{request.params[5].isNull() ? DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY : request.params[5].get_bool()}; + bool complete = true; if (sign) EnsureWalletIsUnlocked(*pwallet); - const auto err{wallet.FillPSBT(psbtx, {.sign = sign, .sighash_type = nHashType, .finalize = finalize, .bip32_derivs = bip32derivs}, complete)}; + const auto err{wallet.FillPSBT(psbtx, {.sign = sign, .sighash_type = nHashType, .finalize = finalize, .bip32_derivs = bip32derivs, .taproot_keypath_only = keypath_only}, complete)}; if (err) { throw JSONRPCPSBTError(*err); } From c8bfd5da8371e1b7281544134406136539d87a84 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 3 Jul 2025 14:36:42 +0200 Subject: [PATCH 03/66] test: cover keypath_only in wallet_taproot.py Expand taproot tests to cover avoid_script_path in walletprocesspsbt. When avoiding script paths, there's no need for the workaround that increases fee_rate to compensate for the wallet's inability to estimate fees for script path spends. We use this to indirectly test that key path was used. We also check that taproot_script_path_sigs is not set. Finally, for transactions that can't be signed using their key path, we try again by allowing the script path. Additional test extended private keys were extracted from other tests. Co-authored-by: rkrux --- test/functional/wallet_taproot.py | 38 +++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index 1aca5d421c84..3a602b3e4cef 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -172,8 +172,8 @@ def do_test_sendtoaddress(self, comment, pattern, privmap, treefn, keys_pay, key assert rpc_online.gettransaction(txid)["confirmations"] > 0 rpc_online.unloadwallet() - def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change): - self.log.info("Testing %s through PSBT" % comment) + def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change, keypath_only): + self.log.info(f"Testing {comment} through PSBT { '(key path only)' if keypath_only else '' }") # Create wallets wallet_uuid = uuid.uuid4().hex @@ -213,12 +213,16 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change) self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) test_balance = int(psbt_online.getbalance() * 100000000) ret_amnt = random.randrange(100000, test_balance) - # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends. - psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, {"subtractFeeFromOutputs":[0], "fee_rate": 200, "change_type": address_type})['psbt'] - res = psbt_offline.walletprocesspsbt(psbt=psbt, finalize=False) + fee_rate = 1 + if not keypath_only: + # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends. + fee_rate = 200 + psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, {"subtractFeeFromOutputs":[0], "fee_rate": fee_rate, "change_type": address_type})['psbt'] + res = psbt_offline.walletprocesspsbt(psbt=psbt, finalize=False, keypath_only=keypath_only) for wallet in [psbt_offline, key_only_wallet]: - res = wallet.walletprocesspsbt(psbt=psbt, finalize=False) + res = wallet.walletprocesspsbt(psbt=psbt, finalize=False, keypath_only=keypath_only) + retry = False decoded = wallet.decodepsbt(res["psbt"]) if pattern.startswith("tr("): for psbtin in decoded["inputs"]: @@ -226,13 +230,25 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change) assert "witness_utxo" in psbtin assert "taproot_internal_key" in psbtin assert "taproot_bip32_derivs" in psbtin - assert "taproot_key_path_sig" in psbtin or "taproot_script_path_sigs" in psbtin + if keypath_only: + assert "taproot_script_path_sigs" not in psbtin + if "taproot_key_path_sig" not in psbtin: + retry = True + else: + assert "taproot_key_path_sig" in psbtin or "taproot_script_path_sigs" in psbtin if "taproot_script_path_sigs" in psbtin: assert "taproot_merkle_root" in psbtin assert "taproot_scripts" in psbtin + if retry: + self.log.debug("Retry with script path") + fee_rate = 200 + psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, {"subtractFeeFromOutputs":[0], "fee_rate": fee_rate, "change_type": address_type})['psbt'] + res = wallet.walletprocesspsbt(psbt=psbt, finalize=False, keypath_only=False) + rawtx = self.nodes[0].finalizepsbt(res['psbt'])['hex'] res = self.nodes[0].testmempoolaccept([rawtx]) + self.log.debug(res) assert res[0]["allowed"] txid = self.nodes[0].sendrawtransaction(rawtx) @@ -252,13 +268,15 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change) def do_test(self, comment, pattern, privmap, treefn): nkeys = len(privmap) - keys = random.sample(self.keys, nkeys * 4) + keys = random.sample(self.keys, nkeys * 6) self.do_test_addr(comment, pattern, privmap, treefn, keys[0:nkeys]) self.do_test_sendtoaddress(comment, pattern, privmap, treefn, keys[0:nkeys], keys[nkeys:2*nkeys]) - self.do_test_psbt(comment, pattern, privmap, treefn, keys[2*nkeys:3*nkeys], keys[3*nkeys:4*nkeys]) + self.do_test_psbt(comment, pattern, privmap, treefn, keys[2*nkeys:3*nkeys], keys[3*nkeys:4*nkeys], keypath_only=False) + if 'tr' in pattern: + self.do_test_psbt(comment, pattern, privmap, treefn, keys[4*nkeys:5*nkeys], keys[5*nkeys:6*nkeys], keypath_only=True) def generate_test_keys(self): - xprvs = [ExtendedPrivateKey.generate() for _ in range(0, 13)] + xprvs = [ExtendedPrivateKey.generate() for _ in range(0, 18)] return [{ "xprv": xprv.to_string(), "xpub": xprv.pubkey().to_string(), From f6587dc376de75a65effc998fb5cbaafd070e727 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 3 Jul 2025 14:43:05 +0200 Subject: [PATCH 04/66] rpc: add keypath_only to send and sendall --- src/rpc/client.cpp | 2 ++ src/wallet/rpc/spend.cpp | 12 +++++-- test/functional/wallet_fundrawtransaction.py | 2 ++ test/functional/wallet_taproot.py | 35 ++++++++++++++++++++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 41230a0b56da..13f82d903c68 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -267,6 +267,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 4, "replaceable"}, { "send", 4, "solving_data"}, { "send", 4, "max_tx_weight"}, + { "send", 4, "keypath_only"}, { "send", 5, "version"}, { "sendall", 0, "recipients" }, { "sendall", 1, "conf_target" }, @@ -282,6 +283,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "sendall", 4, "send_max"}, { "sendall", 4, "minconf"}, { "sendall", 4, "maxconf"}, + { "sendall", 4, "keypath_only"}, { "sendall", 4, "conf_target"}, { "sendall", 4, "replaceable"}, { "sendall", 4, "solving_data"}, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index d85081810824..6a945358231a 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -111,11 +111,13 @@ static UniValue FinishTransaction(const std::shared_ptr pwallet, const // Make a blank psbt PartiallySignedTransaction psbtx(rawTx, /*version=*/2); + bool keypath_only{options.exists("keypath_only") ? options["keypath_only"].get_bool() : DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY}; + // First fill transaction with our data without signing, // so external signers are not asked to sign more than once. bool complete; - pwallet->FillPSBT(psbtx, {.sign = false, .bip32_derivs = true}, complete); - const auto err{pwallet->FillPSBT(psbtx, {.sign = true, .bip32_derivs = false}, complete)}; + pwallet->FillPSBT(psbtx, {.sign = false, .bip32_derivs = true, .taproot_keypath_only = keypath_only}, complete); + const auto err{pwallet->FillPSBT(psbtx, {.sign = true, .bip32_derivs = false, .taproot_keypath_only = keypath_only}, complete)}; if (err) { throw JSONRPCPSBTError(*err); } @@ -499,6 +501,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact {"includeWatching", UniValueType(UniValue::VBOOL)}, {"include_watching", UniValueType(UniValue::VBOOL)}, {"inputs", UniValueType(UniValue::VARR)}, + {"keypath_only", UniValueType(UniValue::VBOOL)}, {"lockUnspents", UniValueType(UniValue::VBOOL)}, {"lock_unspents", UniValueType(UniValue::VBOOL)}, {"locktime", UniValueType(UniValue::VNUM)}, @@ -811,6 +814,9 @@ RPCMethod fundrawtransaction() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } UniValue options = request.params[1]; + if (options.exists("keypath_only")) { + throw JSONRPCError(RPC_TYPE_ERROR, "Unexpected key keypath_only"); + } std::vector> destinations; for (const auto& tx_out : tx.vout) { CTxDestination dest; @@ -1222,6 +1228,7 @@ RPCMethod send() }}, }, }, + {"keypath_only", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY}, "Only add Taproot key-path data, and only sign and finalize Taproot inputs using the key path. Existing script-path data is not signed or finalized."}, {"locktime", RPCArg::Type::NUM, RPCArg::DefaultHint{"locktime close to block height to prevent fee sniping"}, "Raw locktime. Non-0 value also locktime-activates inputs"}, {"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"}, {"psbt", RPCArg::Type::BOOL, RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."}, @@ -1346,6 +1353,7 @@ RPCMethod sendall() {"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."}, {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "Require inputs with at least this many confirmations."}, {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "Require inputs with at most this many confirmations."}, + {"keypath_only", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY}, "Only add Taproot key-path data, and only sign and finalize Taproot inputs using the key path. Existing script-path data is not signed or finalized."}, {"version", RPCArg::Type::NUM, RPCArg::Default{DEFAULT_WALLET_TX_VERSION}, "Transaction version"}, }, FundTxDoc() diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index a35b2c80cdd0..93525b425e53 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -294,8 +294,10 @@ def test_invalid_option(self): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) + assert "keypath_only" not in self.nodes[2].help("fundrawtransaction") assert_raises_rpc_error(-8, "Unknown named parameter foo", self.nodes[2].fundrawtransaction, rawtx, foo='bar') assert_raises_rpc_error(-3, "JSON value of type bool is not of expected type object", self.nodes[2].fundrawtransaction, rawtx, True) + assert_raises_rpc_error(-3, "Unexpected key keypath_only", self.nodes[2].fundrawtransaction, rawtx, {"keypath_only": True}) # reserveChangeKey was deprecated and is now removed assert_raises_rpc_error(-8, "Unknown named parameter reserveChangeKey", lambda: self.nodes[2].fundrawtransaction(hexstring=rawtx, reserveChangeKey=True)) diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index 3a602b3e4cef..96832dc16d0b 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -255,6 +255,41 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change, self.generatetoaddress(self.nodes[0], 1, self.boring.getnewaddress(), sync_fun=self.no_op) assert psbt_online.gettransaction(txid)['confirmations'] > 0 + if keypath_only: + self.log.info("Testing send with keypath_only") + + def assert_no_script_path(psbt): + decoded = psbt_online.decodepsbt(psbt) + for psbtin in decoded["inputs"]: + assert "taproot_script_path_sigs" not in psbtin + + test_balance = int(psbt_online.getbalance() * 100000000) + outputs = {self.boring.getnewaddress(): Decimal(test_balance // 2) / 100000000} + res = psbt_online.send( + outputs=outputs, + options={ + "keypath_only": True, + "psbt": True, + "add_to_wallet": False, + "fee_rate": 1, + "subtract_fee_from_outputs": [0], + }, + ) + assert_equal(res["complete"], False) + assert_no_script_path(res["psbt"]) + + self.log.info("Testing sendall with keypath_only") + res = psbt_online.sendall( + recipients=[self.boring.getnewaddress()], + keypath_only=True, + psbt=True, + add_to_wallet=False, + fee_rate=1, + ) + assert_equal(res["complete"], False) + assert_no_script_path(res["psbt"]) + + # Cleanup # Match the framework fallbackfee; otherwise the underestimated taproot # script-path spend size can produce an effective feerate below min relay. psbt = psbt_online.sendall(recipients=[self.boring.getnewaddress()], psbt=True, fee_rate=20)["psbt"] From d4501d12f0806229f81173cf63956758b1537539 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Aug 2026 14:17:10 +0200 Subject: [PATCH 05/66] rpc: add keypath_only to walletcreatefundedpsbt --- src/rpc/client.cpp | 1 + src/wallet/rpc/spend.cpp | 4 +++- test/functional/wallet_taproot.py | 13 ++++++++++++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 13f82d903c68..2a4d551350c9 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -206,6 +206,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "replaceable"}, { "walletcreatefundedpsbt", 3, "solving_data"}, { "walletcreatefundedpsbt", 3, "max_tx_weight"}, + { "walletcreatefundedpsbt", 3, "keypath_only"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, { "walletcreatefundedpsbt", 5, "version" }, { "walletcreatefundedpsbt", 6, "psbt_version" }, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 6a945358231a..81cd94274f37 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1728,6 +1728,7 @@ RPCMethod walletcreatefundedpsbt() {"changePosition", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"}, {"change_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -changetype"}, "The output type to use. Only valid if changeAddress is not specified. Options are " + FormatAllOutputTypes() + "."}, {"includeWatching", RPCArg::Type::BOOL, RPCArg::Default{false}, "(DEPRECATED) No longer used"}, + {"keypath_only", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY}, "Only add Taproot key-path data to the PSBT. Script-path data is not added."}, {"lockUnspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"}, {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."}, {"feeRate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_UNIT + "/kvB."}, @@ -1808,8 +1809,9 @@ RPCMethod walletcreatefundedpsbt() // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool(); + bool keypath_only{options.exists("keypath_only") ? options["keypath_only"].get_bool() : DEFAULT_SIGN_TAPROOT_KEYPATH_ONLY}; bool complete = true; - const auto err{wallet.FillPSBT(psbtx, {.sign = false, .bip32_derivs = bip32derivs}, complete)}; + const auto err{wallet.FillPSBT(psbtx, {.sign = false, .bip32_derivs = bip32derivs, .taproot_keypath_only = keypath_only}, complete)}; if (err) { throw JSONRPCPSBTError(*err); } diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index 96832dc16d0b..a2e5e425b402 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -183,6 +183,8 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change, psbt_online = self.nodes[0].get_wallet_rpc(f"psbt_online_{wallet_uuid}") psbt_offline = self.nodes[1].get_wallet_rpc(f"psbt_offline_{wallet_uuid}") key_only_wallet = self.nodes[1].get_wallet_rpc(f"key_only_wallet_{wallet_uuid}") + if keypath_only: + assert "keypath_only" in psbt_online.help("walletcreatefundedpsbt") desc_pay = self.make_desc(pattern, privmap, keys_pay, False) desc_change = self.make_desc(pattern, privmap, keys_change, False) @@ -217,7 +219,16 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change, if not keypath_only: # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends. fee_rate = 200 - psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, {"subtractFeeFromOutputs":[0], "fee_rate": fee_rate, "change_type": address_type})['psbt'] + options = { + "subtractFeeFromOutputs": [0], + "fee_rate": fee_rate, + "change_type": address_type, + "keypath_only": keypath_only, + } + psbt = psbt_online.walletcreatefundedpsbt([], [{self.boring.getnewaddress(): Decimal(ret_amnt) / 100000000}], None, options)['psbt'] + if keypath_only: + for psbtin in psbt_online.decodepsbt(psbt)["inputs"]: + assert "taproot_scripts" not in psbtin res = psbt_offline.walletprocesspsbt(psbt=psbt, finalize=False, keypath_only=keypath_only) for wallet in [psbt_offline, key_only_wallet]: res = wallet.walletprocesspsbt(psbt=psbt, finalize=False, keypath_only=keypath_only) From 27aa32d46da46a7fbfbac0776eb707b651d4c945 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 8 Jun 2026 16:36:55 +0200 Subject: [PATCH 06/66] rpc: add keypath_only to descriptorprocesspsbt --- src/rpc/client.cpp | 1 + src/rpc/rawtransaction.cpp | 21 ++++++++--- test/functional/rpc_psbt.py | 69 +++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 2a4d551350c9..22ad340f67fd 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -221,6 +221,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "descriptorprocesspsbt", 2, "sighashtype", ParamFormat::STRING }, { "descriptorprocesspsbt", 3, "bip32derivs" }, { "descriptorprocesspsbt", 4, "finalize" }, + { "descriptorprocesspsbt", 5, "keypath_only"}, { "createpsbt", 0, "inputs" }, { "createpsbt", 1, "outputs" }, { "createpsbt", 2, "locktime" }, diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 97b22c11e08e..27e452605ba7 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -125,7 +125,13 @@ static std::vector CreateTxDoc() // Update PSBT with information from the mempool, the UTXO set, the txindex, and the provided descriptors. // Optionally, sign the inputs that we can using information from the descriptors. -PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std::any& context, const HidingSigningProvider& provider, std::optional sighash_type, bool finalize) +PartiallySignedTransaction ProcessPSBT( + const std::string& psbt_string, + const std::any& context, + const HidingSigningProvider& provider, + std::optional sighash_type, + bool finalize, + bool taproot_keypath_only) { // Unserialize the transactions util::Result psbt_res = DecodeBase64PSBT(psbt_string); @@ -195,7 +201,10 @@ PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std // We only actually care about those if our signing provider doesn't hide private // information, as is the case with `descriptorprocesspsbt` // Only error for mismatching sighash types as it is critical that the sighash to sign with matches the PSBT's - const auto sign_result = SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, {.sighash_type = sighash_type, .finalize = finalize}, /*out_sigdata=*/nullptr); + const auto sign_result = SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, { + .sighash_type = sighash_type, + .finalize = finalize, + .taproot_keypath_only = taproot_keypath_only}, /*out_sigdata=*/nullptr); if (!sign_result.has_value() && sign_result.error() == common::PSBTError::SIGHASH_MISMATCH) { throw JSONRPCPSBTError(common::PSBTError::SIGHASH_MISMATCH); } @@ -1852,7 +1861,8 @@ static RPCMethod utxoupdatepsbt() request.context, HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false), /*sighash_type=*/std::nullopt, - /*finalize=*/false); + /*finalize=*/false, + /*taproot_keypath_only=*/false); DataStream ssTx{}; ssTx << psbtx; @@ -2093,6 +2103,7 @@ RPCMethod descriptorprocesspsbt() " \"SINGLE|ANYONECANPAY\""}, {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"}, {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"}, + {"keypath_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Only add Taproot key-path data, and only sign and finalize Taproot inputs using the key path. Existing script-path data is not signed or finalized."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -2119,13 +2130,15 @@ RPCMethod descriptorprocesspsbt() std::optional sighash_type = ParseSighashString(request.params[2]); bool bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool(); bool finalize = request.params[4].isNull() ? true : request.params[4].get_bool(); + bool keypath_only{request.params[5].isNull() ? false : request.params[5].get_bool()}; const PartiallySignedTransaction& psbtx = ProcessPSBT( request.params[0].get_str(), request.context, HidingSigningProvider(&provider, /*hide_secret=*/false, !bip32derivs), sighash_type, - finalize); + finalize, + keypath_only); // Check whether or not all of the inputs are now correctly signed bool complete = true; diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index d8c60fe61a31..9949febe7ace 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -1512,6 +1512,75 @@ def test_psbt_input_keys(psbt_input, keys): # Broadcast transaction self.nodes[2].sendrawtransaction(processed_psbt['hex']) + self.log.info("Test descriptorprocesspsbt keypath_only skips taproot script path signing") + taproot_key = get_generate_key() + taproot_descriptor = descsum_create(f"tr({H_POINT},pk({taproot_key.privkey}))") + taproot_public_descriptor = descsum_create(f"tr({H_POINT},pk({taproot_key.pubkey}))") + taproot_address = self.nodes[2].deriveaddresses(taproot_descriptor)[0] + taproot_utxo = self.create_outpoints(self.nodes[0], outputs=[{taproot_address: 1}])[0] + self.sync_all() + + taproot_psbt = self.nodes[2].createpsbt([taproot_utxo], {self.nodes[0].getnewaddress(): 0.99999}) + keypath_only_psbt = self.nodes[2].descriptorprocesspsbt( + psbt=taproot_psbt, + descriptors=[taproot_descriptor], + finalize=False, + keypath_only=True, + ) + decoded = self.nodes[2].decodepsbt(keypath_only_psbt["psbt"]) + assert "taproot_scripts" not in decoded["inputs"][0] + assert "taproot_key_path_sig" not in decoded["inputs"][0] + assert "taproot_script_path_sigs" not in decoded["inputs"][0] + + prepopulated_psbt = self.nodes[2].descriptorprocesspsbt( + psbt=taproot_psbt, + descriptors=[taproot_public_descriptor], + finalize=False, + ) + decoded = self.nodes[2].decodepsbt(prepopulated_psbt["psbt"]) + assert "taproot_scripts" in decoded["inputs"][0] + assert "taproot_script_path_sigs" not in decoded["inputs"][0] + + keypath_only_prepopulated_psbt = self.nodes[2].descriptorprocesspsbt( + psbt=prepopulated_psbt["psbt"], + descriptors=[taproot_descriptor], + finalize=False, + keypath_only=True, + ) + decoded = self.nodes[2].decodepsbt(keypath_only_prepopulated_psbt["psbt"]) + assert "taproot_scripts" in decoded["inputs"][0] + assert "taproot_key_path_sig" not in decoded["inputs"][0] + assert "taproot_script_path_sigs" not in decoded["inputs"][0] + + signed_psbt = self.nodes[2].descriptorprocesspsbt( + psbt=taproot_psbt, + descriptors=[taproot_descriptor], + finalize=False, + keypath_only=False, + ) + decoded = self.nodes[2].decodepsbt(signed_psbt["psbt"]) + assert "taproot_script_path_sigs" in decoded["inputs"][0] + + # Do not finalize an existing script-path signature when keypath_only is + # requested, since that would produce a broadcastable script-path spend. + keypath_only_signed_psbt = self.nodes[2].descriptorprocesspsbt( + psbt=signed_psbt["psbt"], + descriptors=[taproot_descriptor], + finalize=True, + keypath_only=True, + ) + assert_equal(keypath_only_signed_psbt["complete"], False) + decoded = self.nodes[2].decodepsbt(keypath_only_signed_psbt["psbt"]) + assert "taproot_script_path_sigs" in decoded["inputs"][0] + assert "final_scriptwitness" not in decoded["inputs"][0] + + # The standalone finalizer has no key-path-only policy and can explicitly + # finalize the otherwise unchanged script-path signature. + finalized = self.nodes[2].finalizepsbt(keypath_only_signed_psbt["psbt"]) + assert_equal(finalized["complete"], True) + rawtx = finalized["hex"] + assert self.nodes[2].testmempoolaccept([rawtx])[0]["allowed"] + self.log.info("Test descriptorprocesspsbt raises if an invalid sighashtype is passed") assert_raises_rpc_error(-8, "'all' is not a valid sighash parameter.", self.nodes[2].descriptorprocesspsbt, psbt=psbt, descriptors=[descriptor], sighashtype="all") From 203e493f026d42396247522421958409e5b26354 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 9 Jun 2026 17:42:09 +0200 Subject: [PATCH 07/66] test: cover keypath_only in wallet_musig.py Extract the descriptor with both MuSig key and script paths into key_and_script_path_musigs. Reuse it for the existing cases and give those cases descriptions. Expand the same scenario with keypath_only=true. The new case asks walletprocesspsbt to avoid script-path signing. Co-authored-by: rkrux --- test/functional/wallet_musig.py | 37 ++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py index e23c9496154e..dd2d7329e7a0 100755 --- a/test/functional/wallet_musig.py +++ b/test/functional/wallet_musig.py @@ -194,7 +194,16 @@ def test_failure_case_3(self, comment, pat): assert "musig2_pubnonces" in dec["inputs"][0] assert "musig2_partial_sigs" not in dec["inputs"][0] - def test_success_case(self, comment, pattern, sighash_type=None, scriptpath=False, nosign_wallets=None, only_one_musig_wallet=False): + def test_success_case( + self, + comment, + pattern, + sighash_type=None, + scriptpath=False, + keypath_only=False, + nosign_wallets=None, + only_one_musig_wallet=False, + ): self.log.info(f"Testing {comment}") has_internal = MULTIPATH_TWO_RE.search(pattern) is not None @@ -221,6 +230,9 @@ def test_success_case(self, comment, pattern, sighash_type=None, scriptpath=Fals continue if musig_partial_sigs is not None: expected_partial_sigs += musig_partial_sigs + if keypath_only: + # The first MuSig aggregate is the key path, so do not count script-path MuSigs. + break # Check that the wallets agree on the same musig address addr = None @@ -289,7 +301,7 @@ def test_success_case(self, comment, pattern, sighash_type=None, scriptpath=Fals if nosign_wallets and i in nosign_wallets: continue for psbt_list in [nonce_psbts, nonce_psbts2]: - proc = wallet.walletprocesspsbt(psbt=psbt, sighashtype=sighash_type) + proc = wallet.walletprocesspsbt(psbt=psbt, sighashtype=sighash_type, keypath_only=keypath_only) assert_equal(proc["complete"], False) psbt_list.append(proc["psbt"]) @@ -310,7 +322,7 @@ def test_success_case(self, comment, pattern, sighash_type=None, scriptpath=Fals if nosign_wallets and i in nosign_wallets: continue for psbt, psbt_list in [(comb_nonce_psbt, psig_psbts), (comb_nonce_psbt2, psig_psbts2)]: - proc = wallet.walletprocesspsbt(psbt=psbt, sighashtype=sighash_type) + proc = wallet.walletprocesspsbt(psbt=psbt, sighashtype=sighash_type, keypath_only=keypath_only) assert_equal(proc["complete"], False) psbt_list.append(proc["psbt"]) @@ -355,8 +367,23 @@ def run_test(self): self.test_success_case("tr(H,{pk(musig/*), pk(musig/*)})", "tr($H,{pk(musig($0,$1,$2)/<0;1>/*),pk(musig($3,$4,$5)/0/*)})", scriptpath=True) self.test_success_case("tr(H,{pk(musig/*), pk(same keys different musig/*)})", "tr($H,{pk(musig($0,$1,$2)/<0;1>/*),pk(musig($1,$2)/0/*)})", scriptpath=True) self.test_success_case("tr(H,and(pk(musig/*),pk(same musig, other derivation/*)))", "tr($H,and_v(v:pk(musig($0,$1,$2)/<0;1>/*),pk(musig($0,$1,$2)/<2;3>/*)))", scriptpath=True) - self.test_success_case("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})}", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})") - self.test_success_case("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})} script-path", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})", scriptpath=True, nosign_wallets=[0]) + # Descriptor with one MuSig key path and two MuSig script paths. + key_and_script_path_musigs = "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})" + self.test_success_case( + "tr() with MuSig key path and different MuSig script paths", + key_and_script_path_musigs, + ) + self.test_success_case( + "tr() with MuSig script path when key path cannot sign", + key_and_script_path_musigs, + scriptpath=True, + nosign_wallets=[0], + ) + self.test_success_case( + "tr() with MuSig key path and keypath_only", + key_and_script_path_musigs, + keypath_only=True, + ) self.test_success_case("tr(H,and(pk(musig/*),after(1)))", "tr($H,and_v(v:pk(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True) self.test_success_case("tr(H,and(pk_k(musig/*),after(1)))", "tr($H,and_v(vc:pk_k(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True) self.test_success_case("tr(H,and(pkh(musig/*),after(1)))", "tr($H,and_v(v:pkh(musig($0,$1,$2)/<0;1>/*),after(1)))", scriptpath=True) From bad6f06a5c7d7b3f61484f2fcb1d25a1aa6806b8 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 16 Jun 2026 17:56:18 +0200 Subject: [PATCH 08/66] doc: add release note for keypath_only --- doc/release-notes-32857.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/release-notes-32857.md diff --git a/doc/release-notes-32857.md b/doc/release-notes-32857.md new file mode 100644 index 000000000000..24cdb811e794 --- /dev/null +++ b/doc/release-notes-32857.md @@ -0,0 +1,8 @@ +Updated RPCs +------------ + +- The `send`, `sendall`, `walletprocesspsbt`, `walletcreatefundedpsbt`, and + `descriptorprocesspsbt` RPCs now accept a `keypath_only` option. When enabled, + they do not add new Taproot script-path data, sign script paths, or finalize + script-path spends. Existing script-path data and signatures remain in a + supplied PSBT. (#32857) From 798522e0489dabe9c65845646375d42b11b6f1ed Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 29 Jun 2026 16:31:57 +0200 Subject: [PATCH 09/66] test: remove unused wallet_taproot init_wallet --- test/functional/wallet_taproot.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index a2e5e425b402..fee8e5fc0fcf 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -64,9 +64,6 @@ def skip_test_if_missing_module(self): def setup_network(self): self.setup_nodes() - def init_wallet(self, *, node): - pass - @staticmethod def make_desc(pattern, privmap, keys, pub_only = False): pat = pattern.replace("$H", H_POINT) From 331f87613fc575dcb27180948c783c0d0f5c2fcc Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 14 May 2026 13:30:39 +0200 Subject: [PATCH 10/66] test: check unused xprv descriptor pubkeys Co-authored-by: adyshimony <6388409+adyshimony@users.noreply.github.com> --- src/test/descriptor_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index ed410dfbb148..c4d277b777a0 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -1394,7 +1394,7 @@ void CheckUnused(const std::string& prv, const std::string& pub) // Check both only have one pubkey std::set prv_pubkeys; std::set prv_extpubs; - parse_pub->GetPubKeys(prv_pubkeys, prv_extpubs); + parse_priv->GetPubKeys(prv_pubkeys, prv_extpubs); BOOST_CHECK_EQUAL(prv_pubkeys.size() + prv_extpubs.size(), 1); std::set pub_pubkeys; std::set pub_extpubs; From 4daf376b5e8ef34205c85eb0a0415c816027372c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 14 May 2026 13:30:44 +0200 Subject: [PATCH 11/66] wallet: reject duplicate addhdkey xprvs Co-authored-by: adyshimony <6388409+adyshimony@users.noreply.github.com> --- src/wallet/rpc/wallet.cpp | 4 ++++ test/functional/wallet_hd.py | 2 ++ 2 files changed, 6 insertions(+) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 054ab6b76310..f9b559f886c7 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -882,6 +882,10 @@ RPCMethod addhdkey() } LOCK(wallet->cs_wallet); + if (wallet->GetKey(hdkey.Neuter().pubkey.GetID())) { + throw JSONRPCError(RPC_WALLET_ERROR, "HD key already exists"); + } + std::string desc_str = "unused(" + EncodeExtKey(hdkey) + ")"; FlatSigningProvider keys; std::string error; diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py index e5227e6f496f..f4350457d0e5 100755 --- a/test/functional/wallet_hd.py +++ b/test/functional/wallet_hd.py @@ -34,6 +34,8 @@ def test_addhdkey(self): wallet = self.nodes[0].get_wallet_rpc("hdkey") assert_equal(len(wallet.gethdkeys()), 1) + existing_wallet_xprv = wallet.gethdkeys(private=True)[0]["xprv"] + assert_raises_rpc_error(-4, "HD key already exists", wallet.addhdkey, existing_wallet_xprv) wallet.addhdkey() xpub_info = wallet.gethdkeys() From f36bf793e324fc26127481a380a2de73fdc750ef Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 3 Jul 2025 10:29:58 +0200 Subject: [PATCH 12/66] rpc: make createwalletdescriptor smarter When a wallet contains only an unused(KEY) descriptor, use it. Previously the user would have to call listdescriptors and manually specify it. --- src/wallet/rpc/wallet.cpp | 19 ++++++++++++--- .../wallet_createwalletdescriptor.py | 23 ++++++++++++++++++- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index f9b559f886c7..9cb649f258ee 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -748,7 +748,7 @@ static RPCMethod createwalletdescriptor() {"type", RPCArg::Type::STR, RPCArg::Optional::NO, "The address type the descriptor will produce. Options are " + FormatAllOutputTypes() + "."}, {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", { {"internal", RPCArg::Type::BOOL, RPCArg::DefaultHint{"Both external and internal will be generated unless this parameter is specified"}, "Whether to only make one descriptor that is internal (if parameter is true) or external (if parameter is false)"}, - {"hdkey", RPCArg::Type::STR, RPCArg::DefaultHint{"The HD key used by all other active descriptors"}, "The HD key that the wallet knows the private key of, listed using 'gethdkeys', to use for this descriptor's key"}, + {"hdkey", RPCArg::Type::STR, RPCArg::DefaultHint{"The HD key used by all other active descriptors, or, if there are none, the HD key of an unused(KEY) descriptor."}, "The HD key that the wallet knows the private key of, listed using 'gethdkeys', to use for this descriptor's key"}, }}, }, RPCResult{ @@ -790,11 +790,24 @@ static RPCMethod createwalletdescriptor() CExtPubKey xpub; if (hdkey.isNull()) { + // First consider the HD key from active descriptors HDPubKeyMap active_xpubs = pwallet->GetHDPubKeys(HDKeyFilter::Active); - if (active_xpubs.size() != 1) { + if (active_xpubs.size() == 1) { + xpub = active_xpubs.begin()->first; + } else if (active_xpubs.size() > 1) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to determine which HD key to use from active descriptors. Please specify with 'hdkey'"); + } else { + // Look for an unused(KEY) descriptor + HDPubKeyMap wallet_xpubs{pwallet->GetHDPubKeys(HDKeyFilter::UnusedKey)}; + + if (wallet_xpubs.empty()) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No HD key found. Please generate one with 'addhdkey' or import an active descriptor."); + } else if (wallet_xpubs.size() > 1) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to determine which HD key to use. Please specify with 'hdkey'"); + } + + xpub = wallet_xpubs.begin()->first; } - xpub = active_xpubs.begin()->first; } else { xpub = DecodeExtPubKey(hdkey.get_str()); if (!xpub.pubkey.IsValid()) { diff --git a/test/functional/wallet_createwalletdescriptor.py b/test/functional/wallet_createwalletdescriptor.py index b1d411322812..73a75f3f2d3a 100755 --- a/test/functional/wallet_createwalletdescriptor.py +++ b/test/functional/wallet_createwalletdescriptor.py @@ -25,6 +25,7 @@ def run_test(self): self.test_basic() self.test_imported_other_keys() self.test_encrypted() + self.test_from_unused_desc() def test_basic(self): def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) @@ -39,7 +40,7 @@ def test_basic(self): if desc["desc"].startswith("wpkh("): expected_descs.append(desc["desc"]) - assert_raises_rpc_error(-5, "Unable to determine which HD key to use from active descriptors. Please specify with 'hdkey'", wallet.createwalletdescriptor, "bech32") + assert_raises_rpc_error(-5, "No HD key found. Please generate one with 'addhdkey' or import an active descriptor.", wallet.createwalletdescriptor, "bech32") assert_raises_rpc_error(-5, f"Private key for {xpub} is not known", wallet.createwalletdescriptor, type="bech32", hdkey=xpub) self.log.info("Test createwalletdescriptor after importing active descriptor to blank wallet") @@ -114,6 +115,26 @@ def test_encrypted(self): with WalletUnlock(wallet, "pass"): wallet.createwalletdescriptor(type="bech32m") + def test_from_unused_desc(self): + self.log.info("Test createwalletdescriptor from only an unused(KEY) descriptor") + self.nodes[0].createwallet("w1", blank=True) + w1 = self.nodes[0].get_wallet_rpc("w1") + + # Wallet can't be completely empty + assert_raises_rpc_error(-5, "No HD key found. Please generate one with 'addhdkey' or import an active descriptor.", w1.createwalletdescriptor, "bech32") + + # Create unused(KEY) descriptor and try again + w1.addhdkey() + w1.createwalletdescriptor(type="bech32") + + self.nodes[0].createwallet("w2", blank=True) + w2 = self.nodes[0].get_wallet_rpc("w2") + + # Multiple unused(KEY) descriptors require user to choose + w2.addhdkey() + w2.addhdkey() + + assert_raises_rpc_error(-5, "Unable to determine which HD key to use. Please specify with 'hdkey'", w2.createwalletdescriptor, "bech32") if __name__ == '__main__': From 97522077bfa50a6f1fc5fc262fc4ab5ab3800d57 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 25 Aug 2026 09:39:21 +0200 Subject: [PATCH 13/66] doc: use a blank wallet in the multisig tutorial The participant wallets are only used to derive an xpub and sign PSBTs for the multisig wallet. Creating them blank and adding just the legacy descriptor avoids handing the reader singlesig addresses they are not supposed to use. The legacy descriptor can't be dropped entirely because the signing code only considers descriptors whose derivation paths cover the input. To further discourage use of the singlesig descriptors, the PSBT example now pays a separate recipient wallet rather than participant_1. --- doc/multisig-tutorial.md | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/doc/multisig-tutorial.md b/doc/multisig-tutorial.md index fd74e62ed28f..0a9379d9caf7 100644 --- a/doc/multisig-tutorial.md +++ b/doc/multisig-tutorial.md @@ -20,15 +20,27 @@ This tutorial also uses the default PKH derivation path to get the xpubs and doe For a 2-of-3 multisig, create 3 wallets. These wallets contain HD seed and private keys, which will be used to sign the PSBTs and derive the xpub. -These three wallets should not be used directly for privacy reasons (public key reuse). They should only be used to sign transactions for the (watch-only) multisig wallet. +These three wallets should not be used directly for privacy reasons (public key reuse). They should only be used to sign transactions for the (watch-only) multisig wallet. To make that less likely to happen by accident, participant wallets only have singlesig legacy addresses, and no singlesig Bech32 addresses. + +`addhdkey` adds an HD key to the blank wallet, which `createwalletdescriptor` then uses: ```bash for ((n=1;n<=3;n++)) do - ./build/bin/bitcoin rpc -signet createwallet "participant_${n}" + ./build/bin/bitcoin rpc -signet -named createwallet wallet_name="participant_${n}" blank=true + ./build/bin/bitcoin rpc -signet -rpcwallet="participant_${n}" addhdkey + ./build/bin/bitcoin rpc -signet -rpcwallet="participant_${n}" createwalletdescriptor legacy done ``` +The `legacy` descriptor is only there to hold the private keys: it derives from `m/44h/1h/0h/<0;1>/*`, the same paths as the multisig descriptor defined below, and a wallet can only sign for a derivation path that one of its descriptors uses. + +A later step spends from the multisig wallet, so create one more wallet to receive that payment. This one is an ordinary singlesig wallet, standing in for whoever is being paid: + +```bash +./build/bin/bitcoin rpc -signet createwallet "recipient" +``` + Extract the xpub of each wallet. To do this, the `derivehdkey` RPC is used. Note that previously at least two descriptors were usually used, one for external derivation paths and one for internal ones. Since https://github.com/bitcoin/bitcoin/pull/22838 this redundancy has been eliminated by a multipath descriptor with <0;1> at the [BIP-44](https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#change) change level expanding to external and internal descriptors when imported. @@ -100,6 +112,7 @@ Once the wallets have already been created and this tutorial needs to be repeate ```bash for ((n=1;n<=3;n++)); do ./build/bin/bitcoin rpc -signet loadwallet "participant_${n}"; done ./build/bin/bitcoin rpc -signet loadwallet "multisig_wallet_01" +./build/bin/bitcoin rpc -signet loadwallet "recipient" ``` ### 1.4 Fund the wallet @@ -138,7 +151,7 @@ PSBT is a data format that allows wallets and other tools to exchange informatio The current PSBT version (v0) is defined in [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki). -For simplicity, the destination address is taken from the `participant_1` wallet in the code above, but it can be any valid bitcoin address. +The destination address is taken from the `recipient` wallet. The `walletcreatefundedpsbt` RPC is used to create and fund a transaction in the PSBT format. It is the first step in creating the PSBT. @@ -147,7 +160,7 @@ balance=$(./build/bin/bitcoin rpc -signet -rpcwallet="multisig_wallet_01" getbal amount=$(echo "$balance * 0.8" | bc -l | sed -e 's/^\./0./' -e 's/^-\./-0./') -destination_addr=$(./build/bin/bitcoin rpc -signet -rpcwallet="participant_1" getnewaddress) +destination_addr=$(./build/bin/bitcoin rpc -signet -rpcwallet="recipient" getnewaddress "" bech32m) funded_psbt=$(./build/bin/bitcoin rpc -signet -rpcwallet="multisig_wallet_01" walletcreatefundedpsbt outputs="{\"$destination_addr\": $amount}" | jq -r '.psbt') ``` From 9d4eb1f50d59a258592d4faab5e7c6daa2616a53 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 25 Aug 2026 09:43:02 +0200 Subject: [PATCH 14/66] doc: add release note for #32861 --- doc/release-notes-32861.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 doc/release-notes-32861.md diff --git a/doc/release-notes-32861.md b/doc/release-notes-32861.md new file mode 100644 index 000000000000..9030ee57f9f9 --- /dev/null +++ b/doc/release-notes-32861.md @@ -0,0 +1,9 @@ +Updated RPCs +------------ + +- `createwalletdescriptor` no longer requires the `hdkey` argument when the + wallet has no active descriptors but does have exactly one HD key added with + `addhdkey`. Previously the user had to look the key up with `listdescriptors` + and pass it in. This makes it easier to set up a blank wallet that only has + the descriptors it actually needs, which `doc/multisig-tutorial.md` now + demonstrates. (#32861) From dabe9c036e902a7b6feccacb698cad0477ae9769 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 1 Aug 2025 11:06:54 +0200 Subject: [PATCH 15/66] wallet: don't import external keys at creation if blank There's no need to treat external signer wallets different in this regard. When the user sets the 'blank' flag, don't generate or import keys. For multisig setups that involve an external signer, it may be useful to start from a blank wallet and manually import descriptors. --- src/wallet/wallet.cpp | 8 ++++---- test/functional/wallet_signer.py | 7 +++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 310e9ea07a1b..380b75acf0af 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3703,10 +3703,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans() void CWallet::SetupWalletGeneration() { AssertLockHeld(cs_wallet); - // Skip setup for non-external-signer wallets that are either blank - // or have private keys disabled (not having private keys implies blank). - if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) && - (IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS))) { + // Skip setup for blank wallets. Non-external-signer wallets with disabled + // private keys also skip setup (not having private keys implies blank). + if (IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || + (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) && IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS))) { return; } SetupDescriptorScriptPubKeyMans(); diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 896169d4276a..ba678888b0ad 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -72,6 +72,13 @@ def test_valid_signer(self): hww = self.nodes[1].get_wallet_rpc('hww') assert_equal(hww.getwalletinfo()["external_signer"], True) + # A blank external signer wallet does not auto-import any keys. + self.nodes[1].createwallet(wallet_name='hww_blank', disable_private_keys=True, external_signer=True, blank=True) + hww_blank = self.nodes[1].get_wallet_rpc('hww_blank') + assert_equal(hww_blank.getwalletinfo()["keypoolsize"], 0) + assert_equal(hww_blank.listdescriptors()["descriptors"], []) + self.nodes[1].unloadwallet('hww_blank') + # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets) self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, external_signer=False) not_hww = self.nodes[1].get_wallet_rpc('not_hww') From 2f91efb26f4996f0b37b76082f1aaba715c7468c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 1 Aug 2025 14:51:53 +0200 Subject: [PATCH 16/66] wallet: avoid signing via createTransaction() with external signer External signer enabled wallets should always use the process PSBT flow. Avoid going through CreateTransaction. This has no effect until a later commit where WALLET_FLAG_EXTERNAL_SIGNER no longer implies WALLET_FLAG_DISABLE_PRIVATE_KEYS. Without this change signing with the GUI would break for external signers with private keys enabled. --- src/qt/walletmodel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 97f2f4268cac..35a5575051b4 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -203,7 +203,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact try { auto& newTx = transaction.getWtx(); - const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled(), /*change_pos=*/std::nullopt); + const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled() && !wallet().hasExternalSigner(), /*change_pos=*/std::nullopt); if (!res) { Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated), CClientUIInterface::MSG_ERROR); From 034375fc9ce18d4f4127fc163cd3307fbae3019e Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 1 Aug 2025 10:45:49 +0200 Subject: [PATCH 17/66] wallet: make watch-only optional for external signer Before this change the external_signer flag required the wallet to be watch-only. This precludes multisig setups in which we hold a hot key. Remove this as a requirement, but disable private keys by default. This leaves the typical (and only documented) use case of a single external signer unaffected. --- src/qt/createwalletdialog.cpp | 28 ++++++++-------------------- src/wallet/rpc/wallet.cpp | 19 ++++++++++++++----- src/wallet/wallet.cpp | 7 ------- test/functional/wallet_signer.py | 29 +++++++++++++++++++---------- 4 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp index 59c6f51a27ec..b73c9728240e 100644 --- a/src/qt/createwalletdialog.cpp +++ b/src/qt/createwalletdialog.cpp @@ -26,34 +26,25 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : }); connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) { - // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is + // Disable the disable_privkeys_checkbox when isEncryptWalletChecked is // set to true, enable it when isEncryptWalletChecked is false. ui->disable_privkeys_checkbox->setEnabled(!checked); -#ifdef ENABLE_EXTERNAL_SIGNER - ui->external_signer_checkbox->setEnabled(m_has_signers && !checked); -#endif + // When the disable_privkeys_checkbox is disabled, uncheck it. if (!ui->disable_privkeys_checkbox->isEnabled()) { ui->disable_privkeys_checkbox->setChecked(false); } - - // When the external_signer_checkbox box is disabled, uncheck it. - if (!ui->external_signer_checkbox->isEnabled()) { - ui->external_signer_checkbox->setChecked(false); - } - }); connect(ui->external_signer_checkbox, &QCheckBox::toggled, [this](bool checked) { - ui->encrypt_wallet_checkbox->setEnabled(!checked); - ui->blank_wallet_checkbox->setEnabled(!checked); - ui->disable_privkeys_checkbox->setEnabled(!checked); + // In the basic use case all keys will be on the external signer + // device and the wallet should be watch-only. Makes this the + // default suggestion. + ui->disable_privkeys_checkbox->setChecked(checked); - // The external signer checkbox is only enabled when a device is detected. - // In that case it is checked by default. Toggling it restores the other - // options to their default. + // The external signer box is checked by default when a device is + // detected. Toggling it restores the other options to their default. ui->encrypt_wallet_checkbox->setChecked(false); - ui->disable_privkeys_checkbox->setChecked(checked); ui->blank_wallet_checkbox->setChecked(false); }); @@ -103,12 +94,9 @@ void CreateWalletDialog::setSigners(const std::vectorexternal_signer_checkbox->setEnabled(true); ui->external_signer_checkbox->setChecked(true); - ui->encrypt_wallet_checkbox->setEnabled(false); ui->encrypt_wallet_checkbox->setChecked(false); // The order matters, because connect() is called when toggling a checkbox: - ui->blank_wallet_checkbox->setEnabled(false); ui->blank_wallet_checkbox->setChecked(false); - ui->disable_privkeys_checkbox->setEnabled(false); ui->disable_privkeys_checkbox->setChecked(true); const std::string label = signers[0]->getName(); ui->wallet_name_line_edit->setText(QString::fromStdString(label)); diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 054ab6b76310..61b8bacab2f1 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -359,13 +359,13 @@ static RPCMethod createwallet() "Creates and loads a new wallet.\n", { {"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name for the new wallet. If this is a path, the wallet will be created at the path location."}, - {"disable_private_keys", RPCArg::Type::BOOL, RPCArg::Default{false}, "Disable the possibility of private keys (only watchonlys are possible in this mode)."}, + {"disable_private_keys", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false unless external_signer is set"}, "Disable the possibility of private keys (only watchonlys are possible in this mode)."}, {"blank", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a blank wallet. A blank wallet has no keys."}, {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Encrypt the wallet with this passphrase."}, {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."}, {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "If set, must be \"true\""}, {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, - {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."}, + {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -387,9 +387,8 @@ static RPCMethod createwallet() { WalletContext& context = EnsureWalletContext(request.context); uint64_t flags = 0; - if (!request.params[1].isNull() && request.params[1].get_bool()) { - flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS; - } + + std::optional disable_private_keys{self.MaybeArg("disable_private_keys")}; if (!request.params[2].isNull() && request.params[2].get_bool()) { flags |= WALLET_FLAG_BLANK_WALLET; @@ -415,11 +414,21 @@ static RPCMethod createwallet() if (!request.params[7].isNull() && request.params[7].get_bool()) { #ifdef ENABLE_EXTERNAL_SIGNER flags |= WALLET_FLAG_EXTERNAL_SIGNER; + if (!disable_private_keys.has_value()) { + // In the basic use case all keys will be on the external signer + // device and the wallet should be watch-only. Makes this the + // default. + disable_private_keys = true; + } #else throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)"); #endif } + if (disable_private_keys.value_or(false)) { + flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS; + } + DatabaseOptions options; DatabaseStatus status; ReadDatabaseArgs(*context.args, options); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 380b75acf0af..40804025f497 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -402,13 +402,6 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& options.require_format = DatabaseFormat::SQLITE; - // Private keys must be disabled for an external signer wallet - if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { - error = Untranslated("Private keys must be disabled when using an external signer"); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; - } - // Do not allow a passphrase when private keys are disabled if (born_encrypted && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { error = Untranslated("Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled."); diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index ba678888b0ad..1aa4e39579d3 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -66,29 +66,38 @@ def test_valid_signer(self): self.log.debug(f"-signer={self.mock_signer_path()}") # Create new wallets for an external signer. - # disable_private_keys and descriptors must be true: - assert_raises_rpc_error(-4, "Private keys must be disabled when using an external signer", self.nodes[1].createwallet, wallet_name='not_hww', disable_private_keys=False, external_signer=True) - self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, external_signer=True) + self.nodes[1].createwallet(wallet_name='hww', external_signer=True) hww = self.nodes[1].get_wallet_rpc('hww') assert_equal(hww.getwalletinfo()["external_signer"], True) + # Private keys are disabled by default + assert_equal(hww.getwalletinfo()["private_keys_enabled"], False) + + # Private keys can be explicitly enabled for external signer wallets + self.nodes[1].createwallet(wallet_name='hww_hot', external_signer=True, disable_private_keys=False) + hww_hot = self.nodes[1].get_wallet_rpc('hww_hot') + assert_equal(hww_hot.getwalletinfo()["external_signer"], True) + assert_equal(hww_hot.getwalletinfo()["private_keys_enabled"], True) + self.nodes[1].unloadwallet('hww_hot') + # A blank external signer wallet does not auto-import any keys. - self.nodes[1].createwallet(wallet_name='hww_blank', disable_private_keys=True, external_signer=True, blank=True) + self.nodes[1].createwallet(wallet_name='hww_blank', external_signer=True, blank=True) hww_blank = self.nodes[1].get_wallet_rpc('hww_blank') assert_equal(hww_blank.getwalletinfo()["keypoolsize"], 0) assert_equal(hww_blank.listdescriptors()["descriptors"], []) self.nodes[1].unloadwallet('hww_blank') # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets) - self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, external_signer=False) + self.nodes[1].createwallet(wallet_name='not_hww', external_signer=False) not_hww = self.nodes[1].get_wallet_rpc('not_hww') assert_equal(not_hww.getwalletinfo()["external_signer"], False) + # Without external_signer, private keys are enabled by default + assert_equal(not_hww.getwalletinfo()["private_keys_enabled"], True) assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True) - self.set_mock_result(self.nodes[1], '0 {"invalid json"}') assert_raises_rpc_error(-1, 'Unable to parse JSON', - self.nodes[1].createwallet, wallet_name='hww2', disable_private_keys=True, external_signer=True + self.nodes[1].createwallet, wallet_name='hww2', external_signer=True ) self.clear_mock_result(self.nodes[1]) @@ -232,7 +241,7 @@ def test_disconnected_signer(self): self.log.info('Test disconnected external signer') # First create a wallet with the signer connected - self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True) + self.nodes[1].createwallet(wallet_name='hww_disconnect', external_signer=True) hww = self.nodes[1].get_wallet_rpc('hww_disconnect') assert_equal(hww.getwalletinfo()["external_signer"], True) @@ -253,13 +262,13 @@ def test_disconnected_signer(self): def test_invalid_signer(self): self.log.debug(f"-signer={self.mock_invalid_signer_path()}") self.log.info('Test invalid external signer') - assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', disable_private_keys=True, external_signer=True) + assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', external_signer=True) def test_multiple_signers(self): self.log.debug(f"-signer={self.mock_multi_signers_path()}") self.log.info('Test multiple external signers') - assert_raises_rpc_error(-1, "More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', disable_private_keys=True, external_signer=True) + assert_raises_rpc_error(-1, "More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', external_signer=True) if __name__ == '__main__': WalletSignerTest(__file__).main() From 1413480f2edfeea69a184be26adc5a58ac735e92 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 1 Aug 2025 10:46:50 +0200 Subject: [PATCH 18/66] wallet: make external_signer flag mutable With the removal of legacy wallets and the relaxing of restrictions in the previous commit, it's no longer a problem to toggle this flag. --- src/wallet/rpc/wallet.cpp | 6 ++++++ src/wallet/wallet.h | 3 ++- test/functional/wallet_avoidreuse.py | 3 +++ test/functional/wallet_signer.py | 13 +++++++------ 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 61b8bacab2f1..4df7b09c22bd 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -328,6 +328,12 @@ static RPCMethod setwalletflag() throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Wallet flag is immutable: %s", flag_str)); } +#ifndef ENABLE_EXTERNAL_SIGNER + if (flag == WALLET_FLAG_EXTERNAL_SIGNER && value) { + throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)"); + } +#endif + UniValue res(UniValue::VOBJ); if (pwallet->IsWalletFlagSet(flag) == value) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 61ba2dea2634..db0aaa110995 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -157,7 +157,8 @@ inline constexpr uint64_t KNOWN_WALLET_FLAGS = | WALLET_FLAG_EXTERNAL_SIGNER; inline constexpr uint64_t MUTABLE_WALLET_FLAGS = - WALLET_FLAG_AVOID_REUSE; + WALLET_FLAG_AVOID_REUSE + | WALLET_FLAG_EXTERNAL_SIGNER; inline const std::map WALLET_FLAG_TO_STRING{ {WALLET_FLAG_AVOID_REUSE, "avoid_reuse"}, diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py index 80d67683680b..bd564904d1d2 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/test/functional/wallet_avoidreuse.py @@ -139,6 +139,9 @@ def test_immutable(self): # Attempt to set the disable_private_keys flag; this should not work assert_raises_rpc_error(-8, "Wallet flag is immutable", self.nodes[1].setwalletflag, 'disable_private_keys') + if not self.is_external_signer_compiled(): + assert_raises_rpc_error(-4, "Compiled without external signing support", self.nodes[1].setwalletflag, "external_signer") + tempwallet = ".wallet_avoidreuse.py_test_immutable_wallet.dat" # Create a wallet with disable_private_keys set; this should work diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 1aa4e39579d3..aa285d79e0d0 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -87,13 +87,14 @@ def test_valid_signer(self): assert_equal(hww_blank.listdescriptors()["descriptors"], []) self.nodes[1].unloadwallet('hww_blank') - # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets) - self.nodes[1].createwallet(wallet_name='not_hww', external_signer=False) - not_hww = self.nodes[1].get_wallet_rpc('not_hww') - assert_equal(not_hww.getwalletinfo()["external_signer"], False) + # Flag can be set afterwards + self.nodes[1].createwallet(wallet_name='not_hww_initially', external_signer=False) + not_hww_initially = self.nodes[1].get_wallet_rpc('not_hww_initially') + assert_equal(not_hww_initially.getwalletinfo()["external_signer"], False) # Without external_signer, private keys are enabled by default - assert_equal(not_hww.getwalletinfo()["private_keys_enabled"], True) - assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True) + assert_equal(not_hww_initially.getwalletinfo()["private_keys_enabled"], True) + not_hww_initially.setwalletflag("external_signer", True) + assert_equal(not_hww_initially.getwalletinfo()["external_signer"], True) self.set_mock_result(self.nodes[1], '0 {"invalid json"}') assert_raises_rpc_error(-1, 'Unable to parse JSON', From af00263548dce47faf64c7ad7ccab534cf6e83bd Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 26 Aug 2026 10:27:50 +0200 Subject: [PATCH 19/66] wallet: extract load and unload wallet RPC helpers A later commit reuses these helpers to reload a wallet. This does not change behavior. --- src/wallet/rpc/wallet.cpp | 92 ++++++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 36 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 4df7b09c22bd..b86b970786aa 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -219,6 +220,53 @@ static RPCMethod listwallets() }; } +struct WalletLoadResult { + std::shared_ptr wallet; + std::vector warnings; +}; + +static WalletLoadResult LoadWalletForRPC(WalletContext& context, const std::string& name, std::optional load_on_start) +{ + DatabaseOptions options; + DatabaseStatus status; + ReadDatabaseArgs(*context.args, options); + options.require_existing = true; + bilingual_str error; + std::vector warnings; + + { + LOCK(context.wallets_mutex); + if (std::any_of(context.wallets.begin(), context.wallets.end(), [&name](const auto& wallet) { return wallet->GetName() == name; })) { + throw JSONRPCError(RPC_WALLET_ALREADY_LOADED, "Wallet \"" + name + "\" is already loaded."); + } + } + + std::shared_ptr wallet{LoadWallet(context, name, load_on_start, options, status, error, warnings)}; + HandleWalletError(wallet, status, error); + return {std::move(wallet), std::move(warnings)}; +} + +static std::vector UnloadWallet(WalletContext& context, std::shared_ptr wallet, std::optional load_on_start, std::unique_ptr reserver = nullptr) +{ + if (!reserver) { + reserver = std::make_unique(*wallet); + if (!reserver->reserve()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); + } + } else { + CHECK_NONFATAL(reserver->isReserved()); + } + + std::vector warnings; + if (!RemoveWallet(context, wallet, load_on_start, warnings)) { + throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); + } + + reserver.reset(); + WaitForDeleteWallet(std::move(wallet)); + return warnings; +} + static RPCMethod loadwallet() { return RPCMethod{ @@ -256,28 +304,12 @@ static RPCMethod loadwallet() WalletContext& context = EnsureWalletContext(request.context); const std::string name(request.params[0].get_str()); - DatabaseOptions options; - DatabaseStatus status; - ReadDatabaseArgs(*context.args, options); - options.require_existing = true; - bilingual_str error; - std::vector warnings; std::optional load_on_start = request.params[1].isNull() ? std::nullopt : std::optional(request.params[1].get_bool()); - - { - LOCK(context.wallets_mutex); - if (std::any_of(context.wallets.begin(), context.wallets.end(), [&name](const auto& wallet) { return wallet->GetName() == name; })) { - throw JSONRPCError(RPC_WALLET_ALREADY_LOADED, "Wallet \"" + name + "\" is already loaded."); - } - } - - std::shared_ptr const wallet = LoadWallet(context, name, load_on_start, options, status, error, warnings); - - HandleWalletError(wallet, status, error); + WalletLoadResult load_result{LoadWalletForRPC(context, name, load_on_start)}; UniValue obj(UniValue::VOBJ); - obj.pushKV("name", wallet->GetName()); - PushWarnings(warnings, obj); + obj.pushKV("name", load_result.wallet->GetName()); + PushWarnings(load_result.warnings, obj); return obj; }, @@ -484,23 +516,11 @@ static RPCMethod unloadwallet() throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded"); } - std::vector warnings; - { - WalletRescanReserver reserver(*wallet); - if (!reserver.reserve()) { - throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); - } - - // Release the "main" shared pointer and prevent further notifications. - // Note that any attempt to load the same wallet would fail until the wallet - // is destroyed (see CheckUniqueFileid). - std::optional load_on_start{self.MaybeArg("load_on_startup")}; - if (!RemoveWallet(context, wallet, load_on_start, warnings)) { - throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); - } - } - - WaitForDeleteWallet(std::move(wallet)); + // Release the "main" shared pointer and prevent further notifications. + // Note that any attempt to load the same wallet would fail until the wallet + // is destroyed (see CheckUniqueFileid). + std::optional load_on_start{self.MaybeArg("load_on_startup")}; + std::vector warnings{UnloadWallet(context, std::move(wallet), load_on_start)}; UniValue result(UniValue::VOBJ); PushWarnings(warnings, result); From f470ba91cee244d93cee6250bcbc83d645704c5b Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 26 Aug 2026 10:47:33 +0200 Subject: [PATCH 20/66] wallet: report whether flag changes require reload A later commit uses this signal to reload the wallet after changing flags that affect in-memory state. Existing callers ignore the return value, so this does not change behavior. --- src/wallet/wallet.cpp | 14 +++++++++----- src/wallet/wallet.h | 15 +++++++++------ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 40804025f497..6e2ff84fa6d5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1737,32 +1737,36 @@ bool CWallet::CanGetAddresses(bool internal) const return false; } -void CWallet::SetWalletFlag(uint64_t flags) +bool CWallet::SetWalletFlag(uint64_t flags) { WalletBatch batch(GetDatabase()); return SetWalletFlagWithDB(batch, flags); } -void CWallet::SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags) +bool CWallet::SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags) { LOCK(cs_wallet); + const uint64_t flags_before{m_wallet_flags}; m_wallet_flags |= flags; if (!batch.WriteWalletFlags(m_wallet_flags)) throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed"); + return ((flags_before ^ m_wallet_flags) & WALLET_FLAGS_REQUIRING_RELOAD) != 0; } -void CWallet::UnsetWalletFlag(uint64_t flag) +bool CWallet::UnsetWalletFlag(uint64_t flag) { WalletBatch batch(GetDatabase()); - UnsetWalletFlagWithDB(batch, flag); + return UnsetWalletFlagWithDB(batch, flag); } -void CWallet::UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag) +bool CWallet::UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag) { LOCK(cs_wallet); + const uint64_t flags_before{m_wallet_flags}; m_wallet_flags &= ~flag; if (!batch.WriteWalletFlags(m_wallet_flags)) throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed"); + return ((flags_before ^ m_wallet_flags) & WALLET_FLAGS_REQUIRING_RELOAD) != 0; } void CWallet::UnsetBlankWalletFlag(WalletBatch& batch) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index db0aaa110995..63e9b49ea0dc 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -160,6 +160,9 @@ inline constexpr uint64_t MUTABLE_WALLET_FLAGS = WALLET_FLAG_AVOID_REUSE | WALLET_FLAG_EXTERNAL_SIGNER; +inline constexpr uint64_t WALLET_FLAGS_REQUIRING_RELOAD = + WALLET_FLAG_EXTERNAL_SIGNER; + inline const std::map WALLET_FLAG_TO_STRING{ {WALLET_FLAG_AVOID_REUSE, "avoid_reuse"}, {WALLET_FLAG_BLANK_WALLET, "blank"}, @@ -384,7 +387,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional& strPurpose); //! Unsets a wallet flag and saves it to disk - void UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag); + bool UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag); //! Unset the blank wallet flag and saves it to disk void UnsetBlankWalletFlag(WalletBatch& batch) override; @@ -429,7 +432,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal); /** Store wallet flags */ - void SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags); + bool SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags); //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms std::unordered_map, SaltedSipHasher> m_cached_spks; @@ -911,11 +914,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati */ void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet); - /** set a single wallet flag */ - void SetWalletFlag(uint64_t flags); + /** Set wallet flags. Returns whether the wallet needs to be reloaded. */ + bool SetWalletFlag(uint64_t flags); - /** Unsets a single wallet flag */ - void UnsetWalletFlag(uint64_t flag); + /** Unset wallet flags. Returns whether the wallet needs to be reloaded. */ + bool UnsetWalletFlag(uint64_t flag); /** check if a certain wallet flag is set */ bool IsWalletFlagSet(uint64_t flag) const override; From cd3bfc969225941a8292d8d4b5d7c570a9b23048 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 26 Aug 2026 10:47:45 +0200 Subject: [PATCH 21/66] wallet: reload wallet when external signer flag changes Have setwalletflag unload and reload the wallet when a flag setter reports this is needed. Reuse the normal wallet loading path to recreate descriptor ScriptPubKeyMans. Document which flags trigger a reload and warn users to avoid concurrent wallet RPC clients while changing them. --- src/wallet/rpc/wallet.cpp | 44 ++++++++++++++++++++++++++++---- test/functional/wallet_signer.py | 4 +++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index b86b970786aa..8a86473a80bc 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -267,6 +268,15 @@ static std::vector UnloadWallet(WalletContext& context, std::shar return warnings; } +static std::vector ReloadWallet(WalletContext& context, std::shared_ptr wallet, std::unique_ptr reserver) +{ + const std::string wallet_name{wallet->GetName()}; + std::vector warnings{UnloadWallet(context, std::move(wallet), /*load_on_start=*/std::nullopt, std::move(reserver))}; + WalletLoadResult load_result{LoadWalletForRPC(context, wallet_name, /*load_on_start=*/std::nullopt)}; + warnings.insert(warnings.end(), load_result.warnings.begin(), load_result.warnings.end()); + return warnings; +} + static RPCMethod loadwallet() { return RPCMethod{ @@ -323,9 +333,16 @@ static RPCMethod setwalletflag() if (it.second & MUTABLE_WALLET_FLAGS) flags += (flags == "" ? "" : ", ") + it.first; + std::string reload_flags; + for (auto& it : STRING_TO_WALLET_FLAG) + if (it.second & WALLET_FLAGS_REQUIRING_RELOAD) + reload_flags += (reload_flags == "" ? "" : ", ") + it.first; + return RPCMethod{ "setwalletflag", - "Change the state of the given wallet flag for a wallet.\n", + "Change the state of the given wallet flag for a wallet.\n" + "The following flags trigger a wallet reload: " + reload_flags + ".\n" + "Make sure no other RPC clients are using the wallet when changing these flags.\n", { {"flag", RPCArg::Type::STR, RPCArg::Optional::NO, "The name of the flag to change. Current available flags: " + flags}, {"value", RPCArg::Type::BOOL, RPCArg::Default{true}, "The new state."}, @@ -344,7 +361,7 @@ static RPCMethod setwalletflag() }, [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return UniValue::VNULL; std::string flag_str = request.params[0].get_str(); @@ -372,18 +389,35 @@ static RPCMethod setwalletflag() throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Wallet flag is already set to %s: %s", value ? "true" : "false", flag_str)); } + std::unique_ptr rescan_reserver; + if (flag & WALLET_FLAGS_REQUIRING_RELOAD) { + rescan_reserver = std::make_unique(*pwallet); + if (!rescan_reserver->reserve()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); + } + } + res.pushKV("flag_name", flag_str); res.pushKV("flag_state", value); + bool reload_wallet; if (value) { - pwallet->SetWalletFlag(flag); + reload_wallet = pwallet->SetWalletFlag(flag); } else { - pwallet->UnsetWalletFlag(flag); + reload_wallet = pwallet->UnsetWalletFlag(flag); } + std::vector warnings; if (flag && value && WALLET_FLAG_CAVEATS.contains(flag)) { - res.pushKV("warnings", WALLET_FLAG_CAVEATS.at(flag)); + warnings.push_back(WALLET_FLAG_CAVEATS.at(flag)); + } + if (reload_wallet) { + WalletContext& context{EnsureWalletContext(request.context)}; + for (const bilingual_str& reload_warning : ReloadWallet(context, std::move(pwallet), std::move(rescan_reserver))) { + warnings.push_back(reload_warning.original); + } } + if (!warnings.empty()) res.pushKV("warnings", util::Join(warnings, "\n")); return res; }, diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index aa285d79e0d0..3fa2dd438df9 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -132,6 +132,10 @@ def test_valid_signer(self): assert_equal(address_info['ismine'], True) assert_equal(address_info['hdkeypath'], "m/86h/1h/0h/0/0") + hww.setwalletflag("external_signer", False) + assert_raises_rpc_error(-1, "There is no ScriptPubKeyManager for this address", hww.walletdisplayaddress, address1) + hww.setwalletflag("external_signer", True) + self.log.info('Test walletdisplayaddress') for address in [address1, address2, address3]: result = hww.walletdisplayaddress(address) From 517824b93c34d1d6c2bbb531839688be87fe0ea8 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 6 May 2026 11:03:37 +0200 Subject: [PATCH 22/66] test: move mock signer path helper to the test framework Both rpc_signer.py and wallet_signer.py defined identical mock_signer_path() helpers; the upcoming wallet_signer_musig2.py test needs the same helper. Move it to BitcoinTestFramework. --- test/functional/rpc_signer.py | 5 ---- .../test_framework/test_framework.py | 5 ++++ test/functional/wallet_signer.py | 29 ++++--------------- 3 files changed, 11 insertions(+), 28 deletions(-) diff --git a/test/functional/rpc_signer.py b/test/functional/rpc_signer.py index 6d51182c6bf2..fbec9597f183 100755 --- a/test/functional/rpc_signer.py +++ b/test/functional/rpc_signer.py @@ -9,7 +9,6 @@ """ import os import platform -import sys from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -19,10 +18,6 @@ class RPCSignerTest(BitcoinTestFramework): - def mock_signer_path(self): - path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py') - return sys.executable + " " + path - def set_test_params(self): self.num_nodes = 4 diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 70aeb4266621..ac57574c0cc0 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -1118,6 +1118,11 @@ def skip_if_no_external_signer(self): if not self.is_external_signer_compiled(): raise SkipTest("external signer support has not been compiled.") + def mock_signer_path(self, name='signer.py'): + """Return a command that invokes a mock external signer script under test/functional/mocks/.""" + path = os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', 'mocks', name) + return sys.executable + " " + os.path.realpath(path) + def skip_if_running_under_valgrind(self): """Skip the running test if Valgrind is being used.""" if self.options.valgrind: diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 3fa2dd438df9..648101a4905a 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -8,7 +8,6 @@ See also rpc_signer.py for tests without wallet context. """ import os -import sys from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -19,22 +18,6 @@ class WalletSignerTest(BitcoinTestFramework): - def mock_signer_path(self): - path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py') - return sys.executable + " " + path - - def mock_no_connected_signer_path(self): - path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'no_signer.py') - return sys.executable + " " + path - - def mock_invalid_signer_path(self): - path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'invalid_signer.py') - return sys.executable + " " + path - - def mock_multi_signers_path(self): - path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'multi_signers.py') - return sys.executable + " " + path - def set_test_params(self): self.num_nodes = 2 @@ -57,9 +40,9 @@ def clear_mock_result(self, node): def run_test(self): self.test_valid_signer() self.test_disconnected_signer() - self.restart_node(1, [f"-signer={self.mock_invalid_signer_path()}", "-keypool=10"]) + self.restart_node(1, [f"-signer={self.mock_signer_path('invalid_signer.py')}", "-keypool=10"]) self.test_invalid_signer() - self.restart_node(1, [f"-signer={self.mock_multi_signers_path()}", "-keypool=10"]) + self.restart_node(1, [f"-signer={self.mock_signer_path('multi_signers.py')}", "-keypool=10"]) self.test_multiple_signers() def test_valid_signer(self): @@ -255,8 +238,8 @@ def test_disconnected_signer(self): self.generate(self.nodes[0], 1) # Restart node with no signer connected - self.log.debug(f"-signer={self.mock_no_connected_signer_path()}") - self.restart_node(1, [f"-signer={self.mock_no_connected_signer_path()}", "-keypool=10"]) + self.log.debug(f"-signer={self.mock_signer_path('no_signer.py')}") + self.restart_node(1, [f"-signer={self.mock_signer_path('no_signer.py')}", "-keypool=10"]) self.nodes[1].loadwallet('hww_disconnect') hww = self.nodes[1].get_wallet_rpc('hww_disconnect') @@ -265,12 +248,12 @@ def test_disconnected_signer(self): assert_raises_rpc_error(-25, "External signer not found", hww.send, outputs=[{dest:0.5}]) def test_invalid_signer(self): - self.log.debug(f"-signer={self.mock_invalid_signer_path()}") + self.log.debug(f"-signer={self.mock_signer_path('invalid_signer.py')}") self.log.info('Test invalid external signer') assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', external_signer=True) def test_multiple_signers(self): - self.log.debug(f"-signer={self.mock_multi_signers_path()}") + self.log.debug(f"-signer={self.mock_signer_path('multi_signers.py')}") self.log.info('Test multiple external signers') assert_raises_rpc_error(-1, "More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', external_signer=True) From f4dd05644f3eada3656f619f71ab3b94fe08ec3d Mon Sep 17 00:00:00 2001 From: Sjors Date: Wed, 29 Apr 2026 12:03:11 +0200 Subject: [PATCH 23/66] wallet: upgrade to ExternalSignerScriptPubKeyMan in AddWalletDescriptor CWallet::AddWalletDescriptor created a plain DescriptorScriptPubKeyMan even when WALLET_FLAG_EXTERNAL_SIGNER was set. Create the external-signer variant immediately so newly imported descriptors can use address display and signing without requiring an unload/reload cycle. --- src/wallet/external_signer_scriptpubkeyman.cpp | 9 +++++++++ src/wallet/external_signer_scriptpubkeyman.h | 5 +++++ src/wallet/scriptpubkeyman.h | 14 +++++++------- src/wallet/wallet.cpp | 7 ++++++- test/functional/wallet_signer.py | 18 ++++++++++++++++++ 5 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 758be582ac4a..ba7af0f93d6f 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -26,6 +26,15 @@ std::unique_ptr ExternalSignerScriptPubKeyMan::Lo return std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); } +std::unique_ptr ExternalSignerScriptPubKeyMan::CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider) +{ + auto spkm = std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, descriptor, keypool_size)); + if (auto res = spkm->UpdateWalletDescriptor(descriptor, provider); !res) { + throw std::runtime_error(util::ErrorString(res).original); + } + return spkm; +} + std::unique_ptr ExternalSignerScriptPubKeyMan::CreateNew(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc) { auto spkm = std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, keypool_size)); diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 8a3ae3df7a2b..e509f8923350 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -21,12 +21,17 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan : DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys) {} + ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) + : DescriptorScriptPubKeyMan(storage, descriptor, keypool_size) + {} + ExternalSignerScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : DescriptorScriptPubKeyMan(storage, keypool_size) {} public: static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + static std::unique_ptr CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); static std::unique_ptr CreateNew(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc); static util::Result GetExternalSigner(); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 5c977b12d404..d1e89d6e8d69 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -301,13 +301,6 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan */ mutable std::map m_musig2_secnonces; - //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import) - DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) - : ScriptPubKeyMan(storage), - m_keypool_size(keypool_size), - m_wallet_descriptor(descriptor) - {} - bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); @@ -328,6 +321,13 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan void SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); protected: + //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import) + DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) + : ScriptPubKeyMan(storage), + m_keypool_size(keypool_size), + m_wallet_descriptor(descriptor) + {} + //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6e2ff84fa6d5..7e494bf3bf89 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3807,7 +3807,12 @@ util::Result> CWallet::AddWall return util::Error{util::ErrorString(spkm_res)}; } } else { - auto new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider); + std::unique_ptr new_spk_man; + if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { + new_spk_man = ExternalSignerScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider); + } else { + new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider); + } spk_man = new_spk_man.get(); // Save the descriptor to memory diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 648101a4905a..d8cdeb45123f 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -9,6 +9,7 @@ """ import os +from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -39,6 +40,7 @@ def clear_mock_result(self, node): def run_test(self): self.test_valid_signer() + self.test_import_descriptor() self.test_disconnected_signer() self.restart_node(1, [f"-signer={self.mock_signer_path('invalid_signer.py')}", "-keypool=10"]) self.test_invalid_signer() @@ -224,6 +226,22 @@ def test_valid_signer(self): assert_greater_than(res["fee"], res["origfee"]) assert_equal(res["errors"], []) + def test_import_descriptor(self): + self.log.info('Test using the signer for an imported descriptor, without reloading') + + self.nodes[1].createwallet(wallet_name='hww_import', external_signer=True, blank=True) + hww_import = self.nodes[1].get_wallet_rpc('hww_import') + xpub = "tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B" + result = hww_import.importdescriptors([{ + "desc": descsum_create(f"wpkh([00000001/84h/1h/0h]{xpub}/0/*)"), + "active": True, + "timestamp": "now", + }]) + assert_equal(result[0]["success"], True) + + address = hww_import.getnewaddress(address_type="bech32") + assert_equal(hww_import.walletdisplayaddress(address), {"address": address}) + self.nodes[1].unloadwallet('hww_import') def test_disconnected_signer(self): self.log.info('Test disconnected external signer') From 5a7c4309da24d9dd3ff5a8f7edc3d316cd9355c0 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Aug 2026 09:19:20 +0200 Subject: [PATCH 24/66] wallet: sign with own keys before using the external signer ExternalSignerScriptPubKeyMan::FillPSBT went straight to the external signer whenever sign is set. Now that a signer wallet can hold private keys, a descriptor with a hot key gets an ExternalSignerScriptPubKeyMan as well, and its signature was never made. Let the base class fill and sign first, and only involve the signer if an input that belongs to this descriptor is still unsigned. --- .../external_signer_scriptpubkeyman.cpp | 19 +++++++++++--- test/functional/wallet_signer.py | 25 +++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index ba7af0f93d6f..282f4f67d857 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -97,13 +97,24 @@ util::Result ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestin // If sign is true, transaction must previously have been filled std::optional ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, const common::PSBTFillOptions& options, int* n_signed) const { - if (!options.sign) { - return DescriptorScriptPubKeyMan::FillPSBT(psbt, txdata, options, n_signed); - } + // Fill in metadata. The base class only signs if options.sign is set, and + // only with keys we hold ourselves. + if (auto err = DescriptorScriptPubKeyMan::FillPSBT(psbt, txdata, options, n_signed)) return err; + if (!options.sign) return {}; - // Already complete if every input is now signed + // No need for an external signer roundtrip if we already have all the + // signatures we need. bool complete = true; for (const auto& input : psbt.inputs) { + CTxOut utxo; + // An externally created PSBT may not include the UTXO for inputs we + // don't know, in which case we can't tell whether it's ours. Leave it + // to the signer. + if (input.GetUTXO(utxo)) { + // Only consider inputs that belong to this descriptor; the wallet + // may hold the private key for the other inputs. + if (!IsMine(utxo.scriptPubKey)) continue; + } complete &= PSBTInputSigned(input); } if (complete) return {}; diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index d8cdeb45123f..fb8f4f1f687e 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -10,6 +10,7 @@ import os from test_framework.descriptors import descsum_create +from test_framework.extendedkey import ExtendedPrivateKey from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -41,6 +42,7 @@ def clear_mock_result(self, node): def run_test(self): self.test_valid_signer() self.test_import_descriptor() + self.test_hot_key() self.test_disconnected_signer() self.restart_node(1, [f"-signer={self.mock_signer_path('invalid_signer.py')}", "-keypool=10"]) self.test_invalid_signer() @@ -243,6 +245,29 @@ def test_import_descriptor(self): assert_equal(hww_import.walletdisplayaddress(address), {"address": address}) self.nodes[1].unloadwallet('hww_import') + def test_hot_key(self): + self.log.info('Test spending a hot key coin in an external signer wallet') + + self.nodes[1].createwallet(wallet_name='hww_hot_key', external_signer=True, disable_private_keys=False) + hww_hot_key = self.nodes[1].get_wallet_rpc('hww_hot_key') + hot_desc = descsum_create(f"wpkh({ExtendedPrivateKey.generate().to_string()}/<0;1>/*)") + result = hww_hot_key.importdescriptors([{ + "desc": hot_desc, + "active": True, + "timestamp": "now", + }]) + assert_equal(result[0]["success"], True) + + hot_address = hww_hot_key.getnewaddress(address_type="bech32") + self.nodes[0].sendtoaddress(hot_address, 1) + self.generate(self.nodes[0], 1) + hot_utxo = hww_hot_key.listunspent(addresses=[hot_address])[0] + + dest = self.nodes[0].getnewaddress() + res = hww_hot_key.send(outputs={dest: 0.5}, inputs=[hot_utxo], add_inputs=False, add_to_wallet=False) + assert res["complete"] + assert self.nodes[1].testmempoolaccept([res["hex"]])[0]["allowed"] + def test_disconnected_signer(self): self.log.info('Test disconnected external signer') From 7c79fd2e01ce74a39a93bc316c4399cc9ebd3669 Mon Sep 17 00:00:00 2001 From: Thomas Date: Tue, 25 Aug 2026 11:53:34 +0200 Subject: [PATCH 25/66] psbt: preserve sighash type when merging inputs `PSBTInput::Merge` copies every optional input field from the other input when it is absent locally, except `PSBT_IN_SIGHASH_TYPE`. So `combinepsbt` silently drops the sighash type whenever the first PSBT does not carry it, making the result depend on the argument order. The field is what lets finalizers enforce the sighash type of existing signatures (BIP 174). When it is lost, `FinalizePSBT` falls back to the default type (`SIGHASH_ALL`, or `SIGHASH_DEFAULT` for taproot inputs), rejects signatures made with any other type as a sighash mismatch, and the PSBT can no longer be finalized. Combining a PSBT signed with `ALL|ANYONECANPAY` after a merely updated copy of the same PSBT reproduces this: `finalizepsbt` reports it as incomplete, while the reverse order finalizes and broadcasts fine. Merge the sighash type like the other optional fields, keeping the one already present, and test both combine orders. --- src/psbt.cpp | 1 + test/functional/rpc_psbt.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/psbt.cpp b/src/psbt.cpp index c9f0311634e5..18e2d0a7f84a 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -464,6 +464,7 @@ bool PSBTInput::Merge(const PSBTInput& input) for (const auto& [agg_key_lh, psigs] : input.m_musig2_partial_sigs) { m_musig2_partial_sigs[agg_key_lh].insert(psigs.begin(), psigs.end()); } + if (sighash_type == std::nullopt && input.sighash_type != std::nullopt) sighash_type = input.sighash_type; if (sequence == std::nullopt && input.sequence != std::nullopt) sequence = input.sequence; if (time_locktime == std::nullopt && input.time_locktime != std::nullopt) time_locktime = input.time_locktime; if (height_locktime == std::nullopt && input.height_locktime != std::nullopt) height_locktime = input.height_locktime; diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 23273d54b56f..6f17088a097b 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -557,6 +557,34 @@ def test_sighash_adding(self): wallet.unloadwallet() + def test_combinepsbt_sighash_type(self): + self.log.info("Test that combining PSBTs preserves the sighash type field regardless of order") + node = self.nodes[0] + node.createwallet("combine_sighash") + wallet = node.get_wallet_rpc("combine_sighash") + def_wallet = node.get_wallet_rpc(self.default_wallet_name) + + def_wallet.send([{wallet.getnewaddress(address_type="bech32"): 1}]) + self.generate(node, 1) + psbt = wallet.walletcreatefundedpsbt(wallet.listunspent(), [{def_wallet.getnewaddress(): 0.5}])["psbt"] + + signed = wallet.walletprocesspsbt(psbt=psbt, sighashtype="ALL|ANYONECANPAY", finalize=False)["psbt"] + assert_equal(node.decodepsbt(signed)["inputs"][0].get("sighash"), "ALL|ANYONECANPAY") + updated = wallet.walletprocesspsbt(psbt=psbt, sign=False)["psbt"] + assert "sighash" not in node.decodepsbt(updated)["inputs"][0] + + finalized = [] + for psbts in [[signed, updated], [updated, signed]]: + combined = node.combinepsbt(psbts) + assert_equal(node.decodepsbt(combined)["inputs"][0].get("sighash"), "ALL|ANYONECANPAY") + fin_res = node.finalizepsbt(combined) + assert_equal(fin_res["complete"], True) + assert_equal(node.testmempoolaccept([fin_res["hex"]])[0]["allowed"], True) + finalized.append(fin_res["hex"]) + assert_equal(finalized[0], finalized[1]) + + wallet.unloadwallet() + def assert_change_type(self, psbtx, expected_type): """Assert that the given PSBT has a change output with the given type.""" @@ -1619,6 +1647,7 @@ def global_xpub_key(extended_pubkey): if not self.options.usecli: self.test_sighash_mismatch() self.test_sighash_adding() + self.test_combinepsbt_sighash_type() self.test_psbt_named_parameter_handling() self.test_psbt_roundtrip() self.test_psbt_version() From 9d774ffaa5c08d1478b38a15906cb57729d99cad Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Aug 2026 11:43:48 +0200 Subject: [PATCH 26/66] psbt: fix rendering for invalid long sighash type field The PSBT sighash type field is a 32 bit unsigned integer in BIP 174, signed in PSBTInput, and it is not validated when deserialized. decodepsbt incorrectly truncates this field before looking up its name. Fix that and add a test. --- src/core_io.cpp | 8 ++++++-- src/core_io.h | 3 ++- src/rpc/rawtransaction.cpp | 2 +- test/functional/rpc_psbt.py | 9 +++++++++ 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/core_io.cpp b/src/core_io.cpp index 3650d70810a9..cbaf24171237 100644 --- a/src/core_io.cpp +++ b/src/core_io.cpp @@ -338,9 +338,13 @@ const std::map mapSigHashTypes = { {static_cast(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY), std::string("SINGLE|ANYONECANPAY")}, }; -std::string SighashToStr(unsigned char sighash_type) +std::string SighashToStr(int32_t sighash_type) { - const auto& it = mapSigHashTypes.find(sighash_type); + // Signatures encode the sighash type in a single byte, but the PSBT field + // for it is a 32 bit unsigned integer in BIP 174 (signed in PSBTInput) + if (sighash_type < 0 || sighash_type > 0xff) return ""; + const uint8_t sighash_byte(sighash_type); + const auto& it = mapSigHashTypes.find(sighash_byte); if (it == mapSigHashTypes.end()) return ""; return it->second; } diff --git a/src/core_io.h b/src/core_io.h index 904f5a8643b9..d78706216c20 100644 --- a/src/core_io.h +++ b/src/core_io.h @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -42,7 +43,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header); UniValue ValueFromAmount(CAmount amount); std::string FormatScript(const CScript& script); std::string EncodeHexTx(const CTransaction& tx); -std::string SighashToStr(unsigned char sighash_type); +std::string SighashToStr(int32_t sighash_type); void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false, const SigningProvider* provider = nullptr); void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex = true, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS, std::function is_change_func = {}); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index e9eae656b515..d64384d4ec9a 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1224,7 +1224,7 @@ static RPCMethod decodepsbt() // Sighash if (input.sighash_type != std::nullopt) { - in.pushKV("sighash", SighashToStr((unsigned char)*input.sighash_type)); + in.pushKV("sighash", SighashToStr(*input.sighash_type)); } // Redeem script and witness script diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 6f17088a097b..e4f16491a41f 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -585,6 +585,14 @@ def test_combinepsbt_sighash_type(self): wallet.unloadwallet() + def test_decodepsbt_long_sighash_type(self): + self.log.info("Test that decodepsbt rejects invalid trailing bytes in the sighash type field") + node = self.nodes[0] + psbt = PSBT.from_base64(node.createpsbt([{"txid": "00" * 32, "vout": 0}], [{"data": "00"}])) + # The first byte of this sighash type is ALL, but the type itself is not + psbt.i[0].map[PSBT_IN_SIGHASH_TYPE] = (0x101).to_bytes(4, "little") + assert_equal(node.decodepsbt(psbt.to_base64())["inputs"][0]["sighash"], "") + def assert_change_type(self, psbtx, expected_type): """Assert that the given PSBT has a change output with the given type.""" @@ -1648,6 +1656,7 @@ def global_xpub_key(extended_pubkey): self.test_sighash_mismatch() self.test_sighash_adding() self.test_combinepsbt_sighash_type() + self.test_decodepsbt_long_sighash_type() self.test_psbt_named_parameter_handling() self.test_psbt_roundtrip() self.test_psbt_version() From 3ec0242ffc89c51c333208f99f113d2b8f55453c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Aug 2026 10:36:36 +0200 Subject: [PATCH 27/66] test: have external signer mock use a wallet The external signer mock previously replayed a PSBT that the test prepared in advance. This makes it difficult to test more complicated scenarios like a misbehaving wallet and (MuSig2) multisig. Instead, give the mock its own descriptor wallet. The test provides a dedicated node for this wallet and keeps it offline, so the mock can't cheat by e.g. inspecting the UTXO set. The mock creates the wallet on first use and signs with walletprocesspsbt. wallet_signer.py now funds all four descriptor types and spends them in a single transaction, exercising every signing code path. --- test/functional/mocks/signer.py | 48 +++++++++++++---- test/functional/wallet_signer.py | 92 +++++++++++++------------------- 2 files changed, 75 insertions(+), 65 deletions(-) diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index a13c97d1216f..79cd505fb2cc 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -8,6 +8,17 @@ import argparse import json +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.realpath(__file__)))) + +from test_framework.authproxy import AuthServiceProxy, JSONRPCException + +# Master private key for the tpub in getdescriptors below. Used by signtx, +# which imports the keys into a wallet on the offline node provided by the +# test and lets it do the actual signing. +tprv = "tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK" + +MOCK_WALLET = "mock" + def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") if os.path.isfile(mock_result_path): @@ -56,20 +67,37 @@ def displayaddress(args): return sys.stdout.write(json.dumps({"address": expected_desc[args.desc]})) +def get_mock_wallet(): + """RPC connection to the wallet holding our private keys, created on + first use. The test provides a dedicated offline node for it and passes + the node's RPC URL via a file in our working directory.""" + with open(os.path.join(os.getcwd(), "mock_rpc_url"), "r", encoding="utf8") as f: + node_url = f.read().strip() + node = AuthServiceProxy(node_url) + wallet = AuthServiceProxy(f"{node_url}/wallet/{MOCK_WALLET}") + try: + node.loadwallet(filename=MOCK_WALLET) + return wallet + except JSONRPCException as e: + if e.error["code"] == -35: # RPC_WALLET_ALREADY_LOADED + return wallet + if e.error["code"] != -18: # RPC_WALLET_NOT_FOUND + raise + node.createwallet(wallet_name=MOCK_WALLET, blank=True) + requests = [] + for desc in [f"pkh({tprv}/<0;1>/*)", f"sh(wpkh({tprv}/<0;1>/*))", f"wpkh({tprv}/<0;1>/*)", f"tr({tprv}/<0;1>/*)"]: + checksum = node.getdescriptorinfo(descriptor=desc)["checksum"] + requests.append({"desc": f"{desc}#{checksum}", "timestamp": "now", "range": [0, 99]}) + result = wallet.importdescriptors(requests=requests) + assert all(r["success"] for r in result) + return wallet + def signtx(args): if args.fingerprint != "00000001": return sys.stdout.write(json.dumps({"error": "Unexpected fingerprint", "fingerprint": args.fingerprint})) - with open(os.path.join(os.getcwd(), "mock_psbt"), "r") as f: - mock_psbt = f.read() - - if args.fingerprint == "00000001" : - sys.stdout.write(json.dumps({ - "psbt": mock_psbt, - "complete": True - })) - else: - sys.stdout.write(json.dumps({"psbt": args.psbt})) + result = get_mock_wallet().walletprocesspsbt(psbt=args.psbt, sign=True, bip32derivs=False, finalize=False) + sys.stdout.write(json.dumps({"psbt": result["psbt"]})) parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock') parser.add_argument('--fingerprint') diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 896169d4276a..061e757cb163 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -36,13 +36,25 @@ def mock_multi_signers_path(self): return sys.executable + " " + path def set_test_params(self): - self.num_nodes = 2 + self.num_nodes = 3 self.extra_args = [ [], [f"-signer={self.mock_signer_path()}", '-keypool=10'], + # Node for the signer mock's wallet, kept offline so the mock + # can't cheat by e.g. inspecting the UTXO set + ["-maxconnections=0"], ] + def setup_network(self): + self.setup_nodes() + # Leave the signer mock's node disconnected + self.connect_nodes(0, 1) + + def sync_except_mock(self): + """Sync all nodes except the signer mock's, which never receives blocks.""" + self.sync_all(self.nodes[0:2]) + def skip_test_if_missing_module(self): self.skip_if_no_external_signer() self.skip_if_no_wallet() @@ -54,7 +66,16 @@ def set_mock_result(self, node, res): def clear_mock_result(self, node): os.remove(os.path.join(node.cwd, "mock_result")) + def init_mock_node(self): + """Hand the signer mock its dedicated offline node, on which it + creates the wallet it signs with.""" + signer_node = self.nodes[2] + assert_equal(signer_node.getconnectioncount(), 0) + with open(os.path.join(self.nodes[1].cwd, "mock_rpc_url"), "w") as f: + f.write(signer_node.url) + def run_test(self): + self.init_mock_node() self.test_valid_signer() self.test_disconnected_signer() self.restart_node(1, [f"-signer={self.mock_invalid_signer_path()}", "-keypool=10"]) @@ -149,73 +170,34 @@ def test_valid_signer(self): hww.walletdisplayaddress, address_fail ) - self.log.info('Prepare mock PSBT') - self.nodes[0].sendtoaddress(address4, 1) - self.generate(self.nodes[0], 1) - - # Load private key into wallet to generate a signed PSBT for the mock - self.nodes[1].createwallet(wallet_name="mock", disable_private_keys=False, blank=True) - mock_wallet = self.nodes[1].get_wallet_rpc("mock") - assert mock_wallet.getwalletinfo()['private_keys_enabled'] - - result = mock_wallet.importdescriptors([{ - "desc": "tr([00000001/86h/1h/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#7ew68cn8", - "timestamp": 0, - "range": [0,1], - "internal": False, - "active": True - }, - { - "desc": "tr([00000001/86h/1h/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/*)#0dtm6drl", - "timestamp": 0, - "range": [0, 0], - "internal": True, - "active": True - }]) - assert_equal(result[0], {'success': True}) - assert_equal(result[1], {'success': True}) - assert_equal(mock_wallet.getwalletinfo()["txcount"], 1) - dest = self.nodes[0].getnewaddress(address_type='bech32') - mock_psbt = mock_wallet.walletcreatefundedpsbt([], {dest:0.5}, 0, {'replaceable': True}, True)['psbt'] - mock_psbt_signed = mock_wallet.walletprocesspsbt(psbt=mock_psbt, sign=True, sighashtype="ALL", bip32derivs=True) - mock_tx = mock_psbt_signed["hex"] - assert mock_wallet.testmempoolaccept([mock_tx])[0]["allowed"] - - assert_equal(hww.getwalletinfo()["txcount"], 1) - - assert hww.testmempoolaccept([mock_tx])[0]["allowed"] + self.log.info('Fund hww wallet') + for address in [address1, address2, address3, address4]: + self.nodes[0].sendtoaddress(address, 1) + self.generate(self.nodes[0], 1, sync_fun=self.sync_except_mock) + assert_equal(hww.getwalletinfo()["txcount"], 4) - with open(os.path.join(self.nodes[1].cwd, "mock_psbt"), "w") as f: - f.write(mock_psbt_signed["psbt"]) + dest = self.nodes[0].getnewaddress(address_type='bech32') self.log.info('Test send using hww1') - # Don't broadcast transaction yet so the RPC returns the raw hex - res = hww.send(outputs={dest:0.5},add_to_wallet=False) + # Spend all four address types at once. Don't broadcast the transaction + # yet so the RPC returns the raw hex. + res = hww.send(outputs={dest:3.5}, add_to_wallet=False) assert res["complete"] - assert_equal(res["hex"], mock_tx) + assert_equal(len(hww.decoderawtransaction(res["hex"])["vin"]), 4) + assert hww.testmempoolaccept([res["hex"]])[0]["allowed"] self.log.info('Test sendall using hww1') - res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()], add_to_wallet=False) + res = hww.sendall(recipients=[{dest:3.5}, hww.getrawchangeaddress()], add_to_wallet=False) assert res["complete"] - assert_equal(res["hex"], mock_tx) + assert hww.testmempoolaccept([res["hex"]])[0]["allowed"] # Broadcast transaction so we can bump the fee hww.sendrawtransaction(res["hex"]) - self.log.info('Prepare fee bumped mock PSBT') - - # Now that the transaction is broadcast, bump fee in mock wallet: - orig_tx_id = res["txid"] - mock_psbt_bumped = mock_wallet.psbtbumpfee(orig_tx_id)["psbt"] - mock_psbt_bumped_signed = mock_wallet.walletprocesspsbt(psbt=mock_psbt_bumped, sign=True, sighashtype="ALL", bip32derivs=True) - - with open(os.path.join(self.nodes[1].cwd, "mock_psbt"), "w") as f: - f.write(mock_psbt_bumped_signed["psbt"]) - self.log.info('Test bumpfee using hww1') - # Bump fee + orig_tx_id = res["txid"] res = hww.bumpfee(orig_tx_id) assert_greater_than(res["fee"], res["origfee"]) assert_equal(res["errors"], []) @@ -231,7 +213,7 @@ def test_disconnected_signer(self): # Fund wallet self.nodes[0].sendtoaddress(hww.getnewaddress(address_type="bech32m"), 1) - self.generate(self.nodes[0], 1) + self.generate(self.nodes[0], 1, sync_fun=self.sync_except_mock) # Restart node with no signer connected self.log.debug(f"-signer={self.mock_no_connected_signer_path()}") From f04a43678737f01a6ed83129af9c1f37c8834164 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Aug 2026 10:37:08 +0200 Subject: [PATCH 28/66] external_signer: merge PSBT response instead of replacing Previously the PSBT returned by the external signer replaced the original wholesale, trusting the signer not to modify the transaction. Merge it instead. This rejects a response that describes a different transaction. Merging also supports signers that strip fields they don't need from their response. New tests cover both scenarios. Co-authored-by: brunoerg --- src/external_signer.cpp | 5 ++- test/functional/mocks/signer.py | 50 ++++++++++++++++++++++++++-- test/functional/wallet_signer.py | 57 ++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index cb49cedc69c2..3002b90df991 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -121,7 +121,10 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str return false; } - psbtx = *signer_psbtx; + if (!psbtx.Merge(*signer_psbtx)) { + error = "Signer returned a PSBT for a different transaction"; + return false; + } return true; } diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index 79cd505fb2cc..8803c27b3a25 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -11,6 +11,13 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.realpath(__file__)))) from test_framework.authproxy import AuthServiceProxy, JSONRPCException +from test_framework.psbt import ( + PSBT, + PSBT_IN_PARTIAL_SIG, + PSBT_IN_TAP_KEY_SIG, + PSBT_OUT_AMOUNT, + PSBT_OUT_SCRIPT, +) # Master private key for the tpub in getdescriptors below. Used by signtx, # which imports the keys into a wallet on the offline node provided by the @@ -92,12 +99,51 @@ def get_mock_wallet(): assert all(r["success"] for r in result) return wallet +def tamper(psbt_b64, mode): + """Alter the transaction described by the (version 2) PSBT before signing + it, like a rogue or broken signer might.""" + psbt = PSBT.from_base64(psbt_b64) + if mode == "change_amount": + # Steal from the output by redirecting the value to fees + amount = int.from_bytes(psbt.o[0].map[PSBT_OUT_AMOUNT], "little", signed=True) + psbt.o[0].map[PSBT_OUT_AMOUNT] = (amount - 1).to_bytes(8, "little", signed=True) + elif mode == "change_script": + psbt.o[0].map[PSBT_OUT_SCRIPT] = bytes([0x51]) # OP_TRUE + elif mode == "remove_output": + psbt.o.pop() + return psbt.to_base64() + def signtx(args): if args.fingerprint != "00000001": return sys.stdout.write(json.dumps({"error": "Unexpected fingerprint", "fingerprint": args.fingerprint})) - result = get_mock_wallet().walletprocesspsbt(psbt=args.psbt, sign=True, bip32derivs=False, finalize=False) - sys.stdout.write(json.dumps({"psbt": result["psbt"]})) + # The test can instruct us to sign in a specific, possibly misbehaving, way + mode = None + sign_mode_path = os.path.join(os.getcwd(), "mock_sign_mode") + if os.path.isfile(sign_mode_path): + with open(sign_mode_path, "r", encoding="utf8") as f: + mode = f.read().strip() + + psbt = args.psbt + if mode in ("change_amount", "change_script", "remove_output"): + psbt = tamper(psbt, mode) + + result = get_mock_wallet().walletprocesspsbt(psbt=psbt, sign=True, bip32derivs=False, finalize=False) + reply = result["psbt"] + + if mode == "strip": + # Return only the signatures, plus the fields required to describe + # the same transaction + signed = PSBT.from_base64(reply) + stripped = PSBT.from_base64(reply) + stripped.make_blank() + for signed_in, stripped_in in zip(signed.i, stripped.i): + for key, value in signed_in.map.items(): + if key == PSBT_IN_TAP_KEY_SIG or (isinstance(key, bytes) and key[0] == PSBT_IN_PARTIAL_SIG): + stripped_in.map[key] = value + reply = stripped.to_base64() + + sys.stdout.write(json.dumps({"psbt": reply})) parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock') parser.add_argument('--fingerprint') diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 061e757cb163..b7675eb018f4 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -66,6 +66,13 @@ def set_mock_result(self, node, res): def clear_mock_result(self, node): os.remove(os.path.join(node.cwd, "mock_result")) + def set_mock_sign_mode(self, node, mode): + with open(os.path.join(node.cwd, "mock_sign_mode"), "w") as f: + f.write(mode) + + def clear_mock_sign_mode(self, node): + os.remove(os.path.join(node.cwd, "mock_sign_mode")) + def init_mock_node(self): """Hand the signer mock its dedicated offline node, on which it creates the wallet it signs with.""" @@ -77,6 +84,8 @@ def init_mock_node(self): def run_test(self): self.init_mock_node() self.test_valid_signer() + self.test_unusual_signer() + self.test_misbehaving_signer() self.test_disconnected_signer() self.restart_node(1, [f"-signer={self.mock_invalid_signer_path()}", "-keypool=10"]) self.test_invalid_signer() @@ -203,6 +212,54 @@ def test_valid_signer(self): assert_equal(res["errors"], []) + def test_unusual_signer(self): + self.log.info('Test unusual but acceptable external signer behavior') + hww = self.nodes[1].get_wallet_rpc('hww') + + # Spend a segwit and a taproot UTXO, to cover both ECDSA and schnorr + # signatures + addresses = [hww.getnewaddress(address_type="bech32"), hww.getnewaddress(address_type="bech32m")] + for address in addresses: + self.nodes[0].sendtoaddress(address, 1) + self.generate(self.nodes[0], 1, sync_fun=self.sync_except_mock) + inputs = [{"txid": utxo["txid"], "vout": utxo["vout"]} for utxo in hww.listunspent(addresses=addresses)] + assert_equal(len(inputs), 2) + dest = self.nodes[0].getnewaddress() + + self.log.info('The signer may strip fields it does not need') + self.set_mock_sign_mode(self.nodes[1], "strip") + res = hww.send(outputs={dest: 1.5}, inputs=inputs, add_inputs=False, add_to_wallet=False) + assert res["complete"] + assert hww.testmempoolaccept([res["hex"]])[0]["allowed"] + + self.clear_mock_sign_mode(self.nodes[1]) + + def test_misbehaving_signer(self): + self.log.info('Test misbehaving external signer') + hww = self.nodes[1].get_wallet_rpc('hww') + + # Spend a segwit and a taproot UTXO, to cover both ECDSA and schnorr + # signatures + addresses = [hww.getnewaddress(address_type="bech32"), hww.getnewaddress(address_type="bech32m")] + for address in addresses: + self.nodes[0].sendtoaddress(address, 1) + self.generate(self.nodes[0], 1, sync_fun=self.sync_except_mock) + inputs = [{"txid": utxo["txid"], "vout": utxo["vout"]} for utxo in hww.listunspent(addresses=addresses)] + assert_equal(len(inputs), 2) + dest = self.nodes[0].getnewaddress() + + self.log.info('The signer must not tamper with the transaction') + for mode in ["change_amount", "change_script", "remove_output"]: + self.set_mock_sign_mode(self.nodes[1], mode) + with self.nodes[1].assert_debug_log(["Signer returned a PSBT for a different transaction"]): + assert_raises_rpc_error(-25, "External signer failed to sign", hww.send, outputs={dest: 1.5}, inputs=inputs, add_inputs=False) + + self.clear_mock_sign_mode(self.nodes[1]) + + # The same transaction is accepted from a well-behaved signer + res = hww.send(outputs={dest: 1.5}, inputs=inputs, add_inputs=False, add_to_wallet=False) + assert res["complete"] + def test_disconnected_signer(self): self.log.info('Test disconnected external signer') From 79fb761ed88b1c9e261cffe666a2eeb2ee5a4005 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Aug 2026 10:38:11 +0200 Subject: [PATCH 29/66] external_signer: reject unsafe sighash types A signature with SIGHASH_NONE or SIGHASH_SINGLE doesn't commit to all outputs, letting anyone alter them after signing. Reject a PSBT from an external signer that declares such a sighash type or contains signatures made with one. SIGHASH_ANYONECANPAY is still accepted: it only permits adding inputs, which does not affect us. The mock signer produces real signatures for these scenarios by letting its wallet sign with the requested sighash type, optionally hiding the declared sighash type field so that only the signatures themselves reveal it. Co-authored-by: brunoerg --- src/external_signer.cpp | 36 ++++++++++++++++++++++++++++++++ test/functional/mocks/signer.py | 18 ++++++++++++++-- test/functional/wallet_signer.py | 14 +++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index 3002b90df991..3aaaf92b3c50 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -8,10 +8,13 @@ #include #include #include +#include