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
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);
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).
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;
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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
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);
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.
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.
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);