From eb9a1a2c595a03475fd4275b104676b7e2200f07 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 15 Nov 2021 13:54:01 -0500 Subject: [PATCH 1/3] psbt: Make sighash_type std::optional It is better to ues an optional to determine whether the sighash type is set rather than using 0 as a magic number. --- src/psbt.h | 10 ++++++---- src/rpc/rawtransaction.cpp | 4 ++-- src/wallet/scriptpubkeyman.cpp | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/psbt.h b/src/psbt.h index 1171ecf1dd..21daa050ea 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -57,7 +57,7 @@ struct PSBTInput std::map hd_keypaths; std::map partial_sigs; std::map, std::vector> unknown; - int sighash_type = 0; + std::optional sighash_type; bool IsNull() const; void FillSignatureData(SignatureData& sigdata) const; @@ -86,9 +86,9 @@ struct PSBTInput } // Write the sighash type - if (sighash_type > 0) { + if (sighash_type != std::nullopt) { SerializeToVector(s, PSBT_IN_SIGHASH); - SerializeToVector(s, sighash_type); + SerializeToVector(s, *sighash_type); } // Write the redeem script @@ -201,7 +201,9 @@ struct PSBTInput } else if (key.size() != 1) { throw std::ios_base::failure("Sighash type key is more than one byte type"); } - UnserializeFromVector(s, sighash_type); + int sighash; + UnserializeFromVector(s, sighash); + sighash_type = sighash; break; case PSBT_IN_REDEEMSCRIPT: { diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index a3583e7f12..713b0afcdc 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1255,8 +1255,8 @@ static RPCHelpMan decodepsbt() } // Sighash - if (input.sighash_type > 0) { - in.pushKV("sighash", SighashToStr((unsigned char)input.sighash_type)); + if (input.sighash_type != std::nullopt) { + in.pushKV("sighash", SighashToStr((unsigned char)*input.sighash_type)); } // Redeem script and witness script diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1769429efe..a9e2624188 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -633,7 +633,7 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb } // Get the Sighash type - if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) { + if (sign && input.sighash_type != std::nullopt && *input.sighash_type != sighash_type) { return TransactionError::SIGHASH_MISMATCH; } @@ -2114,7 +2114,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& } // Get the Sighash type - if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) { + if (sign && input.sighash_type != std::nullopt && *input.sighash_type != sighash_type) { return TransactionError::SIGHASH_MISMATCH; } From d3992669df826899a3de78a77a366dab46028026 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Jul 2021 22:05:28 -0400 Subject: [PATCH 2/3] psbt: Actually use SIGHASH_DEFAULT Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs. --- src/core_read.cpp | 2 +- src/wallet/scriptpubkeyman.h | 6 +++--- src/wallet/wallet.h | 2 +- test/functional/rpc_psbt.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core_read.cpp b/src/core_read.cpp index 2149b428d2..484f41f262 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -248,7 +248,7 @@ std::vector ParseHexUV(const UniValue& v, const std::string& strN int ParseSighashString(const UniValue& sighash) { - int hash_type = SIGHASH_ALL; + int hash_type = SIGHASH_DEFAULT; if (!sighash.isNull()) { static std::map map_sighash_values = { {std::string("DEFAULT"), int(SIGHASH_DEFAULT)}, diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ebe064fa0a..dda31d2c0b 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -236,7 +236,7 @@ public: /** Sign a message with the given script */ virtual SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const { return SigningResult::SIGNING_FAILED; }; /** Adds script and derivation path information to a PSBT, and optionally signs it. */ - virtual TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const { return TransactionError::INVALID_PSBT; } + virtual TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const { return TransactionError::INVALID_PSBT; } virtual uint256 GetID() const { return uint256(); } @@ -400,7 +400,7 @@ public: bool SignTransaction(CMutableTransaction& tx, const std::map& coins, int sighash, std::map& input_errors) const override; SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override; - TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; + TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; uint256 GetID() const override; @@ -609,7 +609,7 @@ public: bool SignTransaction(CMutableTransaction& tx, const std::map& coins, int sighash, std::map& input_errors) const override; SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override; - TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; + TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; uint256 GetID() const override; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index dbf0f6375d..93150cc583 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -561,7 +561,7 @@ public: */ TransactionError FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, - int sighash_type = 1 /* SIGHASH_ALL */, + int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = true, size_t* n_signed = nullptr, diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index a8034849cc..153a201e95 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -457,7 +457,7 @@ class PSBTTest(BitcoinTestFramework): wrpc = self.nodes[2].get_wallet_rpc("wallet{}".format(i)) for key in signer['privkeys']: wrpc.importprivkey(key) - signed_tx = wrpc.walletprocesspsbt(signer['psbt'])['psbt'] + signed_tx = wrpc.walletprocesspsbt(signer['psbt'], True, "ALL")['psbt'] assert_equal(signed_tx, signer['result']) # Combiner test From c0405ee27fa23a9868f04c052bdc94d7accc57e2 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 19 Nov 2021 13:13:27 -0500 Subject: [PATCH 3/3] rpc: Document that DEFAULT is for Taproot, ALL for everything else --- src/rpc/rawtransaction.cpp | 2 +- src/wallet/rpc/spend.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 713b0afcdc..5c161c7299 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -785,7 +785,7 @@ static RPCHelpMan signrawtransactionwithkey() }, }, }, - {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT"}, "The signature hash type. Must be one of:\n" + {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type. Must be one of:\n" " \"DEFAULT\"\n" " \"ALL\"\n" " \"NONE\"\n" diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index a22c175032..62b0a839c1 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -709,7 +709,7 @@ RPCHelpMan signrawtransactionwithwallet() }, }, }, - {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT"}, "The signature hash type. Must be one of\n" + {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type. Must be one of\n" " \"DEFAULT\"\n" " \"ALL\"\n" " \"NONE\"\n" @@ -1167,7 +1167,7 @@ RPCHelpMan walletprocesspsbt() { {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction base64 string"}, {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)"}, - {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n" + {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n" " \"DEFAULT\"\n" " \"ALL\"\n" " \"NONE\"\n"