The current PSBT signing code can end up producing a non-segwit signature, while only the UTXO being spent is provided in the PSBT (as opposed to the entire transaction being spent). This may be used to trick a user to incorrectly decide a transaction has the semantics he intends to sign.
Fix this by refusing to sign if there is any mismatch between the provided data and what is being signed.
Excellent! Thanks for tightening PSBT!
Concept ACK
4526 | @@ -4525,10 +4527,12 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C 4527 | } 4528 | 4529 | // Drop the unnecessary UTXO 4530 | - if (sigdata.witness) { 4531 | - input.non_witness_utxo = nullptr; 4532 | - } else { 4533 | - input.witness_utxo.SetNull(); 4534 | + if (from_wallet) {
Commit "Only wipe wrong UTXO type data if overwritten by wallet"
Suggestion, either reuse above condition if (it != pwallet->mapWallet.end())or above do:
const bool from_wallet = it != pwallet->mapWallet.end();
if (from_wallet) {
...
}
Done.
243 | @@ -244,17 +244,33 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t 244 | input.FillSignatureData(sigdata); 245 | 246 | // Get UTXO 247 | + bool require_witness_sig = false; 248 | CTxOut utxo; 249 | if (input.non_witness_utxo) { 250 | + // If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
Commit "Additional sanity checks in SignPSBTInput"
nit, could reflow comments.
I think they're fine.
<!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
utACK
Here is a commit with some more test cases: https://github.com/achow101/bitcoin/commit/9f110e1acbb1dc882ea871043fd03cf9064a1063. It tests for non-matching redeem scripts with both witness and non-witness utxos and non-matching witness scripts (for witness utxo only)
249 | if (input.non_witness_utxo) {
250 | + // If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
251 | + if (input.non_witness_utxo->GetHash() != tx.vin[index].prevout.hash) return false;
252 | + // If both witness and non-witness UTXO are provided, verify that they match. This check shouldn't
253 | + // matter, as the PSBT deserializer enforces only one of both is provided, and the only way both
254 | + // can be present is when they're added simultaneously by FillPSBT (in which case they always match).
Is this comment based on the BIP/spec or implementation? What about other implementations that may not do what FillPSBT does in Bitcoin Core? I understand that the check is done anyway, but the comment sounds like it could be skipped due to an implementation detail, which sounds error-prone.
utACK 2bc1296db7ba4e99f52a4002e67b11c0351819d6
utACK 2bc1296db7ba4e99f52a4002e67b11c0351819d6
Perhaps unrelated, but either createpsbt is too lenient, or decodepsbt is broken.
$ ./bitcoin-cli createpsbt "[]" "[{\"$(./bitcoin-cli getnewaddress)\":0.01}]"
cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==
$ ./bitcoin-cli decodepsbt cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==
error code: -22
error message:
TX decode failed CDataStream::read(): end of data: unspecified iostream_category error
re-utACK
re-utACK 5df6f089b53c5b5859e5a3454c026447e4752f82
utACK 5df6f089b53c5b5859e5a3454c026447e4752f82
Milestone
0.17.0