Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

SigHash changes seem not needed to me. #6

Closed
zander opened this issue May 28, 2021 · 1 comment
Closed

SigHash changes seem not needed to me. #6

zander opened this issue May 28, 2021 · 1 comment

Comments

@zander
Copy link
Contributor

zander commented May 28, 2021

The CHIP includes a suggestion for a new sighash (0x04) detached.

This looks unneeded to me because a) we already have ForkID and b) the signature being detached is really already signalled by the fact that the push is too short to be a signature.

The adding of this SigHash also doesn't make much sense because it doesn't change the way the signature hash is calculated. So I don't think it belongs.

I don't know if the sighash makes most sense on the signature (in the detached-signatures area of the tx) or in the proof. Initially I would say it belongs in the detached-signatures area. Essentially following current practice of having the siganture-push include (in the same stack-unit) the sighash.

If you have a reason to move it to the proof instead, would be good to mention why.

@bitjson
Copy link
Owner

bitjson commented Jun 23, 2021

Hey, thanks for bringing this up @zander! The spec was also sorely missing a clearer description of the signing serialization algorithm for detached signatures. (It was just described in a couple sentences of prose, now there's a clearer table.)

I agree that replay protection was poorly addressed – it's valuable to have the Fork ID field in signing serializations to keep it consistent with the existing signing serialization algorithms. (That should make better replay protection simpler to implement for all of them at the same time, if it's needed.) Fixed by #9 👍

I also agree on sighash byte location – I had originally left it inside the inputs because I somehow thought it was needed for future signature aggregation changes, but it definitely isn't: a future upgrade could add a SIGHASH_AGGREGATE flag + algorithm that even supports many-to-many groupings without changing this transaction format. So I definitely prefer it how you suggested – the sighash byte is appended to the signature in the detached signatures area (like with all other signatures).

And the updated spec now differentiates "signatures" from "signature references" because signature references are less than or equal to 2 bytes in length.

So I think we can consider this issue fixed by #9? I'll close for now, but please let me know if there's more we need to address!

@bitjson bitjson closed this as completed Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants