vPSBT: add sighash support (no version bump) #779
Conversation
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
from
5b58fbe to
071ce34
Compare
| require.NoError(t.t, err) | ||
|
|
||
| // Edit the already signed PSBT and change the output amounts. This | ||
| // should be ok as we used SIGHASH_NONE for the input's signature. |
There was a problem hiding this comment.
Should also test swapping out the dest script. This is an important component for the non-interactive swap demo.
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
3 times, most recently
from
27ac081 to
5d4a45f
Compare
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
from
5d4a45f to
17237ee
Compare
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
2 times, most recently
from
1f277ad to
63c043a
Compare
vm/vm.go
Outdated
|
|
||
| // If we are commiting to the outputs, we need to also perform | ||
| // an inflation check. | ||
| if sighashFlag != txscript.SigHashNone { |
There was a problem hiding this comment.
We probably don't want to always do this?
This if is useful when vPSBTs are partially constructed and passed around, as tapd will run the vm on psbts that are being signed.
But at the same time we don't want to have transfers that are never checked in terms of supply.
For a vPSBT that has only one input of SigHashNone we need to signal when the inflation check should be performed or not, when there's no intention of further adding more inputs that commit to all inputs/outputs.
There was a problem hiding this comment.
Copying my answer from chat:
I think we only should skip the check at sign time but not at validation time. I think I mentioned this before, but the current check we have in the
SignVirtualPsbtcode where we validate the signature directly after signing probably needs to be removed or updated in any case. Otherwise we couldn't use that RPC for multi-signature as it would always fail for the first signer(s). So maybe we can pass in a booleanisSigninginto the VM so we know we can skip some checks
There was a problem hiding this comment.
If it's about the validation flow for vPSBTs, then I think for that flow, we can just compute the tapscript sighash directly and validate the signature given the available information. This way, we don't need a special case in the VM that could possibly be exploited with some edge case (valid sig that actually_does_ do inflation).
I think by the time we get to the layer of fully validating a proof file, we want the strictness checks possible to avoid inadvertent vectors that allow side stepping the rule set.
There was a problem hiding this comment.
Reading in more of the diff: this won't work as intended if the virtual tx at this point is incomplete (output doesn't have the true value/amount).
Also the check (if it were needed, I don't think it is) needs to actually apply the sighash mask to extract the value rather than do a plain equality check.
|
diff significantly reduced, also tidied up the commits a bit, PTAL |
vm/vm.go
Outdated
|
|
||
| // Check that we support the provided sighash flag. | ||
| switch sighashFlag { | ||
| case txscript.SigHashDefault, txscript.SigHashAll, |
There was a problem hiding this comment.
Why do we need to officially support some flags vs others? Stuff like single or anyonecan pay has no bearing at this level. It's also the case that the logic above doesn't exhaustively extract sighashes: it assumes keyspend only. All sigs within the witness can also have sighashes, eg: someone doing OP_CHECKSGIADD.
There was a problem hiding this comment.
Also sighash values are masks, you can't just compare directly.
There was a problem hiding this comment.
Stuff like single or anyonecan pay has no bearing at this level
So should I allow them even though they'll have no effect, or a malformed effect?
vm/vm.go
Outdated
|
|
||
| // If we are commiting to the outputs, we need to also perform | ||
| // an inflation check. | ||
| if sighashFlag != txscript.SigHashNone { |
There was a problem hiding this comment.
Reading in more of the diff: this won't work as intended if the virtual tx at this point is incomplete (output doesn't have the true value/amount).
Also the check (if it were needed, I don't think it is) needs to actually apply the sighash mask to extract the value rather than do a plain equality check.
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
from
63c043a to
ba3401c
Compare
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
from
ba3401c to
e5b02db
Compare
There was a problem hiding this comment.
Hmm, I'm confused. Did something get lost in a rebase somewhere? Because I don't see the actual changes for supporting
SigHashNoneanymore... Also makes me wonder why the unit/integration tests still run file? Or am I missing something?
Okay, I guess I missed the part that we already hooked in the vPSBTs sighash flag into the actual sign descriptor. So all good here (after addressing nits).
GeorgeTsagk
force-pushed
the
vpsbt-sighash-simple
branch
from
e5b02db to
59acd27
Compare
Description
This PR replaces #759 , as it introduces simple sighash functionality (
SIGHASH_ALLandSIGHASH_NONE) for vPSBTs without changing the way we create the virtual transactions that are being signed. This allows us to be backwards compatible and does not require a new script version.