From d6001dcd4ada5b64c8113450ed736a2581c97518 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 8 Jan 2025 17:48:41 -0500 Subject: [PATCH] wallet: change FillPSBT to take sighash as optional Instead of having the caller have to figure out the correct sane default to provide to FillPSBT, have FillPSBT do that by having it take the sighash type as an optional. This further allows it to distinguish between an explicit sighash type being provided and expecting the default value to be used. --- src/interfaces/wallet.h | 2 +- src/qt/psbtoperationsdialog.cpp | 6 +++--- src/qt/sendcoinsdialog.cpp | 6 +++--- src/qt/walletmodel.cpp | 2 +- src/wallet/external_signer_scriptpubkeyman.cpp | 2 +- src/wallet/external_signer_scriptpubkeyman.h | 2 +- src/wallet/feebumper.cpp | 4 ++-- src/wallet/interfaces.cpp | 2 +- src/wallet/rpc/spend.cpp | 8 ++++---- src/wallet/scriptpubkeyman.cpp | 5 +++-- src/wallet/scriptpubkeyman.h | 4 ++-- src/wallet/test/fuzz/scriptpubkeyman.cpp | 3 ++- src/wallet/test/psbt_wallet_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 5 +++-- src/wallet/wallet.h | 2 +- 15 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 9fc8ab0ef51..89477aa16d1 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -204,7 +204,7 @@ public: int& num_blocks) = 0; //! Fill PSBT. - virtual std::optional fillPSBT(int sighash_type, + virtual std::optional fillPSBT(std::optional sighash_type, bool sign, bool bip32derivs, size_t* n_signed, diff --git a/src/qt/psbtoperationsdialog.cpp b/src/qt/psbtoperationsdialog.cpp index 454d9287aea..afe6a8999ac 100644 --- a/src/qt/psbtoperationsdialog.cpp +++ b/src/qt/psbtoperationsdialog.cpp @@ -59,7 +59,7 @@ void PSBTOperationsDialog::openWithPSBT(PartiallySignedTransaction psbtx) bool complete = FinalizePSBT(psbtx); // Make sure all existing signatures are fully combined before checking for completeness. if (m_wallet_model) { size_t n_could_sign; - const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, &n_could_sign, m_transaction_data, complete)}; + const auto err{m_wallet_model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, &n_could_sign, m_transaction_data, complete)}; if (err) { showStatus(tr("Failed to load transaction: %1") .arg(QString::fromStdString(PSBTErrorString(*err).translated)), @@ -83,7 +83,7 @@ void PSBTOperationsDialog::signTransaction() WalletModel::UnlockContext ctx(m_wallet_model->requestUnlock()); - const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_DEFAULT, /*sign=*/true, /*bip32derivs=*/true, &n_signed, m_transaction_data, complete)}; + const auto err{m_wallet_model->wallet().fillPSBT(std::nullopt, /*sign=*/true, /*bip32derivs=*/true, &n_signed, m_transaction_data, complete)}; if (err) { showStatus(tr("Failed to sign transaction: %1") @@ -251,7 +251,7 @@ size_t PSBTOperationsDialog::couldSignInputs(const PartiallySignedTransaction &p size_t n_signed; bool complete; - const auto err{m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/false, &n_signed, m_transaction_data, complete)}; + const auto err{m_wallet_model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/false, &n_signed, m_transaction_data, complete)}; if (err) { return 0; diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index b8a9854f0e0..c2ebb875de6 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -450,7 +450,7 @@ void SendCoinsDialog::presentPSBT(PartiallySignedTransaction& psbtx) bool SendCoinsDialog::signWithExternalSigner(PartiallySignedTransaction& psbtx, CMutableTransaction& mtx, bool& complete) { std::optional err; try { - err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/true, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); + err = model->wallet().fillPSBT(std::nullopt, /*sign=*/true, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); } catch (const std::runtime_error& e) { QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); return false; @@ -507,7 +507,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) PartiallySignedTransaction psbtx(mtx); bool complete = false; // Fill without signing - const auto err{model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)}; + const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)}; assert(!complete); assert(!err); @@ -523,7 +523,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) bool complete = false; // Always fill without signing first. This prevents an external signer // from being called prematurely and is not expensive. - const auto err{model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)}; + const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)}; assert(!complete); assert(!err); send_failure = !signWithExternalSigner(psbtx, mtx, complete); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 07d5b9413c3..4483144582f 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -519,7 +519,7 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash) // "Create Unsigned" clicked PartiallySignedTransaction psbtx(mtx); bool complete = false; - const auto err{wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)}; + const auto err{wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)}; if (err || complete) { QMessageBox::critical(nullptr, tr("Fee bump error"), tr("Can't draft transaction.")); return false; diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 32e9941453b..7268354f1a9 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -79,7 +79,7 @@ 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, int sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const +std::optional ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const { if (!sign) { return DescriptorScriptPubKeyMan::FillPSBT(psbt, txdata, sighash_type, false, bip32derivs, n_signed, finalize); diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 10d67d2ab47..66c2dd9d170 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -35,7 +35,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan */ util::Result DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const; - std::optional 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; + std::optional FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional sighash_type = std::nullopt, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; }; } // namespace wallet #endif // BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 0a58285a969..f4c8be938e2 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -338,8 +338,8 @@ bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) { // First fill transaction with our data without signing, // so external signers are not asked to sign more than once. bool complete; - wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, false /* sign */, true /* bip32derivs */); - auto err{wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, true /* sign */, false /* bip32derivs */)}; + wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true); + auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)}; if (err) return false; complete = FinalizeAndExtractPSBT(psbtx, mtx); return complete; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 5308b4418d7..0f8925f77ee 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -383,7 +383,7 @@ public: } return {}; } - std::optional fillPSBT(int sighash_type, + std::optional fillPSBT(std::optional sighash_type, bool sign, bool bip32derivs, size_t* n_signed, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index a59d274b176..4626a02da32 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -100,8 +100,8 @@ static UniValue FinishTransaction(const std::shared_ptr pwallet, const // First fill transaction with our data without signing, // so external signers are not asked to sign more than once. bool complete; - pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, /*sign=*/false, /*bip32derivs=*/true); - const auto err{pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, /*sign=*/true, /*bip32derivs=*/false)}; + pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true); + const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)}; if (err) { throw JSONRPCPSBTError(*err); } @@ -1175,7 +1175,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name) } else { PartiallySignedTransaction psbtx(mtx); bool complete = false; - const auto err{pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, /*sign=*/false, /*bip32derivs=*/true)}; + const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true)}; CHECK_NONFATAL(!err); CHECK_NONFATAL(!complete); DataStream ssTx{}; @@ -1777,7 +1777,7 @@ RPCHelpMan walletcreatefundedpsbt() // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool(); bool complete = true; - const auto err{wallet.FillPSBT(psbtx, complete, 1, /*sign=*/false, /*bip32derivs=*/bip32derivs)}; + const auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/bip32derivs)}; if (err) { throw JSONRPCPSBTError(*err); } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index c9d52c18dff..0d20a73871b 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1311,11 +1311,12 @@ SigningResult DescriptorScriptPubKeyMan::SignMessage(const std::string& message, return SigningResult::OK; } -std::optional DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, int sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const +std::optional DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, std::optional sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const { if (n_signed) { *n_signed = 0; } + if (!sighash_type) sighash_type = SIGHASH_DEFAULT; for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { const CTxIn& txin = psbtx.tx->vin[i]; PSBTInput& input = psbtx.inputs.at(i); @@ -1386,7 +1387,7 @@ std::optional DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTran } } - PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize); + PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, *sighash_type, nullptr, finalize); if (res != PSBTError::OK && res != PSBTError::INCOMPLETE) { return res; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index d8aac3ed02f..13b8233879c 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -153,7 +153,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 std::optional 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 common::PSBTError::UNSUPPORTED; } + virtual std::optional FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional sighash_type = std::nullopt, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const { return common::PSBTError::UNSUPPORTED; } virtual uint256 GetID() const { return uint256(); } @@ -382,7 +382,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; - std::optional 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; + std::optional FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, std::optional sighash_type = std::nullopt, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; uint256 GetID() const override; diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index 63768c89afc..6a1c51290b3 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -192,7 +192,8 @@ FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm) } auto psbt{*opt_psbt}; const PrecomputedTransactionData txdata{PrecomputePSBTData(psbt)}; - const int sighash_type{fuzzed_data_provider.ConsumeIntegralInRange(0, 150)}; + std::optional sighash_type{fuzzed_data_provider.ConsumeIntegralInRange(0, 151)}; + if (sighash_type == 151) sighash_type = std::nullopt; auto sign = fuzzed_data_provider.ConsumeBool(); auto bip32derivs = fuzzed_data_provider.ConsumeBool(); auto finalize = fuzzed_data_provider.ConsumeBool(); diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp index c44ea496339..63153fb55db 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -65,7 +65,7 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) // Fill transaction with our data bool complete = true; - BOOST_REQUIRE(!m_wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, false, true)); + BOOST_REQUIRE(!m_wallet.FillPSBT(psbtx, complete, std::nullopt, false, true)); // Get the final tx DataStream ssTx{}; @@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) // Try to sign the mutated input SignatureData sigdata; - BOOST_CHECK(m_wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, true, true)); + BOOST_CHECK(m_wallet.FillPSBT(psbtx, complete, std::nullopt, true, true)); } BOOST_AUTO_TEST_CASE(parse_hd_keypath) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3c5df177679..215d7b505cd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2101,11 +2101,12 @@ bool CWallet::SignTransaction(CMutableTransaction& tx, const std::map CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, int sighash_type, bool sign, bool bip32derivs, size_t * n_signed, bool finalize) const +std::optional CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, std::optional sighash_type, bool sign, bool bip32derivs, size_t * n_signed, bool finalize) const { if (n_signed) { *n_signed = 0; } + if (!sighash_type) sighash_type = SIGHASH_DEFAULT; LOCK(cs_wallet); // Get all of the previous transactions for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { @@ -2144,7 +2145,7 @@ std::optional CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bo } } - RemoveUnnecessaryTransactions(psbtx, sighash_type); + RemoveUnnecessaryTransactions(psbtx, *sighash_type); // Complete if every input is now signed complete = true; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e32b8c7272b..c6b56bd9ef0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -659,7 +659,7 @@ public: */ std::optional FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, - int sighash_type = SIGHASH_DEFAULT, + std::optional sighash_type = std::nullopt, bool sign = true, bool bip32derivs = true, size_t* n_signed = nullptr,