Add test vectors for BIP 341 SigMsg

giacomocaironi commented at 10:32 am on September 26, 2021: none

Add some tests to Taproot SigMsg, inspired by BIP 143 test vector

Add some tests to Taproot SigMsg, inspired by BIP 143 test vector

fa40b5ae35

jonasnick commented at 9:02 am on October 11, 2021: contributor

Hey @giacomocaironi, thanks a lot for your work. This looks like it would be quite helpful to a BIP 341 implementer.

I think test vectors should be part of the reference code’s test harness. Is it already? This would be the best way to ensure correctness of the test vectors. Also, I’m curious how you generated the vectors.

MarcoFalke commented at 9:18 am on October 11, 2021: member

Previous discussion thread at https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-September/019466.html

giacomocaironi commented at 7:20 pm on October 11, 2021: none

These test vectors are new, I created them from scratch them by randomly generating the keys. The reason I did this is that the official test vector contains non-standard transactions (they have strange version values). I thought it would by a nice addition if we added standard transactions to the test vectors, like BIP 143 did.

But if the hassle is too big, especially near the activation date, I can ignore this and simply pick some transactions from the official test vector instead of mine.

MegoyTambayan approved

MegoyTambayan commented at 9:50 pm on October 11, 2021: none

Nicely Done!

MarcoFalke cross-referenced this on Oct 14, 2021 from issue Add unit tests for Taproot code in src/script/interpreter.cpp by practicalswift

sipa commented at 8:50 pm on October 19, 2021: member

@giacomocaironi I think @jonasnick is asking that these test vectors are added to Bitcoin Core’s implementation, so that we can be sure they match the consensus implementation there.

I’ll try implementing them.

Tweak taproot keys 061d9159a2

in bip-0341.mediawiki:299 in fa40b5ae35 outdated

294+    private key  : 6b3973ee2ce444ada0147716925f6f77569350804835498593dd3be95163d558
295+    public key   : 0271be339aeae9ed2c6a5a7f8ac5f49638da387612be881c7ed2fb3848b0ef8a6c
296+    
297+  The second input comes from a P2TR
298+    scriptPubKey : 512029d942d0408906b359397b6f87c5145814a9aefc8c396dd05efa8b5b73576bf2, value: 6
299+    private key  : cf3780a32ef3b2d70366f0124ee40195a251044e82a13146106be75ee049ac02

sipa commented at 2:21 pm on October 20, 2021:

It looks like this is the tweaked private key corresponding to the listed scriptPubKey (or alternatively, the scriptPubKey isn’t tweaked at all). I would strongly prefer to only include test vectors that actually follow the procedure listed in the BIP for constructing taproot scriptPubKeys.

giacomocaironi commented at 10:05 pm on October 21, 2021:

Right I forgot to tweak the keys. I’ll update the pull request with the necessary modifications

sipa commented at 10:06 pm on October 21, 2021:

I’m working on creating my own set of test vectors, which also encompass scriptPubKey generation and control block computation for script path spending.

sipa commented at 9:42 pm on October 27, 2021:

Hi @giacomocaironi, sorry for duplicating some of your work, but what do you think about these: https://github.com/sipa/bips/blob/202110_bip341_vectors/bip-0341.mediawiki

giacomocaironi commented at 3:33 pm on October 28, 2021:

I would only add some fields to the intermediate results, such as the tweaked public key and all the fields which are part of the signature message (right now there are only global values such as sha_prevouts at the start of the test case).

sipa commented at 4:49 pm on October 29, 2021:

@giacomocaironi Done. WDYT?

giacomocaironi commented at 11:37 am on October 30, 2021:

LGTM. If bitcoin core passes the tests I think they should be merged as soon as possible

in bip-0341.mediawiki:326 in 061d9159a2

321+  = 8cdee56004a241f9c79cc55b7d79eaed04909d84660502a2d4e9c357c2047cf5
322+  
323+  hash preimage: 0000020000000000000032553b113292dfa8216546e721388a6c19c76626ca65dc187e0348d6ed445f815733468db74734c00efa0b466bca091d8f1aab074af2538f36bd0a734a5940c5189f262af17538f5e5df96b574a51c29038341001b8fd5ae356f985b9b880f1faf5570f5a1810b7af78caf4bc70a660f0df51e42baf91d4de5b2328de0e83dfc8cdee56004a241f9c79cc55b7d79eaed04909d84660502a2d4e9c357c2047cf50001000000
324+    
325+  sigHash:      71cc53d93fa4419e6ea19eeacd4c6ad57d82ee5dee8810cfa5b1a91efbd48329
326+  signature:    f6bfa62fd9edb4b823e35d9e72043ec0709fda7ee836d9acb9ebc9709746bc87c67ec4d32e08c92f16aa2e52bbff034d414d89d9e10d5ccc2785acd021c893f8

junderw commented at 11:55 am on October 30, 2021:

This test vector should mention the k nonce derivation follows libsecp256k1, or specify an aux_data for use with nonce generation (because the reference implementation requires it, if we eventually try to move this vector into the reference implementation, it will be needed anyways.)

As mentioned in my previous PR (#1218), many people will look at the reference implementation (which requires aux_data) and write their apps in a way that requires aux_data. Whereas these test vectors are using a null pointer for aux_data when signing using libsecp256k1, so many people who don’t use libsecp256k1 will be confused.

A simple one-liner explaining the nonce generation using terminology from BIP340 to describe this behavior, OR including a 32 byte aux_data in the vector (which is probably simpler) should be done in my opinion.

Other than that, LGTM, thanks a ton!

giacomocaironi commented at 1:58 pm on October 30, 2021:

Hi, thank for your suggestions. Including a 32 bytes aux_data instead of an unknown random one would certainly help because the entire test would be reproducible, but this is not its main objective. The main objective is to test the signature message, not the signature itself. Anyway if I am not mistaken we are going to close this PR, and accept the test vector by @sipa which already do what you suggested.

But for the rest I have to disagree with you. As people in your previous PR have already pointed out, a BIP is not tied to any specific library. It only provides a link to a reference implementation which may or may not be the main one (bitcoin core or libsecp256k1). That’s because a BIP is a design document, the reference is only there to help people who want to implement the BIP.

junderw commented at 4:52 pm on October 30, 2021:

The new vector containing aux_rand will make things much clearer to the implementer. I’m glad it will be included.

I never claimed that a BIP should be tied to any library. In the other PR I merely stated that having a reference implementation that takes one specific path on an aspect which the design document states is optional (and has infinite possible paths) without any mechanism to show it’s optional (ie. make it an option, pass the nonce function as an argument to sign, or even just write a comment in the reference reminding people “this is explicitly diverging from the design document for simplicity’s sake and implementing the recommended nonce derivation function which requires aux_data, while recommended, this is strictly optional” etc.) will (and has) lead to confusion.

My proposal to fix that point of possible confusion was implicitly rejected, and I really don’t care too much to fight for my specific fix.

However, debating the point that it is not confusing to anyone I would be more adamant.

That said, this debate is not something to be had on this PR.

Thanks for your time.

sipa commented at 5:07 pm on October 30, 2021:

@junderw https://github.com/bitcoin-core/secp256k1/pull/1002

junderw changes_requested

giacomocaironi cross-referenced this on Oct 30, 2021 from issue BIP341/342: Implementation clarifications by brandonblack

sipa cross-referenced this on Oct 30, 2021 from issue Taproot wallet test vectors (generation+tests) by sipa

sipa cross-referenced this on Oct 30, 2021 from issue BIP341 test vectors by sipa

giacomocaironi commented at 3:00 pm on October 30, 2021: none

Closing. This PR is superseded by #1225

giacomocaironi closed this on Oct 30, 2021

Add test vectors for BIP 341 SigMsg #1191