BIP341/342: Implementation clarifications #1224

pull brandonblack wants to merge 4 commits into bitcoin:master from brandonblack:master changing 2 files +14 −8
  1. 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.
  2. 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.
  3. 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.
  4. 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
  5. brandonblack force-pushed on Oct 31, 2021
  6. brandonblack force-pushed on Oct 31, 2021
  7. 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).
  8. 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.
  9. 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.
  10. jonasnick commented at 7:41 pm on October 31, 2021: contributor
    Thanks @brandonblack for your valuable feedback.
  11. BIP341: SigHash: Clarify SIGHASH_DEFAULT 5b9b44cc79
  12. BIP341: SigHash: Clarify encoding of script pub keys 736e79c938
  13. brandonblack force-pushed on Nov 1, 2021
  14. 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.
  15. 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.
  16. 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
  17. BIP341: Clarify tweaking of secret keys 6222dc45a3
  18. brandonblack force-pushed on Nov 3, 2021
  19. luke-jr commented at 11:12 pm on November 4, 2021: member
  20. luke-jr added the label Proposed BIP modification on Nov 4, 2021
  21. sipa commented at 9:40 pm on November 5, 2021: member
    ACK 6222dc45a301c9b7d83536e2cd97d42899f5cb85, conditional on @jonasnick feeling his comments have been addressed.
  22. jonasnick approved
  23. jonasnick commented at 10:18 pm on November 6, 2021: contributor
    ACK 6222dc45a301c9b7d83536e2cd97d42899f5cb85
  24. ajtowns commented at 6:16 am on November 7, 2021: contributor
    ACK 6222dc45a301c9b7d83536e2cd97d42899f5cb85
  25. kallewoof merged this on Nov 11, 2021
  26. kallewoof closed this on Nov 11, 2021


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 01:10 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me