SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot inputs, and SIGHASH_ALL for all other input types. This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.
See also bitcoin/bitcoin#22514
SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot
inputs, and SIGHASH_ALL for all other input types. This avoids adding an
unnecessary byte to the end of all Taproot signatures added to PSBTs
signed in the GUI.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, pablomartin4btc, hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--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.
This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.
Two bytes, isn't it? For the PSBT_IN_SIGHASH keytype
This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.
Two bytes, isn't it? For the PSBT_IN_SIGHASH keytype
No, that field is not added. This is referring to the actual signature that will show up in the scriptWitness, and as appears in PSBT_IN_TAPROOT_*_SIG fields beforehand.
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e
It would be useful for the FillPSBT doc in wallet.h to explain that passing SIGHASH_DEFAULT will use SIGHASH_ALL for pre-taproot inputs.
You have to crawl all the way to MutableTransactionSignatureCreator::CreateSig to find this out:
// BASE/WITNESS_V0 signatures don't support explicit SIGHASH_DEFAULT, use SIGHASH_ALL instead.
const int hashtype = nHashType == SIGHASH_DEFAULT ? SIGHASH_ALL : nHashType;
https://github.com/bitcoin/bitcoin/pull/31622/commits/e94bb4e5fc6dea67844a8495b948b679649b7b65 makes this problem go away though.
Additional background: https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#taproot-key-path-spending-signature-validation
A Taproot signature is a 64-byte Schnorr signature, as defined in BIP340, with the sighash byte appended in the usual Bitcoin fashion. This sighash byte is optional. If omitted, the resulting signatures are 64 bytes, and a SIGHASH_DEFAULT mode is implied.
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e
I agree with @Sjors but is that not covered by doc at the /** Signature hash types/flags */ enum definition (src/script/interpreter.h)?
https://github.com/bitcoin/bitcoin/commit/e94bb4e5fc6dea67844a8495b948b679649b7b65 makes this problem go away though.
is that part of a PR?
@pablomartin4btc it's part of #31622.
The description in interpreter.h is ambiguous. It says "Taproot only". It also says "equivalent to SIGHASH_ALL", but doesn't say that it gets converted (and obviously a constant can't guarantee that actually happens).
ACK 3e97ff9c5eaa3160426ba112930b047404c54c9e, I have reviewed the code and it looks OK.
