From cfdd6b2f6c01c5c8dbe4691cc84c65b9705784c3 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:18:52 -0700 Subject: 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. Github-Pull: #14588 Rebased-From: fe5d22bc676f158e8d567d71edb3451118759d62 --- src/wallet/rpcwallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/wallet/rpcwallet.cpp') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 0a9242327b..c5cc277c4a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4649,7 +4649,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; @@ -4763,7 +4763,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; -- cgit v1.2.3 From a9eab081d5a31cec3138685fef21e428f833db03 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:24:55 -0700 Subject: Remove redundant txConst parameter to FillPSBT Github-Pull: #14588 Rebased-From: 4f3f5cb4b142f0fcb36241fa33b52a257901dbee --- src/wallet/rpcwallet.cpp | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) (limited to 'src/wallet/rpcwallet.cpp') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c5cc277c4a..24df23edd1 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4502,13 +4502,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 @@ -4559,8 +4559,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); // Dummy tx so we can use ProduceSignature to get stuff out @@ -4637,14 +4637,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); @@ -4750,13 +4746,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); -- cgit v1.2.3 From 70ee1f8709a54a9aeac004d8589faa08f665587a Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:26:16 -0700 Subject: New PartiallySignedTransaction constructor from CTransction New constructor that creates a PartiallySignedTransaction from a CTransaction, automatically sizing the inputs and outputs vectors for convenience. Github-Pull: #14588 Rebased-From: 65166d4cf828909dc4bc49dd68a58103d015f1fd --- src/wallet/rpcwallet.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) (limited to 'src/wallet/rpcwallet.cpp') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 24df23edd1..ec103c6763 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4737,14 +4737,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(); -- cgit v1.2.3 From ad94165db91c0416634459e18f173c5cd063dc55 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:30:50 -0700 Subject: 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. Github-Pull: #14588 Rebased-From: 0f5bda2bd941686620ef0eb90bd7ed973cc7ef73 --- src/wallet/rpcwallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/wallet/rpcwallet.cpp') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ec103c6763..926902708a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4529,9 +4529,9 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sig SignatureData sigdata; if (sign) { - complete &= SignPSBTInput(*pwallet, *psbtx.tx, input, sigdata, i, sighash_type); + complete &= SignPSBTInput(*pwallet, psbtx, sigdata, i, sighash_type); } else { - complete &= SignPSBTInput(PublicOnlySigningProvider(pwallet), *psbtx.tx, input, sigdata, i, sighash_type); + complete &= SignPSBTInput(PublicOnlySigningProvider(pwallet), psbtx, sigdata, i, sighash_type); } if (sigdata.witness) { -- cgit v1.2.3 From db445d4e5a25ff13de014dbbf43bb99cde8b8769 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:31:41 -0700 Subject: 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 Github-Pull: #14588 --- src/wallet/rpcwallet.cpp | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) (limited to 'src/wallet/rpcwallet.cpp') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 926902708a..6400b4470f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4511,15 +4511,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 @@ -4541,15 +4551,6 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sig } } - // 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(); - } - } - // Get public key paths if (bip32derivs) { for (const auto& pubkey_it : sigdata.misc_pubkeys) { -- cgit v1.2.3