As discussed in the comments on #14473, I think that bug was caused primarily by failure to adhere to the invariant that a PSBTInput always has exactly one of the two utxo fields present -- an invariant that is already enforced by PSBTInput::IsSane, but which we were temporarily suspending during signing.
This refactor repairs the invariant, also fixing the bug. It also simplifies some other code, and removes redundant parameters from some related functions.
fixes #14473
Arguably, we may want to call PartiallySignedTransaction::IsSane as the first step in every PSBT RPC, so we can give a thoughtful error message if it fails (or at least an error message.)
1541 | bool complete = true;
1542 | for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
1543 | PSBTInput& input = psbtx.inputs.at(i);
1544 | -
1545 | - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, 1);
1546 | + complete &= PSBTInputSigned(input);
I think the SignPSBTInput call here was necessary to perform finalization (constructing the actual scriptSig/witness; there is no guarantee that the previous step actually did this, for example if it was a combiner that didn't understand the script).
Aha, thanks, not sure how I misunderstood that so badly, it makes much more sense to me now. I will change it back and add a comment.
3757 | - if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) { 3758 | - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Specified Sighash and sighash in PSBT do not match."); 3759 | + // Get the sighash_type (if we're signing). 3760 | + if (sign) { 3761 | + if (input.sighash_type == 0) { 3762 | + input.sighash_type = sighash_type;
Will this sighash_type value get serialized into the PSBT? If so, I believe that's incorrect; if no sighash type is specified in the PSBT file, signers can choose their own, but the choice of one signer shouldn't affect the type used by future/other signers.
Ahh, I see. Ok, will fix, thanks.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
@achow101 I think this probably supersedes #14197? It has 14197's effect as a side-effect, but having done so, that also allows removing all the code to handle having both types of utxos at the same time (since the non_witness_utxo is now sufficient.)
If you agree, I would like to steal your test from 14197 though. :-)
Use .str() instead of .data() and .size() when converting CDataStream to
a string. Uses std::string, avoiding conversion to a C string.
New constructor that creates a PartiallySignedTransaction from a
CTransaction, automatically sizing the inputs and outputs vectors for
convenience.
Refactor out a "PSBTInputSigned" function to check if a PSBT is signed,
for use in subsequent commits.
Also improve a related comment.
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.
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
utACK e13fea975d5e4ae961faba36379a1cdaf9e50c1c
29 | @@ -30,5 +30,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException); 30 | 31 | UniValue getaddressinfo(const JSONRPCRequest& request); 32 | UniValue signrawtransactionwithwallet(const JSONRPCRequest& request); 33 | -bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1, bool sign = true, bool bip32derivs = false); 34 | +bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false);
Why not int sighash_type = SIGHASH_ALL directly?
Well, I wouldn't want to do that without #including script/interpreter.h for the definition of SIGHASH_ALL. (As it turns out, it works without doing that, but that means it's counting on other files to #include specific things in a specific order, which is gross and wrong.) And I didn't want to add a dependency to a header file just for a constant.
But it seems like everything in the universe already includes interpreter.h anyway, so I'm willing to make the change if you prefer.
Oh, I see. I was just wondering if there was a reason - seems there is. No need to hold this PR up for.
utACK e13fea975d5e4ae961faba36379a1cdaf9e50c1c
Backport me?
I've marked it for backport, but this may not be trivial as some of the code it builds upon has changed since 0.17?
58 | @@ -59,12 +59,8 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) 59 | CDataStream ssData(ParseHex("70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f000000000000000000"), SER_NETWORK, PROTOCOL_VERSION); 60 | ssData >> psbtx; 61 | 62 | - // Use CTransaction for the constant parts of the 63 | - // transaction to avoid rehashing.
For posterity, can you explain what the "rehashing" problem was and why it's not actually a problem / why this didn't solve it?
The explanation I received was that the comment was a cut-and-paste from elsewhere and didn't ever actually apply here.
The underlying issue is that calling GetHash on a CMutableTransaction repeatedly is a waste of effort, since it will not cache the transaction hash but recompute it every time. So one is well-advised to copy a CMutableTransaction to a CTransaction if one is done modifying it, and will be calling GetHash more than once. But that doesn't apply here, as we only make extremely limited used of the underlying transaction in FillPSBT, and we never call GetHash on it.
1544 | bool complete = true;
1545 | for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
1546 | PSBTInput& input = psbtx.inputs.at(i);
1547 |
1548 | - complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, 1);
1549 | + complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, SIGHASH_ALL);
This is the kind of thing that makes me want to shill for linters (or better use of the type system) :-) cc @practicalswift
Post merge ACK