diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1726,13 +1726,15 @@ 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 (size_t i = 0; i < psbtx.tx->vin.size(); ++i) { - PSBTInput &input = psbtx.inputs.at(i); - - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, - SigHashType()); + complete &= + SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, SigHashType()); } UniValue result(UniValue::VOBJ); @@ -1745,11 +1747,10 @@ mtx.vin[i].scriptSig = psbtx.inputs[i].final_script_sig; } ssTx << mtx; - result.pushKV("hex", HexStr(ssTx.begin(), ssTx.end())); + result.pushKV("hex", HexStr(ssTx.str())); } else { ssTx << psbtx; - result.pushKV("psbt", - EncodeBase64((uint8_t *)ssTx.data(), ssTx.size())); + result.pushKV("psbt", EncodeBase64(ssTx.str())); } result.pushKV("complete", complete); diff --git a/src/script/sign.h b/src/script/sign.h --- a/src/script/sign.h +++ b/src/script/sign.h @@ -522,6 +522,7 @@ 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 &txIn); // Only checks if they refer to the same transaction friend bool operator==(const PartiallySignedTransaction &a, @@ -685,12 +686,15 @@ CMutableTransaction &txTo, unsigned int nIn, SigHashType sigHashType); +/** 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, + PartiallySignedTransaction &psbt, int index, SigHashType sighash = SigHashType()); /** Extract signature data from a transaction input, and insert it. */ diff --git a/src/script/sign.cpp b/src/script/sign.cpp --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -220,11 +220,17 @@ return sigdata.complete; } +bool PSBTInputSigned(PSBTInput &input) { + return !input.final_script_sig.empty(); +} + bool SignPSBTInput(const SigningProvider &provider, - const CMutableTransaction &tx, PSBTInput &input, int index, + PartiallySignedTransaction &psbt, int index, SigHashType sighash) { - // If this input has a final scriptsig, don't do anything with it. - if (!input.final_script_sig.empty()) { + PSBTInput &input = psbt.inputs.at(index); + const CMutableTransaction &tx = *psbt.tx; + + if (PSBTInputSigned(input)) { return true; } @@ -234,6 +240,12 @@ // Get UTXO CTxOut utxo; + + // Verify input sanity + if (!input.IsSane()) { + return false; + } + if (input.utxo.IsNull()) { return false; } @@ -451,6 +463,12 @@ DummySignatureCreator(33, 32); const SigningProvider &DUMMY_SIGNING_PROVIDER = SigningProvider(); +PartiallySignedTransaction::PartiallySignedTransaction(const CTransaction &txIn) + : tx(txIn) { + inputs.resize(txIn.vin.size()); + outputs.resize(txIn.vout.size()); +} + bool PartiallySignedTransaction::IsNull() const { return !tx && inputs.empty() && outputs.empty() && unknown.empty(); } diff --git a/src/wallet/rpcwallet.h b/src/wallet/rpcwallet.h --- a/src/wallet/rpcwallet.h +++ b/src/wallet/rpcwallet.h @@ -36,7 +36,6 @@ const JSONRPCRequest &request); UniValue getaddressinfo(const Config &config, const JSONRPCRequest &request); bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx, - const CTransaction *txConst, SigHashType sighash_type = SigHashType(), bool sign = true, bool bip32derivs = false); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4580,24 +4580,34 @@ } bool FillPSBT(const CWallet *pwallet, PartiallySignedTransaction &psbtx, - const CTransaction *txConst, SigHashType sighash_type, bool sign, - bool bip32derivs) { + SigHashType sighash_type, bool sign, bool bip32derivs) { LOCK(pwallet->cs_wallet); // Get all of the previous transactions bool complete = true; - for (size_t i = 0; i < txConst->vin.size(); ++i) { - const CTxIn &txin = txConst->vin[i]; + for (size_t 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 - const TxId &txid = txin.prevout.GetTxId(); - const auto it = pwallet->mapWallet.find(txid); - if (it != pwallet->mapWallet.end()) { - const CWalletTx &wtx = it->second; - CTxOut utxo = wtx.tx->vout[txin.prevout.GetN()]; - // Update UTXOs from the wallet. - input.utxo = utxo; + if (PSBTInputSigned(input)) { + continue; + } + + // Verify input looks sane. + 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.utxo.IsNull()) { + const TxId &txid = txin.prevout.GetTxId(); + const auto it = pwallet->mapWallet.find(txid); + if (it != pwallet->mapWallet.end()) { + const CWalletTx &wtx = it->second; + CTxOut utxo = wtx.tx->vout[txin.prevout.GetN()]; + // Update UTXOs from the wallet. + input.utxo = utxo; + } } // Get the Sighash type @@ -4610,13 +4620,13 @@ complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), - *psbtx.tx, input, i, sighash_type); + psbtx, i, sighash_type); } // Fill in the bip32 keypaths and redeemscripts for the outputs so that // hardware wallets can identify change - for (size_t i = 0; i < txConst->vout.size(); ++i) { - const CTxOut &out = txConst->vout.at(i); + for (size_t 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 @@ -4706,22 +4716,17 @@ "Signature must use SIGHASH_FORKID"); } - // 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); ssTx << psbtx; - result.pushKV("psbt", EncodeBase64((uint8_t *)ssTx.data(), ssTx.size())); + result.pushKV("psbt", EncodeBase64(ssTx.str())); result.pushKV("complete", complete); return result; @@ -4891,31 +4896,20 @@ FundTransaction(pwallet, rawTx, fee, change_position, request.params[3]); // Make a blank psbt - PartiallySignedTransaction psbtx; - psbtx.tx = rawTx; - for (size_t i = 0; i < rawTx.vin.size(); ++i) { - psbtx.inputs.push_back(PSBTInput()); - } - for (size_t i = 0; i < rawTx.vout.size(); ++i) { - psbtx.outputs.push_back(PSBTOutput()); - } - - // Use CTransaction for the constant parts of the - // transaction to avoid rehashing. - const CTransaction txConst(*psbtx.tx); + const CTransaction tx = CTransaction(rawTx); + PartiallySignedTransaction 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, SigHashType().withForkId(), false, - bip32derivs); + FillPSBT(pwallet, psbtx, SigHashType().withForkId(), false, bip32derivs); // Serialize the PSBT CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << psbtx; UniValue result(UniValue::VOBJ); - result.pushKV("psbt", EncodeBase64((uint8_t *)ssTx.data(), ssTx.size())); + result.pushKV("psbt", EncodeBase64(ssTx.str())); result.pushKV("fee", ValueFromAmount(fee)); result.pushKV("changepos", change_position); return result; diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -84,15 +84,11 @@ SER_NETWORK, PROTOCOL_VERSION); ssData >> psbtx; - // Use CTransaction for the constant parts of the - // transaction to avoid rehashing. - const CTransaction txConst(*psbtx.tx); - // FIXME: input 2 hd path is missing. // The path missing comes from the HD masterkey. // Fill transaction with our data - FillPSBT(&m_wallet, psbtx, &txConst, SigHashType(), false, true); + FillPSBT(&m_wallet, psbtx, SigHashType(), false, true); // Get the final tx CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -139,6 +139,19 @@ self.nodes[0].generate(6) self.sync_all() + unspent = self.nodes[0].listunspent()[0] + + # Regression test for 14473 (mishandling of already-signed + # 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 # Check that unknown values are just passed through