From a3fe125490978a684528c40fc9a15ccdfdf2c92a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 2 Oct 2018 20:33:31 -0400 Subject: [PATCH 1/9] check that a separator is found for psbt inputs, outputs, and global map Github-Pull: #14377 Rebased-From: 4fb3388db95f408566e43ebb9736842cfbff0a7d --- src/script/sign.h | 30 +++++++++++++++++++++++++++--- test/functional/data/rpc_psbt.json | 3 ++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/script/sign.h b/src/script/sign.h index 245b15410..2b45df85a 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -286,6 +286,7 @@ struct PSBTInput template inline void Unserialize(Stream& s) { // Read loop + bool found_sep = false; while(!s.empty()) { // Read std::vector key; @@ -293,7 +294,10 @@ struct PSBTInput // the key is empty if that was actually a separator byte // This is a special case for key lengths 0 as those are not allowed (except for separator) - if (key.empty()) return; + if (key.empty()) { + found_sep = true; + break; + } // First byte of key is the type unsigned char type = key[0]; @@ -408,6 +412,10 @@ struct PSBTInput break; } } + + if (!found_sep) { + throw std::ios_base::failure("Separator is missing at the end of an input map"); + } } template @@ -461,6 +469,7 @@ struct PSBTOutput template inline void Unserialize(Stream& s) { // Read loop + bool found_sep = false; while(!s.empty()) { // Read std::vector key; @@ -468,7 +477,10 @@ struct PSBTOutput // the key is empty if that was actually a separator byte // This is a special case for key lengths 0 as those are not allowed (except for separator) - if (key.empty()) return; + if (key.empty()) { + found_sep = true; + break; + } // First byte of key is the type unsigned char type = key[0]; @@ -513,6 +525,10 @@ struct PSBTOutput } } } + + if (!found_sep) { + throw std::ios_base::failure("Separator is missing at the end of an output map"); + } } template @@ -588,6 +604,7 @@ struct PartiallySignedTransaction } // Read global data + bool found_sep = false; while(!s.empty()) { // Read std::vector key; @@ -595,7 +612,10 @@ struct PartiallySignedTransaction // the key is empty if that was actually a separator byte // This is a special case for key lengths 0 as those are not allowed (except for separator) - if (key.empty()) break; + if (key.empty()) { + found_sep = true; + break; + } // First byte of key is the type unsigned char type = key[0]; @@ -635,6 +655,10 @@ struct PartiallySignedTransaction } } + if (!found_sep) { + throw std::ios_base::failure("Separator is missing at the end of the global map"); + } + // Make sure that we got an unsigned tx if (!tx) { throw std::ios_base::failure("No unsigned transcation was provided"); diff --git a/test/functional/data/rpc_psbt.json b/test/functional/data/rpc_psbt.json index 9f970b496..bff5f23e2 100644 --- a/test/functional/data/rpc_psbt.json +++ b/test/functional/data/rpc_psbt.json @@ -17,7 +17,8 @@ "cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWABTYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFgAUAK6pouXw+HaliN9VRuh0LR2HAI8AAAAAAAEAuwIAAAABqtc5MQGL0l+ErkALaISL4J23BurCrBgpi6vucatlb4sAAAAASEcwRAIgWPb8fGoz4bMVSNSByCbAFb0wE1qtQs1neQ2rZtKtJDsCIEoc7SYExnNbY5PltBaR3XiwDwxZQvufdRhW+qk4FX26Af7///8CgPD6AgAAAAAXqRQPuUY0IWlrgsgzryQceMF9295JNIfQ8gonAQAAABepFCnKdPigj4GZlCgYXJe12FLkBj9hh2UAAAABB9oARzBEAiB0AYrUGACXuHMyPAAVcgs2hMyBI4kQSOfbzZtVrWecmQIgc9Npt0Dj61Pc76M4I8gHBRTKVafdlUTxV8FnkTJhEYwBSDBFAiEA9hA4swjcHahlo0hSdG8BV3KTQgjG0kRUOTzZm98iF3cCIAVuZ1pnWm0KArhbFOXikHTYolqbV2C+ooFvZhkQoAbqAUdSIQKVg785rgpgl0etGZrd1jT6YQhVnWxc05tMIYPxq5bgfyEC2rYf9JoU22p9ArDNH7t4/EsYMStbTlTa5Nui+/71NtdSrgABASAAwusLAAAAABepFLf1+vQOPUClpFmx2zU18rcvqSHohwEHIyIAIIwjUxc3Q7WV37Sge3K6jkLjeX2nTof+fZ10l+OyAokDAQjaBABHMEQCIGLrelVhB6fHP0WsSrWh3d9vcHX7EnWWmn84Pv/3hLyyAiAMBdu3Rw2/LwhVfdNWxzJcHtMJE+mWzThAlF2xIijaXwFHMEQCIGX0W6WZi1mif/4ae+0BavHx+Q1Us6qPdFCqX1aiUQO9AiB/ckcDrR7blmgLKEtW1P/LiPf7dZ6rvgiqMPKbhROD0gFHUiEDCJ3BDHrG21T5EymvYXMz2ziM6tDCMfcjN50bmQMLAtwhAjrdkE89bc9Z3bkGsN7iNSm3/7ntUOXoYVGSaGAiHw5zUq4AIQIDqaTDf1mW06ol26xrVwrwZQOUSSlCRgs1R1PtnuylhxDZDGpPAAAAgAAAAIAEAACAACICAn9jmXV9Lv9VoTatAsaEsYOLZVbl8bazQoKpS2tQBRCWENkMak8AAACAAAAAgAUAAIAA", "cHNidP8BAHMCAAAAATAa6YblFqHsisW0vGVz0y+DtGXiOtdhZ9aLOOcwtNvbAAAAAAD/////AnR7AQAAAAAAF6kUA6oXrogrXQ1Usl1jEE5P/s57nqKHYEOZOwAAAAAXqRS5IbG6b3IuS/qDtlV6MTmYakLsg4cAAAAAAAEBHwDKmjsAAAAAFgAU0tlLZK4IWH7vyO6xh8YB6Tn5A3wCAwABAAAAAAEAFgAUYunpgv/zTdgjlhAxawkM0qO3R8sAAQAiACCHa62DLx0WgBXtQSMqnqZaGBXZ7xPA74dZ9ktbKyeKZQEBJVEhA7fOI6AcW0vwCmQlN836uzFbZoMyhnR471EwnSvVf4qHUa4A", "cHNidP8BAHMCAAAAATAa6YblFqHsisW0vGVz0y+DtGXiOtdhZ9aLOOcwtNvbAAAAAAD/////AnR7AQAAAAAAF6kUA6oXrogrXQ1Usl1jEE5P/s57nqKHYEOZOwAAAAAXqRS5IbG6b3IuS/qDtlV6MTmYakLsg4cAAAAAAAEBHwDKmjsAAAAAFgAU0tlLZK4IWH7vyO6xh8YB6Tn5A3wAAgAAFgAUYunpgv/zTdgjlhAxawkM0qO3R8sAAQAiACCHa62DLx0WgBXtQSMqnqZaGBXZ7xPA74dZ9ktbKyeKZQEBJVEhA7fOI6AcW0vwCmQlN836uzFbZoMyhnR471EwnSvVf4qHUa4A", - "cHNidP8BAHMCAAAAATAa6YblFqHsisW0vGVz0y+DtGXiOtdhZ9aLOOcwtNvbAAAAAAD/////AnR7AQAAAAAAF6kUA6oXrogrXQ1Usl1jEE5P/s57nqKHYEOZOwAAAAAXqRS5IbG6b3IuS/qDtlV6MTmYakLsg4cAAAAAAAEBHwDKmjsAAAAAFgAU0tlLZK4IWH7vyO6xh8YB6Tn5A3wAAQAWABRi6emC//NN2COWEDFrCQzSo7dHywABACIAIIdrrYMvHRaAFe1BIyqeploYFdnvE8Dvh1n2S1srJ4plIQEAJVEhA7fOI6AcW0vwCmQlN836uzFbZoMyhnR471EwnSvVf4qHUa4A" + "cHNidP8BAHMCAAAAATAa6YblFqHsisW0vGVz0y+DtGXiOtdhZ9aLOOcwtNvbAAAAAAD/////AnR7AQAAAAAAF6kUA6oXrogrXQ1Usl1jEE5P/s57nqKHYEOZOwAAAAAXqRS5IbG6b3IuS/qDtlV6MTmYakLsg4cAAAAAAAEBHwDKmjsAAAAAFgAU0tlLZK4IWH7vyO6xh8YB6Tn5A3wAAQAWABRi6emC//NN2COWEDFrCQzSo7dHywABACIAIIdrrYMvHRaAFe1BIyqeploYFdnvE8Dvh1n2S1srJ4plIQEAJVEhA7fOI6AcW0vwCmQlN836uzFbZoMyhnR471EwnSvVf4qHUa4A", + "cHNidP8BAHMCAAAAAbiWoY6pOQepFsEGhUPXaulX9rvye2NH+NrdlAHg+WgpAQAAAAD/////AkBLTAAAAAAAF6kUqWwXCcLM5BN2zoNqMNT5qMlIi7+HQEtMAAAAAAAXqRSVF/in2XNxAlN1OSxkyp0z+Wtg2YcAAAAAAAEBIBNssgAAAAAAF6kUamsvautR8hRlMRY6OKNTx03DK96HAQcXFgAUo8u1LWpHprjt/uENAwBpGZD0UH0BCGsCRzBEAiAONfH3DYiw67ZbylrsxCF/XXpVwyWBRgofyRbPslzvwgIgIKCsWw5sHSIPh1icNvcVLZLHWj6NA7Dk+4Os2pOnMbQBIQPGStfYHPtyhpV7zIWtn0Q4GXv5gK1zy/tnJ+cBXu4iiwABABYAFMwmJQEz+HDpBEEabxJ5PogPsqZRAAEAFgAUyCrGc3h3FYCmiIspbv2pSTKZ5jU" ], "valid" : [ "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAAAQD9pQEBAAAAAAECiaPHHqtNIOA3G7ukzGmPopXJRjr6Ljl/hTPMti+VZ+UBAAAAFxYAFL4Y0VKpsBIDna89p95PUzSe7LmF/////4b4qkOnHf8USIk6UwpyN+9rRgi7st0tAXHmOuxqSJC0AQAAABcWABT+Pp7xp0XpdNkCxDVZQ6vLNL1TU/////8CAMLrCwAAAAAZdqkUhc/xCX/Z4Ai7NK9wnGIZeziXikiIrHL++E4sAAAAF6kUM5cluiHv1irHU6m80GfWx6ajnQWHAkcwRAIgJxK+IuAnDzlPVoMR3HyppolwuAJf3TskAinwf4pfOiQCIAGLONfc0xTnNMkna9b7QPZzMlvEuqFEyADS8vAtsnZcASED0uFWdJQbrUqZY3LLh+GFbTZSYG2YVi/jnF6efkE/IQUCSDBFAiEA0SuFLYXc2WHS9fSrZgZU327tzHlMDDPOXMMJ/7X85Y0CIGczio4OFyXBl/saiK9Z9R5E5CVbIBZ8hoQDHAXR8lkqASECI7cr7vCWXRC+B3jv7NYfysb3mk6haTkzgHNEZPhPKrMAAAAAAAAA", From cfdd6b2f6c01c5c8dbe4691cc84c65b9705784c3 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:18:52 -0700 Subject: [PATCH 2/9] 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/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 314184ab0..2fdbe3814 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1662,10 +1662,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 0a9242327..c5cc277c4 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; From a9eab081d5a31cec3138685fef21e428f833db03 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:24:55 -0700 Subject: [PATCH 3/9] Remove redundant txConst parameter to FillPSBT Github-Pull: #14588 Rebased-From: 4f3f5cb4b142f0fcb36241fa33b52a257901dbee --- 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 c5cc277c4..24df23edd 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); 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 61c3fa94a..94b5f2f37 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 70ee1f8709a54a9aeac004d8589faa08f665587a Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:26:16 -0700 Subject: [PATCH 4/9] 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/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 1ab5051ff..190c0bf68 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -491,6 +491,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 2b45df85a..79cd6dc53 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -550,6 +550,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 24df23edd..ec103c676 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(); From 39ece4fc2807b3442759d879985146ac0de9742d Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:28:48 -0700 Subject: [PATCH 5/9] 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. GitHub-Pull: #14588 Rebased-From: 53e6fffb8f5b10f94708d33d667a67cb91c2d09d --- 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 2fdbe3814..5df39f9ba 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1643,13 +1643,16 @@ 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); SignatureData sigdata; - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, sigdata, i, 1); + complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, sigdata, i, SIGHASH_ALL); } UniValue result(UniValue::VOBJ); diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 190c0bf68..945e16235 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -233,10 +233,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, SignatureData& sigdata, 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 79cd6dc53..ebc742c09 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -714,6 +714,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, SignatureData& sigdata, int index, int sighash = 1); From ad94165db91c0416634459e18f173c5cd063dc55 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:30:50 -0700 Subject: [PATCH 6/9] 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/rpc/rawtransaction.cpp | 4 +--- src/script/sign.cpp | 5 ++++- src/script/sign.h | 2 +- src/wallet/rpcwallet.cpp | 4 ++-- src/wallet/rpcwallet.h | 2 +- src/wallet/test/psbt_wallet_tests.cpp | 2 +- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 5df39f9ba..7de3638f4 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1649,10 +1649,8 @@ 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); - SignatureData sigdata; - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, sigdata, i, SIGHASH_ALL); + complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, sigdata, i, SIGHASH_ALL); } UniValue result(UniValue::VOBJ); diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 945e16235..8bc142f6e 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -238,8 +238,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, SignatureData& sigdata, int index, int sighash) +bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, SignatureData& sigdata, 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 ebc742c09..f24d77cb3 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -718,7 +718,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, SignatureData& sigdata, int index, int sighash = 1); +bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, SignatureData& sigdata, 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 ec103c676..926902708 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) { 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 94b5f2f37..8d46718a7 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 db445d4e5a25ff13de014dbbf43bb99cde8b8769 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:31:41 -0700 Subject: [PATCH 7/9] 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/script/sign.cpp | 25 ++++++++++++++++++------- src/wallet/rpcwallet.cpp | 37 +++++++++++++++++++------------------ 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 8bc142f6e..f97d6a253 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -253,15 +253,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 @@ -279,6 +283,13 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& // Verify that a witness signature was produced in case one was required. 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) { + input.witness_utxo = utxo; + input.non_witness_utxo = nullptr; + } + return sig_complete; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 926902708..6400b4470 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) { From ff56bb9b44387f7e20ae7de59087cf19ea216726 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Tue, 30 Oct 2018 00:41:19 -0700 Subject: [PATCH 8/9] Add regression test for PSBT signing bug #14473 Github-Pull: #14588 Rebased-From: e13fea975d5e4ae961faba36379a1cdaf9e50c1c --- 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 7c8c6c9af..92132addb 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -168,6 +168,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"]) # Make sure change address wallet does not have P2SH innerscript access to results in success # when attempting BnB coin selection From 7bee41452bee4d10ab075999580853bdc3431e8b Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 20 Sep 2018 11:43:06 -0700 Subject: [PATCH 9/9] Add test for conversion from non-witness to witness UTXO Github-Pull: #14197 Rebased-From: 862d159d635c1de219d94e030b186a745fe28eb9 --- test/functional/rpc_psbt.py | 47 ++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 92132addb..a31719088 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -6,7 +6,7 @@ """ from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, assert_raises_rpc_error, find_output +from test_framework.util import assert_equal, assert_raises_rpc_error, find_output, disconnect_nodes, connect_nodes_bi, sync_blocks import json import os @@ -23,6 +23,45 @@ class PSBTTest(BitcoinTestFramework): def skip_test_if_missing_module(self): self.skip_if_no_wallet() + def test_utxo_conversion(self): + mining_node = self.nodes[2] + offline_node = self.nodes[0] + online_node = self.nodes[1] + + # Disconnect offline node from others + disconnect_nodes(offline_node, 1) + disconnect_nodes(online_node, 0) + disconnect_nodes(offline_node, 2) + disconnect_nodes(mining_node, 0) + + # Mine a transaction that credits the offline address + offline_addr = offline_node.getnewaddress(address_type="p2sh-segwit") + online_addr = online_node.getnewaddress(address_type="p2sh-segwit") + online_node.importaddress(offline_addr, "", False) + mining_node.sendtoaddress(address=offline_addr, amount=1.0) + mining_node.generate(nblocks=1) + sync_blocks([mining_node, online_node]) + + # Construct an unsigned PSBT on the online node (who doesn't know the output is Segwit, so will include a non-witness UTXO) + utxos = online_node.listunspent(addresses=[offline_addr]) + raw = online_node.createrawtransaction([{"txid":utxos[0]["txid"], "vout":utxos[0]["vout"]}],[{online_addr:0.9999}]) + psbt = online_node.walletprocesspsbt(online_node.converttopsbt(raw))["psbt"] + assert("non_witness_utxo" in mining_node.decodepsbt(psbt)["inputs"][0]) + + # Have the offline node sign the PSBT (which will update the UTXO to segwit) + signed_psbt = offline_node.walletprocesspsbt(psbt)["psbt"] + assert("witness_utxo" in mining_node.decodepsbt(signed_psbt)["inputs"][0]) + + # Make sure we can mine the resulting transaction + txid = mining_node.sendrawtransaction(mining_node.finalizepsbt(signed_psbt)["hex"]) + mining_node.generate(1) + sync_blocks([mining_node, online_node]) + assert_equal(online_node.gettxout(txid,0)["confirmations"], 1) + + # Reconnect + connect_nodes_bi(self.nodes, 0, 1) + connect_nodes_bi(self.nodes, 0, 2) + def run_test(self): # Create and fund a raw tx for sending 10 BTC psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt'] @@ -235,6 +274,12 @@ class PSBTTest(BitcoinTestFramework): extracted = self.nodes[2].finalizepsbt(extractor['extract'], True)['hex'] assert_equal(extracted, extractor['result']) + # Unload extra wallets + for i, signer in enumerate(signers): + self.nodes[2].unloadwallet("wallet{}".format(i)) + + self.test_utxo_conversion() + if __name__ == '__main__': PSBTTest().main()