Refactor PSBT signing logic to enforce invariant and fix signing bug #14588
Conversation
|
@achow101: As original author of most of this code, you should look at this and make sure I have not done anything crazy (and that you agree that by and large my changes are improvements.) |
|
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.) |
src/rpc/rawtransaction.cpp
Outdated
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Agree with @sipa. SignPSBTInput is what does the finalization.
There was a problem hiding this comment.
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.
src/wallet/rpcwallet.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh, I see. Ok, will fix, thanks.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. :-) |
|
@gwillen Go ahead. |
gwillen
force-pushed
the
feature-psbt-sign-fix
branch
3 times, most recently
from
72f2c6b to
2f40f52
Compare
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 bitcoin#14473
gwillen
force-pushed
the
feature-psbt-sign-fix
branch
from
2f40f52 to
e13fea9
Compare
|
It looks like #14197 got merged -- I just rebased over it, which effectively erases it on this branch since it does the same thing as this PR. (It does have an assert that I didn't include in mine -- I could if desired.) |
|
utACK e13fea9 |
| UniValue getaddressinfo(const JSONRPCRequest& request); | ||
| UniValue signrawtransactionwithwallet(const JSONRPCRequest& request); | ||
| bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1, bool sign = true, bool bip32derivs = false); | ||
| bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false); |
There was a problem hiding this comment.
Why not int sighash_type = SIGHASH_ALL directly?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, I see. I was just wondering if there was a reason - seems there is. No need to hold this PR up for.
|
utACK e13fea9 |
…x signing bug e13fea9 Add regression test for PSBT signing bug #14473 (Glenn Willen) 5655005 Refactor PSBTInput signing to enforce invariant (Glenn Willen) 0f5bda2 Simplify arguments to SignPSBTInput (Glenn Willen) 53e6fff Add bool PSBTInputSigned (Glenn Willen) 65166d4 New PartiallySignedTransaction constructor from CTransction (Glenn Willen) 4f3f5cb Remove redundant txConst parameter to FillPSBT (Glenn Willen) fe5d22b More concise conversion of CDataStream to string (Glenn Willen) Pull request description: 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 Tree-SHA512: cbad3428175e30f9b7bac3f600668dd1a8f9acde16b915d27a940a2fa6d5149d4fbe236d5808fd590fb20a032274c99e8cac34bef17f79a53fdf69a5948c0fd0
|
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? |
|
This will be backported in #14780. |
Use .str() instead of .data() and .size() when converting CDataStream to a string. Uses std::string, avoiding conversion to a C string. Github-Pull: bitcoin#14588 Rebased-From: fe5d22b
Github-Pull: bitcoin#14588 Rebased-From: 4f3f5cb
New constructor that creates a PartiallySignedTransaction from a CTransaction, automatically sizing the inputs and outputs vectors for convenience. Github-Pull: bitcoin#14588 Rebased-From: 65166d4
Refactor out a "PSBTInputSigned" function to check if a PSBT is signed, for use in subsequent commits. Also improve a related comment. GitHub-Pull: bitcoin#14588 Rebased-From: 53e6fff
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. Github-Pull: bitcoin#14588 Rebased-From: 0f5bda2
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 bitcoin#14473 Github-Pull: bitcoin#14588
Github-Pull: bitcoin#14588 Rebased-From: e13fea9
| ssData >> psbtx; | ||
|
|
||
| // Use CTransaction for the constant parts of the | ||
| // transaction to avoid rehashing. |
There was a problem hiding this comment.
For posterity, can you explain what the "rehashing" problem was and why it's not actually a problem / why this didn't solve it?
There was a problem hiding this comment.
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.
src/rpc/rawtransaction.cpp
Outdated
| PSBTInput& input = psbtx.inputs.at(i); | ||
|
|
||
| complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, 1); | ||
| complete &= SignPSBTInput(DUMMY_SIGNING_PROVIDER, *psbtx.tx, input, i, SIGHASH_ALL); |
There was a problem hiding this comment.
This is the kind of thing that makes me want to shill for linters (or better use of the type system) :-) cc @practicalswift
7bee414 Add test for conversion from non-witness to witness UTXO (Pieter Wuille) ff56bb9 Add regression test for PSBT signing bug #14473 (Glenn Willen) db445d4 Refactor PSBTInput signing to enforce invariant (Glenn Willen) ad94165 Simplify arguments to SignPSBTInput (Glenn Willen) 39ece4f Add bool PSBTInputSigned (Glenn Willen) 70ee1f8 New PartiallySignedTransaction constructor from CTransction (Glenn Willen) a9eab08 Remove redundant txConst parameter to FillPSBT (Glenn Willen) cfdd6b2 More concise conversion of CDataStream to string (Glenn Willen) a3fe125 check that a separator is found for psbt inputs, outputs, and global map (Andrew Chow) Pull request description: This is a backport of #14588, #14377, and #14197's test to 0.17. Tree-SHA512: 07535ec69a878a63b549e5e463345e233f34662dff805202614cf2ffc896c6d1981363e6d06d02db2e02d815075ad8ebdc5f93f637052cff8c8cbe6c8dfa096a
Summary: Backport of core [[bitcoin/bitcoin#14588 | PR14588]]. ``` This refactor repairs the invariant, [...]. It also simplifies some other code, and removes redundant parameters from some related functions. ``` Note that the bug fixed in the PR is not affecting us, but I kept the test as it can't hurt. Also the `IsSane()` method is kept for easier backports, despite it does nothing and will be optimized out by the compiler. Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5715
Summary: Backport of core [[bitcoin/bitcoin#14588 | PR14588]]. ``` This refactor repairs the invariant, [...]. It also simplifies some other code, and removes redundant parameters from some related functions. ``` Note that the bug fixed in the PR is not affecting us, but I kept the test as it can't hurt. Also the `IsSane()` method is kept for easier backports, despite it does nothing and will be optimized out by the compiler. Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5715
8 participants
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