BIP341/342: Implementation clarifications #1224

brandonblack · 2021-10-29T22:02:58Z

These clarifications would each have prevented an error in our initial implementation of these BIPs.

giacomocaironi · 2021-10-30T14:13:44Z

bip-0341.mediawiki

 *** ''sha_prevouts'' (32): the SHA256 of the serialization of all input outpoints.
 *** ''sha_amounts'' (32): the SHA256 of the serialization of all spent output amounts.
-*** ''sha_scriptpubkeys'' (32): the SHA256 of the serialization of all spent output ''scriptPubKey''s.
+*** ''sha_scriptpubkeys'' (32): the SHA256 of the serialization of all spent output ''(compact_size(size of scriptPubKey) || scriptPubKey)''s.


sha_scriptpubkeys is the hash of the serializations of all spent scriptPubKeys. The serialization of a scriptPubKey is already (compact_size(size of scriptPubKey) || scriptPubKey). I know it may not be super clear but I don't think we should modify it. If it is necessary we should rather put a note somewhere to remember what the serialization of a scriptPubKey

True, I'll adjust the wording to be correct. The reason this part was confusing as written is that later in the description of SigMsg the annex is specifically called out as having its compact_size prepended to it. I think there should be consistency between this and that.

In the description below for scriptPubKey it says "serialized as script inside CTxOut". This is identical to your suggestion but it would be better to be consistent.

Thanks. Changed to use the common wording.

giacomocaironi · 2021-10-30T14:17:59Z

bip-0341.mediawiki


-The total length of ''SigMsg()'' is at most ''206'' bytes<ref>'''What is the output length of ''SigMsg()''?''' The total length of ''SigMsg()'' can be computed using the following formula: ''174 - is_anyonecanpay * 49 - is_none * 32 + has_annex * 32''.</ref>. Note that this does not include the size of sub-hashes such as ''sha_prevouts'', which may be cached across signatures of the same transaction.
+The total length of ''SigMsg()'' (without extensions) is at most ''206'' bytes<ref>'''What is the output length of ''SigMsg()''?''' The total length of ''SigMsg()'' can be computed using the following formula: ''174 - is_anyonecanpay * 49 - is_none * 32 + has_annex * 32''.</ref>. Note that this does not include the size of sub-hashes such as ''sha_prevouts'', which may be cached across signatures of the same transaction.



This is false. The total length of SigMsg is in fact at most 206 bytes. The message extension is appended to the output of SigMsg but it is not in itself part of SigMsg. In BIP 341 and 342 is clearly specified that what is hashed is not the output of SigMsg but rather 0x00 || SigMsg() || ext (if ext is present)

I don't think what you've said and this addition are at odds. As someone who is not a core developer reading this BIP, I would be inclined to allocate exactly 206 bytes for the data to be hashed as input to my signing function. Adding the parenthetical puts me on notice that there may be cases where I need a larger buffer.

Ok I understand your point. But the wording of the BIP is very precise, 206 bytes is the maximum total length of SigMsg(). We may clarify that what is signed is not the output SigMsg() but its output plus additional data; so the maximum total length of what is signed is not 206 bytes but 1 (version) + 206(SigMsg()) + 37(ext) = 244 bytes

I agree with @giacomocaironi on this one. This change makes me thing "if extensions are included, SigMsg() can exceed 206 bytes."

The message extension is appended to the output of SigMsg but it is not in itself part of SigMsg.

Agree. It's simpler if SigMsg remains independent of extensions. Your suggested change could be misunderstood as appending ext twice. First as an Extension inside SigMsg and then again due to the || operator.

I've changed the commit relating to SigMsg / extensions to avoid this confusion by adding a Taproot script path spending signature validation section.

giacomocaironi · 2021-10-30T14:30:34Z

Hi @brandonblack. I am working on some test vectors for these two BIP's because I too spent a lot of time trying to implement them. I think if the tests would have prevented your errors. The pull request is #1191. Your feedback is very welcome!
UPDATE: pull request is now superseded by #1225

brandonblack · 2021-10-31T03:41:35Z

Yes, those test vectors would have been helpful (although we generated our own by patching bitcoin-core, probably similar to how you did).

jonasnick

Thanks @brandonblack for your valuable feedback.

jonasnick · 2021-10-31T19:12:48Z

bip-0341.mediawiki

 The function ''SigMsg(hash_type, ext_flag)'' computes the message being signed as a byte array. It is implicitly also a function of the spending transaction and the outputs it spends, but these are not listed to keep notation simple.

-The parameter ''hash_type'' is an 8-bit unsigned value. The <code>SIGHASH</code> encodings from the legacy script system are reused, including <code>SIGHASH_ALL</code>, <code>SIGHASH_NONE</code>, <code>SIGHASH_SINGLE</code>, and <code>SIGHASH_ANYONECANPAY</code>, plus the default ''hash_type'' value ''0x00'' which results in signing over the whole transaction just as for <code>SIGHASH_ALL</code>. The following restrictions apply, which cause validation failure if violated:
+The parameter ''hash_type'' is an 8-bit unsigned value. The <code>SIGHASH</code> encodings from the legacy script system are reused, including <code>SIGHASH_ALL</code>, <code>SIGHASH_NONE</code>, <code>SIGHASH_SINGLE</code>, and <code>SIGHASH_ANYONECANPAY</code>, plus <code>SIGHASH_DEFAULT</code> (value ''0x00'') which results in signing over the whole transaction just as for <code>SIGHASH_ALL</code>. The following restrictions apply, which cause validation failure if violated:


Since Core apparently gives this a name, it seems like a good idea to have a standardized name for this. How about rephrasing this so it's more clear that this is a new hashtype?

The SIGHASH encodings from the legacy script system are reused, including SIGHASH_ALL, SIGHASH_NONE, SIGHASH_SINGLE, and SIGHASH_ANYONECANPAY.
We define a new ''hashtype'' SIGHASH_DEFAULT (value ''0x00'') which results in signing over the whole transaction just as for SIGHASH_ALL.

Thanks, changed the wording.

jonasnick · 2021-10-31T19:16:44Z

bip-0341.mediawiki


-The total length of ''SigMsg()'' is at most ''206'' bytes<ref>'''What is the output length of ''SigMsg()''?''' The total length of ''SigMsg()'' can be computed using the following formula: ''174 - is_anyonecanpay * 49 - is_none * 32 + has_annex * 32''.</ref>. Note that this does not include the size of sub-hashes such as ''sha_prevouts'', which may be cached across signatures of the same transaction.
+The total length of ''SigMsg()'' (without extensions) is at most ''206'' bytes<ref>'''What is the output length of ''SigMsg()''?''' The total length of ''SigMsg()'' can be computed using the following formula: ''174 - is_anyonecanpay * 49 - is_none * 32 + has_annex * 32''.</ref>. Note that this does not include the size of sub-hashes such as ''sha_prevouts'', which may be cached across signatures of the same transaction.



The message extension is appended to the output of SigMsg but it is not in itself part of SigMsg.

Agree. It's simpler if SigMsg remains independent of extensions. Your suggested change could be misunderstood as appending ext twice. First as an Extension inside SigMsg and then again due to the || operator.

jonasnick · 2021-10-31T19:40:31Z

bip-0341.mediawiki

 In order to allow spending with the key path, we define <code>taproot_tweak_seckey</code> to compute the secret key for a tweaked public key.
 For any byte string <code>h</code> it holds that <code>taproot_tweak_pubkey(pubkey_gen(seckey), h)[1] == pubkey_gen(taproot_tweak_seckey(seckey, h))</code>.

+Note that the secret key must be negated, if necessary, to produce an even-y pubkey before applying the tweak.


This could be misunderstood as an instruction to negate the secret key before calling taproot_tweak_seckey. Also, public keys are 32 byte arrays and therefore don't have even- or odd-Y (and the corresponding points are always even-Y).

How about something like this:
"Note that because tweaks are applied to 32-byte public keys, taproot_tweak_seckey may need to negate the secret key before applying the tweak."

Thank you. Changed the wording.

brandonblack · 2021-11-02T19:20:43Z

Thanks for all the quick feedback! I don't seem to be able to re-request review, but I think I've addressed them.

jonasnick · 2021-11-03T21:54:36Z

bip-0341.mediawiki

+==== Taproot script path spending signature validation ====
+
+Validation is identical to ''key path validation'' except that the input to hash<sub>TapSighash</sub> is ''(0x00 || SigMsg(hash_type, ext_flag) || ext)''.
+
+The value of ''ext'' is computed as specified in the BIP corresponding a particular value of ''ext_flag''.
+


That's a semantic change of the BIP and precludes new leaf versions from computing the signature message hash differently. There's no reason to do that.

Also the beginning of the section Signature validation rules mentions that it only applies to key spending.

Mmm. Right, will remove.

* Pull the definition of the extension in BIP342 to its own section * Add a section to BIP341 on validating script path signatures * Clarify that SigMsg does not produce the message being signed, but a common portion of it

luke-jr · 2021-11-04T23:12:27Z

@sipa @jonasnick @ajtowns

sipa · 2021-11-05T21:40:33Z

ACK 6222dc4, conditional on @jonasnick feeling his comments have been addressed.

jonasnick

ACK 6222dc4

ajtowns · 2021-11-07T06:16:20Z

ACK 6222dc4

giacomocaironi reviewed Oct 30, 2021

View reviewed changes

brandonblack force-pushed the master branch 2 times, most recently from a81124f to 869b6a0 Compare October 31, 2021 03:40

jonasnick reviewed Oct 31, 2021

View reviewed changes

reardencode added 2 commits November 1, 2021 07:20

BIP341: SigHash: Clarify SIGHASH_DEFAULT

5b9b44c

BIP341: SigHash: Clarify encoding of script pub keys

736e79c

brandonblack force-pushed the master branch from 869b6a0 to 4e44559 Compare November 1, 2021 14:30

jonasnick reviewed Nov 3, 2021

View reviewed changes

reardencode added 2 commits November 3, 2021 15:05

BIP341/342: Clarify SigHash extensions

d690408

* Pull the definition of the extension in BIP342 to its own section * Add a section to BIP341 on validating script path signatures * Clarify that SigMsg does not produce the message being signed, but a common portion of it

BIP341: Clarify tweaking of secret keys

6222dc4

brandonblack force-pushed the master branch from 4e44559 to 6222dc4 Compare November 3, 2021 22:07

luke-jr added the Proposed BIP modification label Nov 4, 2021

jonasnick approved these changes Nov 6, 2021

View reviewed changes

kallewoof merged commit b155143 into bitcoin:master Nov 11, 2021


		The total length of ''SigMsg()'' is at most ''206'' bytes<ref>'''What is the output length of ''SigMsg()''?''' The total length of ''SigMsg()'' can be computed using the following formula: ''174 - is_anyonecanpay * 49 - is_none * 32 + has_annex * 32''.</ref>. Note that this does not include the size of sub-hashes such as ''sha_prevouts'', which may be cached across signatures of the same transaction.
		The total length of ''SigMsg()'' (without extensions) is at most ''206'' bytes<ref>'''What is the output length of ''SigMsg()''?''' The total length of ''SigMsg()'' can be computed using the following formula: ''174 - is_anyonecanpay * 49 - is_none * 32 + has_annex * 32''.</ref>. Note that this does not include the size of sub-hashes such as ''sha_prevouts'', which may be cached across signatures of the same transaction.

BIP341/342: Implementation clarifications #1224

BIP341/342: Implementation clarifications #1224

Uh oh!

Conversation

brandonblack commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giacomocaironi Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giacomocaironi commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonblack commented Oct 31, 2021

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonblack commented Nov 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Nov 4, 2021

Uh oh!

sipa commented Nov 5, 2021

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Nov 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

giacomocaironi Oct 30, 2021 •

edited

Loading

giacomocaironi commented Oct 30, 2021 •

edited

Loading