From 043e5e0c9e4fca50e0c15c66a2a7d51565d01f4b Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Fri, 3 Jul 2020 08:53:53 +1200 Subject: [PATCH] Merge #19215: psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow) 46004790588c24174a0bec49b540d158ce163ffd psbt: always put a non_witness_utxo and don't remove it (Andrew Chow) 5279d8bc07d601fe6a67ad665fbc7591fe73c7de psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow) 72f6bec1da198764d4648a10a61c485e7ab65e9e rpc: show both UTXOs in decodepsbt (Andrew Chow) Pull request description: Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition. Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper. Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs. As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs. ACKs for top commit: Sjors: utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators) ryanofsky: Code review re-ACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to meshcollider: utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940 --- src/psbt.cpp | 22 ---------------------- src/psbt.h | 7 ------- src/rpc/rawtransaction.cpp | 5 ++++- src/test/fuzz/psbt.cpp | 2 -- src/wallet/scriptpubkeyman.cpp | 5 ----- src/wallet/wallet.cpp | 5 ----- test/functional/rpc_psbt.py | 3 +++ 7 files changed, 7 insertions(+), 42 deletions(-) diff --git a/src/psbt.cpp b/src/psbt.cpp index 3d38958bc70150..be52ecc502142b 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -42,14 +42,6 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt) return true; } -bool PartiallySignedTransaction::IsSane() const -{ - for (PSBTInput input : inputs) { - if (!input.IsSane()) return false; - } - return true; -} - bool PartiallySignedTransaction::AddInput(const CTxIn& txin, PSBTInput& psbtin) { if (std::find(tx->vin.begin(), tx->vin.end(), txin) != tx->vin.end()) { @@ -142,11 +134,6 @@ void PSBTInput::Merge(const PSBTInput& input) if (final_script_sig.empty() && !input.final_script_sig.empty()) final_script_sig = input.final_script_sig; } -bool PSBTInput::IsSane() const -{ - return true; -} - void PSBTOutput::FillSignatureData(SignatureData& sigdata) const { if (!redeem_script.empty()) { @@ -231,11 +218,6 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& // Get UTXO 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. COutPoint prevout = tx.vin[index].prevout; @@ -308,10 +290,6 @@ TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector return TransactionError::PSBT_MISMATCH; } } - if (!out.IsSane()) { - return TransactionError::INVALID_PSBT; - } - return TransactionError::OK; } diff --git a/src/psbt.h b/src/psbt.h index f6a6e38b96e3e4..90c936c1b35089 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -56,7 +56,6 @@ struct PSBTInput void FillSignatureData(SignatureData& sigdata) const; void FromSignatureData(const SignatureData& sigdata); void Merge(const PSBTInput& input); - bool IsSane() const; PSBTInput() {} template @@ -229,7 +228,6 @@ struct PSBTOutput void FillSignatureData(SignatureData& sigdata) const; void FromSignatureData(const SignatureData& sigdata); void Merge(const PSBTOutput& output); - bool IsSane() const; PSBTOutput() {} template @@ -330,7 +328,6 @@ struct PartiallySignedTransaction /** Merge psbt into this. The two psbts must have the same underlying CTransaction (i.e. the * same actual Bitcoin transaction.) Returns true if the merge succeeded, false otherwise. */ [[nodiscard]] bool Merge(const PartiallySignedTransaction& psbt); - bool IsSane() const; bool AddInput(const CTxIn& txin, PSBTInput& psbtin); bool AddOutput(const CTxOut& txout, const PSBTOutput& psbtout); PartiallySignedTransaction() {} @@ -478,10 +475,6 @@ struct PartiallySignedTransaction if (outputs.size() != tx->vout.size()) { throw std::ios_base::failure("Outputs provided does not match the number of outputs in transaction."); } - // Sanity check - if (!IsSane()) { - throw std::ios_base::failure("PSBT is not sane."); - } } template diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 5ef509977f2ad7..4ea6bc9a345764 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1264,6 +1264,7 @@ UniValue decodepsbt(const JSONRPCRequest& request) const PSBTInput& input = psbtx.inputs[i]; UniValue in(UniValue::VOBJ); // UTXOs + bool have_a_utxo = false; if (input.non_witness_utxo) { UniValue non_wit(UniValue::VOBJ); TxToUniv(*input.non_witness_utxo, uint256(), non_wit, false); @@ -1275,7 +1276,9 @@ UniValue decodepsbt(const JSONRPCRequest& request) // Hack to just not show fee later have_all_utxos = false; } - } else { + have_a_utxo = true; + } + if (!have_a_utxo) { have_all_utxos = false; } diff --git a/src/test/fuzz/psbt.cpp b/src/test/fuzz/psbt.cpp index ce0c27e26964dd..b29f4e2228db57 100644 --- a/src/test/fuzz/psbt.cpp +++ b/src/test/fuzz/psbt.cpp @@ -33,7 +33,6 @@ FUZZ_TARGET(psbt) } (void)psbt.IsNull(); - (void)psbt.IsSane(); std::optional tx = psbt.tx; if (tx) { @@ -44,7 +43,6 @@ FUZZ_TARGET(psbt) for (const PSBTInput& input : psbt.inputs) { (void)PSBTInputSigned(input); (void)input.IsNull(); - (void)input.IsSane(); } for (const PSBTOutput& output : psbt.outputs) { diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index c3bb3cfcb848be..21f6c075d6c3da 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -713,11 +713,6 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb continue; } - // Verify input looks sane. This will check that we have at most one uxto. - if (!input.IsSane()) { - return TransactionError::INVALID_PSBT; - } - // Get the Sighash type if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) { return TransactionError::SIGHASH_MISMATCH; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cd11aac72f3eda..fc72ea81073a3e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3005,11 +3005,6 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp continue; } - // Verify input looks sane. This will check that we have at most one uxto, witness or non-witness. - if (!input.IsSane()) { - return TransactionError::INVALID_PSBT; - } - // If we have no utxo, grab it from the wallet. if (!input.non_witness_utxo) { const uint256& txhash = txin.prevout.hash; diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 7fb07140a71a2c..bc3a4cf628fae3 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -68,6 +68,9 @@ def run_test(self): # spend single key from node 1 rawtx = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99})['psbt'] walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(rawtx) + # Make sure it has both types of UTXOs + decoded = self.nodes[1].decodepsbt(walletprocesspsbt_out['psbt']) + assert 'non_witness_utxo' in decoded['inputs'][0] assert_equal(walletprocesspsbt_out['complete'], True) self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex'])