BIP341/342: Implementation clarifications

brandonblack commented at 10:02 pm on October 29, 2021: contributor

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

in bip-0341.mediawiki:109 in d07faf7195 outdated

105@@ -106,7 +106,7 @@ If the parameters take acceptable values, the message is the concatenation of th
106 ** If the ''hash_type & 0x80'' does not equal <code>SIGHASH_ANYONECANPAY</code>:
107 *** ''sha_prevouts'' (32): the SHA256 of the serialization of all input outpoints.
108 *** ''sha_amounts'' (32): the SHA256 of the serialization of all spent output amounts.
109-*** ''sha_scriptpubkeys'' (32): the SHA256 of the serialization of all spent output ''scriptPubKey''s.
110+*** ''sha_scriptpubkeys'' (32): the SHA256 of the serialization of all spent output ''(compact_size(size of scriptPubKey) || scriptPubKey)''s.

giacomocaironi commented at 2:13 pm on October 30, 2021:

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

brandonblack commented at 3:37 am on October 31, 2021:

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.

jonasnick commented at 7:54 pm on October 31, 2021:

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.

brandonblack commented at 2:32 pm on November 1, 2021:

Thanks. Changed to use the common wording.

in bip-0341.mediawiki:129 in d07faf7195 outdated

123@@ -124,8 +124,9 @@ If the parameters take acceptable values, the message is the concatenation of th
124 * Data about this output:
125 ** If ''hash_type & 3'' equals <code>SIGHASH_SINGLE</code>:
126 *** ''sha_single_output'' (32): the SHA256 of the corresponding output in <code>CTxOut</code> format.
127+* Extensions if ''ext_flag != 0''
128 
129-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.
130+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.
131

giacomocaironi commented at 2:17 pm on October 30, 2021:

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)

brandonblack commented at 3:36 am on October 31, 2021:

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.

giacomocaironi commented at 8:51 am on October 31, 2021:

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

junderw commented at 1:08 pm on October 31, 2021:

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

jonasnick commented at 7:16 pm on October 31, 2021:

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.

brandonblack commented at 2:32 pm on November 1, 2021:

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

giacomocaironi commented at 2:30 pm on October 30, 2021: none

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 force-pushed on Oct 31, 2021

brandonblack commented at 3:41 am on October 31, 2021: contributor

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

in bip-0341.mediawiki:93 in 869b6a0caf outdated

89@@ -90,11 +90,11 @@ We first define a reusable common signature message calculation function, follow
90 
91 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.
92 
93-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:
94+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:

jonasnick commented at 7:12 pm on October 31, 2021:

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.

brandonblack commented at 2:31 pm on November 1, 2021:

Thanks, changed the wording.

in bip-0341.mediawiki:179 in 869b6a0caf outdated

175@@ -175,6 +176,8 @@ The parity bit will be required for spending the output with a script path.
176 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.
177 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>.
178 
179+Note that the secret key must be negated, if necessary, to produce an even-y pubkey before applying the tweak.

jonasnick commented at 7:40 pm on October 31, 2021:

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.”

brandonblack commented at 2:30 pm on November 1, 2021:

Thank you. Changed the wording.

jonasnick commented at 7:41 pm on October 31, 2021: contributor

Thanks @brandonblack for your valuable feedback.

BIP341: SigHash: Clarify SIGHASH_DEFAULT 5b9b44cc79

BIP341: SigHash: Clarify encoding of script pub keys 736e79c938

brandonblack force-pushed on Nov 1, 2021

brandonblack commented at 7:20 pm on November 2, 2021: contributor

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

in bip-0341.mediawiki:149 in 4e445598b4 outdated

140@@ -141,6 +141,12 @@ To validate a signature ''sig'' with public key ''q'':
141 * If the ''sig'' is 65 bytes long, return ''sig[64] &ne; 0x00<ref>'''Why can the <code>hash_type</code> not be <code>0x00</code> in 65-byte signatures?''' Permitting that would enable malleating (by third parties, including miners) 64-byte signatures into 65-byte ones, resulting in a different `wtxid` and a different fee rate than the creator intended</ref> and Verify(q, hash<sub>TapSighash</sub>(0x00 || SigMsg(sig[64], 0)), sig[0:64])''.
142 * Otherwise, fail<ref>'''Why permit two signature lengths?''' By making the most common type of <code>hash_type</code> implicit, a byte can often be saved.</ref>.
143 
144+==== Taproot script path spending signature validation ====
145+
146+Validation is identical to ''key path validation'' except that the input to hash<sub>TapSighash</sub> is ''(0x00 || SigMsg(hash_type, ext_flag) || ext)''.
147+
148+The value of ''ext'' is computed as specified in the BIP corresponding a particular value of ''ext_flag''.
149+

jonasnick commented at 9:54 pm on November 3, 2021:

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.

brandonblack commented at 10:05 pm on November 3, 2021:

Mmm. Right, will remove.

BIP341/342: Clarify SigHash extensions

* 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

d690408080

BIP341: Clarify tweaking of secret keys 6222dc45a3

brandonblack force-pushed on Nov 3, 2021

luke-jr commented at 11:12 pm on November 4, 2021: member

@sipa @jonasnick @ajtowns

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

sipa commented at 9:40 pm on November 5, 2021: member

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

jonasnick approved

jonasnick commented at 10:18 pm on November 6, 2021: contributor

ACK 6222dc45a301c9b7d83536e2cd97d42899f5cb85

ajtowns commented at 6:16 am on November 7, 2021: contributor

ACK 6222dc45a301c9b7d83536e2cd97d42899f5cb85

kallewoof merged this on Nov 11, 2021

kallewoof closed this on Nov 11, 2021

BIP341/342: Implementation clarifications #1224