From 65166d4cf828909dc4bc49dd68a58103d015f1fd 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. --- src/script/sign.cpp | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src/script/sign.cpp') 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(); -- cgit v1.2.3 From 53e6fffb8f5b10f94708d33d667a67cb91c2d09d Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Fri, 26 Oct 2018 15:28:48 -0700 Subject: 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/script/sign.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'src/script/sign.cpp') 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; } -- cgit v1.2.3 From 0f5bda2bd941686620ef0eb90bd7ed973cc7ef73 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. --- src/script/sign.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/script/sign.cpp') 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; } -- cgit v1.2.3 From 565500508aa5df0011109ebf375ba71b693fc7de 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 --- src/script/sign.cpp | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) (limited to 'src/script/sign.cpp') 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; -- cgit v1.2.3