From fe5d22bc676f158e8d567d71edb3451118759d62 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:18:52 -0700 Subject: [PATCH 1/7] More concise conversion of CDataStream to string Use .str() instead of .data() and .size() when converting CDataStream to a string. Uses std::string, avoiding conversion to a C string. --- src/rpc/rawtransaction.cpp | 4 ++-- src/wallet/rpcwallet.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index a2d990b51..4fa345017 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1554,10 +1554,10 @@ UniValue finalizepsbt(const JSONRPCRequest& request) mtx.vin[i].scriptWitness = psbtx.inputs[i].final_script_witness; } ssTx << mtx; - result.pushKV("hex", HexStr(ssTx.begin(), ssTx.end())); + result.pushKV("hex", HexStr(ssTx.str())); } else { ssTx << psbtx; - result.pushKV("psbt", EncodeBase64((unsigned char*)ssTx.data(), ssTx.size())); + result.pushKV("psbt", EncodeBase64(ssTx.str())); } result.pushKV("complete", complete); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2aeb5c298..5f3fece32 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3842,7 +3842,7 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request) UniValue result(UniValue::VOBJ); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << psbtx; - result.pushKV("psbt", EncodeBase64((unsigned char*)ssTx.data(), ssTx.size())); + result.pushKV("psbt", EncodeBase64(ssTx.str())); result.pushKV("complete", complete); return result; @@ -3956,7 +3956,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) ssTx << psbtx; UniValue result(UniValue::VOBJ); - result.pushKV("psbt", EncodeBase64((unsigned char*)ssTx.data(), ssTx.size())); + result.pushKV("psbt", EncodeBase64(ssTx.str())); result.pushKV("fee", ValueFromAmount(fee)); result.pushKV("changepos", change_position); return result; From 4f3f5cb4b142f0fcb36241fa33b52a257901dbee Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:24:55 -0700 Subject: [PATCH 2/7] Remove redundant txConst parameter to FillPSBT --- src/wallet/rpcwallet.cpp | 22 +++++++--------------- src/wallet/rpcwallet.h | 2 +- src/wallet/test/psbt_wallet_tests.cpp | 6 +----- 3 files changed, 9 insertions(+), 21 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5f3fece32..e2ffb42d7 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3735,13 +3735,13 @@ void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::mapcs_wallet); // Get all of the previous transactions bool complete = true; - for (unsigned int i = 0; i < txConst->vin.size(); ++i) { - const CTxIn& txin = txConst->vin[i]; + for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { + const CTxIn& txin = psbtx.tx->vin[i]; PSBTInput& input = psbtx.inputs.at(i); // If we don't know about this input, skip it and let someone else deal with it @@ -3764,8 +3764,8 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C } // Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change - for (unsigned int i = 0; i < txConst->vout.size(); ++i) { - const CTxOut& out = txConst->vout.at(i); + for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) { + const CTxOut& out = psbtx.tx->vout.at(i); PSBTOutput& psbt_out = psbtx.outputs.at(i); // Fill a SignatureData with output info @@ -3830,14 +3830,10 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request) // Get the sighash type int nHashType = ParseSighashString(request.params[2]); - // Use CTransaction for the constant parts of the - // transaction to avoid rehashing. - const CTransaction txConst(*psbtx.tx); - // Fill transaction with our data and also sign bool sign = request.params[1].isNull() ? true : request.params[1].get_bool(); bool bip32derivs = request.params[3].isNull() ? false : request.params[3].get_bool(); - bool complete = FillPSBT(pwallet, psbtx, &txConst, nHashType, sign, bip32derivs); + bool complete = FillPSBT(pwallet, psbtx, nHashType, sign, bip32derivs); UniValue result(UniValue::VOBJ); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); @@ -3943,13 +3939,9 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) psbtx.outputs.push_back(PSBTOutput()); } - // Use CTransaction for the constant parts of the - // transaction to avoid rehashing. - const CTransaction txConst(*psbtx.tx); - // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? false : request.params[4].get_bool(); - FillPSBT(pwallet, psbtx, &txConst, 1, false, bip32derivs); + FillPSBT(pwallet, psbtx, 1, false, bip32derivs); // Serialize the PSBT CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); diff --git a/src/wallet/rpcwallet.h b/src/wallet/rpcwallet.h index 9b9a159b8..54477ef3a 100644 --- a/src/wallet/rpcwallet.h +++ b/src/wallet/rpcwallet.h @@ -30,5 +30,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException); UniValue getaddressinfo(const JSONRPCRequest& request); UniValue signrawtransactionwithwallet(const JSONRPCRequest& request); -bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type = 1, bool sign = true, bool bip32derivs = false); +bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1, bool sign = true, bool bip32derivs = false); #endif //BITCOIN_WALLET_RPCWALLET_H diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp index 34d6e1b87..b14c4f653 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -59,12 +59,8 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) CDataStream ssData(ParseHex("70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f000000000000000000"), SER_NETWORK, PROTOCOL_VERSION); ssData >> psbtx; - // Use CTransaction for the constant parts of the - // transaction to avoid rehashing. - const CTransaction txConst(*psbtx.tx); - // Fill transaction with our data - FillPSBT(&m_wallet, psbtx, &txConst, 1, false, true); + FillPSBT(&m_wallet, psbtx, 1, false, true); // Get the final tx CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); From 65166d4cf828909dc4bc49dd68a58103d015f1fd Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:26:16 -0700 Subject: [PATCH 3/7] New PartiallySignedTransaction constructor from CTransction New constructor that creates a PartiallySignedTransaction from a CTransaction, automatically sizing the inputs and outputs vectors for convenience. --- src/script/sign.cpp | 6 ++++++ src/script/sign.h | 1 + src/wallet/rpcwallet.cpp | 9 +-------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 89cc7c808..bcf110896 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -513,6 +513,12 @@ bool IsSolvable(const SigningProvider& provider, const CScript& script) return false; } +PartiallySignedTransaction::PartiallySignedTransaction(const CTransaction& tx) : tx(tx) +{ + inputs.resize(tx.vin.size()); + outputs.resize(tx.vout.size()); +} + bool PartiallySignedTransaction::IsNull() const { return !tx && inputs.empty() && outputs.empty() && unknown.empty(); diff --git a/src/script/sign.h b/src/script/sign.h index d47aada17..381e2eb12 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -566,6 +566,7 @@ struct PartiallySignedTransaction bool IsSane() const; PartiallySignedTransaction() {} PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {} + explicit PartiallySignedTransaction(const CTransaction& tx); // Only checks if they refer to the same transaction friend bool operator==(const PartiallySignedTransaction& a, const PartiallySignedTransaction &b) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e2ffb42d7..b514f09b0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3930,14 +3930,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) FundTransaction(pwallet, rawTx, fee, change_position, request.params[3]); // Make a blank psbt - PartiallySignedTransaction psbtx; - psbtx.tx = rawTx; - for (unsigned int i = 0; i < rawTx.vin.size(); ++i) { - psbtx.inputs.push_back(PSBTInput()); - } - for (unsigned int i = 0; i < rawTx.vout.size(); ++i) { - psbtx.outputs.push_back(PSBTOutput()); - } + PartiallySignedTransaction psbtx(rawTx); // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? false : request.params[4].get_bool(); From 53e6fffb8f5b10f94708d33d667a67cb91c2d09d Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:28:48 -0700 Subject: [PATCH 4/7] Add bool PSBTInputSigned Refactor out a "PSBTInputSigned" function to check if a PSBT is signed, for use in subsequent commits. Also improve a related comment. --- src/rpc/rawtransaction.cpp | 7 +++++-- src/script/sign.cpp | 8 ++++++-- src/script/sign.h | 3 +++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 4fa345017..a6e94d54d 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1536,12 +1536,15 @@ UniValue finalizepsbt(const JSONRPCRequest& request) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error)); } - // Get all of the previous transactions + // Finalize input signatures -- in case we have partial signatures that add up to a complete + // signature, but have not combined them yet (e.g. because the combiner that created this + // PartiallySignedTransaction did not understand them), this will combine them into a final + // script. bool complete = true; for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { PSBTInput& input = psbtx.inputs.at(i); - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, 1); + complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, SIGHASH_ALL); } UniValue result(UniValue::VOBJ); diff --git a/src/script/sign.cpp b/src/script/sign.cpp index bcf110896..5cdc0112d 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -239,10 +239,14 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato return sigdata.complete; } +bool PSBTInputSigned(PSBTInput& input) +{ + return !input.final_script_sig.empty() || !input.final_script_witness.IsNull(); +} + bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, int index, int sighash) { - // if this input has a final scriptsig or scriptwitness, don't do anything with it - if (!input.final_script_sig.empty() || !input.final_script_witness.IsNull()) { + if (PSBTInputSigned(input)) { return true; } diff --git a/src/script/sign.h b/src/script/sign.h index 381e2eb12..35f7630f4 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -730,6 +730,9 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato bool SignSignature(const SigningProvider &provider, const CScript& fromPubKey, CMutableTransaction& txTo, unsigned int nIn, const CAmount& amount, int nHashType); bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom, CMutableTransaction& txTo, unsigned int nIn, int nHashType); +/** Checks whether a PSBTInput is already signed. */ +bool PSBTInputSigned(PSBTInput& input); + /** Signs a PSBTInput, verifying that all provided data matches what is being signed. */ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, int index, int sighash = SIGHASH_ALL); From 0f5bda2bd941686620ef0eb90bd7ed973cc7ef73 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:30:50 -0700 Subject: [PATCH 5/7] Simplify arguments to SignPSBTInput Remove redundant arguments to SignPSBTInput -- since it needs several bits of the PartiallySignedTransaction, pass in a reference instead of doing it piecemeal. This saves us having to pass in both a PSBTInput and its index, as well as having to pass in the CTransaction. Also avoid redundantly passing the sighash_type, which is contained in the PSBTInput already. --- src/rpc/rawtransaction.cpp | 4 +--- src/script/sign.cpp | 5 ++++- src/script/sign.h | 2 +- src/wallet/rpcwallet.cpp | 2 +- src/wallet/rpcwallet.h | 2 +- src/wallet/test/psbt_wallet_tests.cpp | 2 +- 6 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index a6e94d54d..f169f0917 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1542,9 +1542,7 @@ UniValue finalizepsbt(const JSONRPCRequest& request) // script. bool complete = true; for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { - PSBTInput& input = psbtx.inputs.at(i); - - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, SIGHASH_ALL); + complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, SIGHASH_ALL); } UniValue result(UniValue::VOBJ); diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 5cdc0112d..1e481dd54 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -244,8 +244,11 @@ bool PSBTInputSigned(PSBTInput& input) return !input.final_script_sig.empty() || !input.final_script_witness.IsNull(); } -bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, int index, int sighash) +bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash) { + PSBTInput& input = psbt.inputs.at(index); + const CMutableTransaction& tx = *psbt.tx; + if (PSBTInputSigned(input)) { return true; } diff --git a/src/script/sign.h b/src/script/sign.h index 35f7630f4..e71f43f96 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -734,7 +734,7 @@ bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom, bool PSBTInputSigned(PSBTInput& input); /** Signs a PSBTInput, verifying that all provided data matches what is being signed. */ -bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, int index, int sighash = SIGHASH_ALL); +bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash = SIGHASH_ALL); /** Extract signature data from a transaction input, and insert it. */ SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn, const CTxOut& txout); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b514f09b0..908d403cc 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3760,7 +3760,7 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sig throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Specified Sighash and sighash in PSBT do not match."); } - complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), *psbtx.tx, input, i, sighash_type); + complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), psbtx, i, sighash_type); } // Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change diff --git a/src/wallet/rpcwallet.h b/src/wallet/rpcwallet.h index 54477ef3a..abd775087 100644 --- a/src/wallet/rpcwallet.h +++ b/src/wallet/rpcwallet.h @@ -30,5 +30,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException); UniValue getaddressinfo(const JSONRPCRequest& request); UniValue signrawtransactionwithwallet(const JSONRPCRequest& request); -bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1, bool sign = true, bool bip32derivs = false); +bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false); #endif //BITCOIN_WALLET_RPCWALLET_H diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp index b14c4f653..7f195caaf 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -60,7 +60,7 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) ssData >> psbtx; // Fill transaction with our data - FillPSBT(&m_wallet, psbtx, 1, false, true); + FillPSBT(&m_wallet, psbtx, SIGHASH_ALL, false, true); // Get the final tx CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); From 565500508aa5df0011109ebf375ba71b693fc7de Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:31:41 -0700 Subject: [PATCH 6/7] Refactor PSBTInput signing to enforce invariant Refactor the process of PSBTInput signing to enforce the invariant that a PSBTInput always has _either_ a witness_utxo or a non_witness_utxo, never both. This simplifies the logic of SignPSBTInput slightly, since it no longer has to deal with the "both" case. When calling it, we now give it, in order of preference: (1) whichever of the utxo fields was already present in the PSBT we received, or (2) if neither, the non_witness_utxo field, which is just a copy of the input transaction, which we get from the wallet. SignPSBTInput no longer has to remove one of the two fields; instead, it will check if we have a witness signature, and if so, it will replace the non_witness_utxo with the witness_utxo (which is smaller, as it is just a copy of the output being spent.) Add PSBTInput::IsSane checks in two more places, which checks for both utxo fields being present; we will now give an RPC error early on if we are supplied such a malformed PSBT to fill in. Also add a check to FillPSBT, to avoid touching any input that is already signed. (This is now redundant, since we should no longer potentially harm an already-signed input, but it's harmless.) fixes #14473 --- src/script/sign.cpp | 30 +++++++++++++----------------- src/wallet/rpcwallet.cpp | 28 +++++++++++++++++++--------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 1e481dd54..69ee08ffd 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -260,15 +260,19 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& // Get UTXO bool require_witness_sig = false; CTxOut utxo; + + // Verify input sanity, which checks that at most one of witness or non-witness utxos is provided. + if (!input.IsSane()) { + return false; + } + if (input.non_witness_utxo) { // If we're taking our information from a non-witness UTXO, verify that it matches the prevout. - if (input.non_witness_utxo->GetHash() != tx.vin[index].prevout.hash) return false; - // If both witness and non-witness UTXO are provided, verify that they match. This check shouldn't - // matter, as the PSBT deserializer enforces only one of both is provided, and the only way both - // can be present is when they're added simultaneously by FillPSBT (in which case they always match). - // Still, check in order to not rely on callers to enforce this. - if (!input.witness_utxo.IsNull() && input.non_witness_utxo->vout[tx.vin[index].prevout.n] != input.witness_utxo) return false; - utxo = input.non_witness_utxo->vout[tx.vin[index].prevout.n]; + COutPoint prevout = tx.vin[index].prevout; + if (input.non_witness_utxo->GetHash() != prevout.hash) { + return false; + } + utxo = input.non_witness_utxo->vout[prevout.n]; } else if (!input.witness_utxo.IsNull()) { utxo = input.witness_utxo; // When we're taking our information from a witness UTXO, we can't verify it is actually data from @@ -287,18 +291,10 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& if (require_witness_sig && !sigdata.witness) return false; input.FromSignatureData(sigdata); + // If we have a witness signature, use the smaller witness UTXO. if (sigdata.witness) { - assert(!utxo.IsNull()); input.witness_utxo = utxo; - } - - // If both UTXO types are present, drop the unnecessary one. - if (input.non_witness_utxo && !input.witness_utxo.IsNull()) { - if (sigdata.witness) { - input.non_witness_utxo = nullptr; - } else { - input.witness_utxo.SetNull(); - } + input.non_witness_utxo = nullptr; } return sig_complete; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 908d403cc..c2aa8b1e0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3744,15 +3744,25 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sig const CTxIn& txin = psbtx.tx->vin[i]; PSBTInput& input = psbtx.inputs.at(i); - // If we don't know about this input, skip it and let someone else deal with it - const uint256& txhash = txin.prevout.hash; - const auto it = pwallet->mapWallet.find(txhash); - if (it != pwallet->mapWallet.end()) { - const CWalletTx& wtx = it->second; - CTxOut utxo = wtx.tx->vout[txin.prevout.n]; - // Update both UTXOs from the wallet. - input.non_witness_utxo = wtx.tx; - input.witness_utxo = utxo; + if (PSBTInputSigned(input)) { + continue; + } + + // Verify input looks sane. This will check that we have at most one uxto, witness or non-witness. + if (!input.IsSane()) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "PSBT input is not sane."); + } + + // If we have no utxo, grab it from the wallet. + if (!input.non_witness_utxo && input.witness_utxo.IsNull()) { + const uint256& txhash = txin.prevout.hash; + const auto it = pwallet->mapWallet.find(txhash); + if (it != pwallet->mapWallet.end()) { + const CWalletTx& wtx = it->second; + // We only need the non_witness_utxo, which is a superset of the witness_utxo. + // The signing code will switch to the smaller witness_utxo if this is ok. + input.non_witness_utxo = wtx.tx; + } } // Get the Sighash type From e13fea975d5e4ae961faba36379a1cdaf9e50c1c Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Tue, 30 Oct 2018 00:41:19 -0700 Subject: [PATCH 7/7] Add regression test for PSBT signing bug #14473 --- test/functional/rpc_psbt.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index fca910bf6..86b043176 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -207,6 +207,13 @@ class PSBTTest(BitcoinTestFramework): assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE assert_equal(decoded_psbt["tx"]["locktime"], 0) + # Regression test for 14473 (mishandling of already-signed witness transaction): + psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}]) + complete_psbt = self.nodes[0].walletprocesspsbt(psbtx_info["psbt"]) + double_processed_psbt = self.nodes[0].walletprocesspsbt(complete_psbt["psbt"]) + assert_equal(complete_psbt, double_processed_psbt) + # We don't care about the decode result, but decoding must succeed. + self.nodes[0].decodepsbt(double_processed_psbt["psbt"]) # BIP 174 Test Vectors