LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Refactor PSBTInput signing to enforce invariant

Browse source 
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
This commit is contained in:
Glenn Willen 2018-10-26 15:31:41 -07:00
parent 0f5bda2bd9
commit 565500508a
2 changed files with 32 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

30
src/script/sign.cpp
View file

@ -260,15 +260,19 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
// Get UTXO // Get UTXO
bool require_witness_sig = false; bool require_witness_sig = false;
CTxOut 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 (input.non_witness_utxo) {
// If we're taking our information from a non-witness UTXO, verify that it matches the prevout. // 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; COutPoint prevout = tx.vin[index].prevout;
// If both witness and non-witness UTXO are provided, verify that they match. This check shouldn't if (input.non_witness_utxo->GetHash() != prevout.hash) {
// matter, as the PSBT deserializer enforces only one of both is provided, and the only way both return false;
// 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. utxo = input.non_witness_utxo->vout[prevout.n];
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];
} else if (!input.witness_utxo.IsNull()) { } else if (!input.witness_utxo.IsNull()) {
utxo = input.witness_utxo; utxo = input.witness_utxo;
// When we're taking our information from a witness UTXO, we can't verify it is actually data from // 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; if (require_witness_sig && !sigdata.witness) return false;
input.FromSignatureData(sigdata); input.FromSignatureData(sigdata);
// If we have a witness signature, use the smaller witness UTXO.
if (sigdata.witness) { if (sigdata.witness) {
assert(!utxo.IsNull());
input.witness_utxo = utxo; input.witness_utxo = utxo;
} input.non_witness_utxo = nullptr;
// 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();
}
} }
return sig_complete; return sig_complete;

28
src/wallet/rpcwallet.cpp
View file

@ -3744,15 +3744,25 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sig
const CTxIn& txin = psbtx.tx->vin[i]; const CTxIn& txin = psbtx.tx->vin[i];
PSBTInput& input = psbtx.inputs.at(i); PSBTInput& input = psbtx.inputs.at(i);
// If we don't know about this input, skip it and let someone else deal with it if (PSBTInputSigned(input)) {
const uint256& txhash = txin.prevout.hash; continue;
const auto it = pwallet->mapWallet.find(txhash); }
if (it != pwallet->mapWallet.end()) {
const CWalletTx& wtx = it->second; // Verify input looks sane. This will check that we have at most one uxto, witness or non-witness.
CTxOut utxo = wtx.tx->vout[txin.prevout.n]; if (!input.IsSane()) {
// Update both UTXOs from the wallet. throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "PSBT input is not sane.");
input.non_witness_utxo = wtx.tx; }
input.witness_utxo = utxo;
// 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 // Get the Sighash type