Fix incorrect signature test vectors in BIP322 #1323

wip-abramson commented at 12:56 PM on May 17, 2022: contributor

While attempting to implement bip322 I was unable to generate the same signature as the ones provided. Furthermore, I was unable to verify these signatures in my implementation. After some digging, I noticed the test vectors in the bip322 PR to Bitcoin Core uses different values.

See util_tests.cpp in https://github.com/bitcoin/bitcoin/pull/24058

This P.R. replaces the current test vectors with the ones from the P.R.

It also adds additional test vectors for the transaction hashes of the to_spend and to_sign transactions. Having these would have speeded up my debugging process significantly.

rxgrant commented at 2:56 PM on May 20, 2022: none

Can you point at the code needed to generate these vectors?

Can you test setting version to 2 to see if the old vectors were leftover cruft from before this fix?

wip-abramson commented at 12:54 PM on May 23, 2022: contributor

Setting the version to 2 still did not produce the test vectors currently provided in this bip.

I am not sure how these test vectors were generated, both the ones in the bip and the ones used in the P.R. - perhaps a question for @kallewoof

in bip-0322.mediawiki:177 in ff46cf9af6 outdated

 173 | @@ -174,7 +174,20 @@ Given below parameters:
 174 |  * private key <code>L3VFeEujGtevx9w18HD1fhRbCH67Az2dpCymeRE1SoPK6XQtaN2k</code>
 175 |  * corresponding address <code>bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l</code>
 176 |  
 177 | +

kallewoof commented at 6:41 AM on May 27, 2022:

nit: Unnecessary whitespace.

wip-abramson commented at 10:42 AM on May 27, 2022:

in bip-0322.mediawiki:180 in ff46cf9af6 outdated

 178 |  Produce signatures:
 179 |  
 180 | -* Message = "" (empty string): <code>AkcwRAIgFuS8y5m0ym9Gj2odoVB5NIL+cPYkeEj8LL1N/6P58X8CIA6jJ9QH2iYKRXVfmhsDzHq1bMS4Adj0nb8DDSdN/SpBASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=</code>
 181 | -* Message = "Hello World": <code>AkcwRAIgG3PASL/vRTgAqogWT6S8rUOQXNnfRzX6JncmbFlHc1ACIGQdsW+rnVmsQzyAYRQisHKFMigDmKiL7LUw4x17Fw5tASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=</code>
 182 | +* Message = "" (empty string): <code>AkcwRAIgM2gBAQqvZX15ZiysmKmQpDrG83avLIT492QBzLnQIxYCIBaTpOaD20qRlEylyxFSeEA2ba9YOixpX8z46TSDtS40ASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=</code>
 183 | +* Message = "Hello World": <code>AkcwRAIgZRfIY3p7/DoVTty6YZbWS71bc5Vct9p9Fia83eRmw2QCICK/ENGfwLtptFluMGs2KsqoNSk89pO7F29zJLUx9a/sASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=</code>

kallewoof commented at 6:46 AM on May 27, 2022:

Verified.

$ ./bitcoin-cli signmessage bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l ""
AkcwRAIgM2gBAQqvZX15ZiysmKmQpDrG83avLIT492QBzLnQIxYCIBaTpOaD20qRlEylyxFSeEA2ba9YOixpX8z46TSDtS40ASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=
$ ./bitcoin-cli signmessage bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l "Hello World"
AkcwRAIgZRfIY3p7/DoVTty6YZbWS71bc5Vct9p9Fia83eRmw2QCICK/ENGfwLtptFluMGs2KsqoNSk89pO7F29zJLUx9a/sASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=
$

kallewoof renamed this:
~~Fixe incorrect signature test vectors in BIP322~~
Fix incorrect signature test vectors in BIP322
on May 27, 2022

kallewoof commented at 10:09 AM on May 27, 2022: member

I am not sure how these test vectors were generated, both the ones in the bip and the ones used in the P.R. - perhaps a question for @kallewoof

Forgot to reply: I believe the test vectors in the BIP here were based on an outdated version. @rxgrant let me know if you have issues with or a different opinion on the matter.

Edit: missing word.

kallewoof commented at 2:21 PM on May 29, 2022: member

Squash?

wip-abramson commented at 1:09 PM on June 2, 2022: contributor

Sorry @kallewoof are you asking if we should squash, or asking me to squash?

Happy to squash if that is preferable.

laanwj commented at 3:05 PM on June 2, 2022: member

Yes, please squash, this looks like a single atomic change.

kallewoof commented at 5:05 AM on June 3, 2022: member

↑

wip-abramson force-pushed on Jun 6, 2022

wip-abramson commented at 1:09 PM on June 6, 2022: contributor

Is this squashed? I seem to just be adding more commits.

Apologies, I don't think I have ever attempted to squash before

michaelfolkson commented at 1:42 PM on June 6, 2022: contributor

@wip-abramson: There are 4 commits now. For some help on squashing commits see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits or page 284 of the Pro Git book. You also have a merge commit that you'll want to remove.

If you'd rather start afresh on your branch and go back to latest top commit on master you can do a git reset -hard insert_current_top_commit_hash, re-add your commit and force push.

edit: It is also recommended that you open a PR from a topic branch rather than the master branch in your fork.

Fix incorrect signature test vectors in BIP322 8ed2af0784

wip-abramson force-pushed on Jun 6, 2022

wip-abramson commented at 2:17 PM on June 6, 2022: contributor

Thanks @michaelfolkson. This looks more like it. Although, if you prefer I could reopen this PR from a branch as suggested.

michaelfolkson commented at 2:19 PM on June 6, 2022: contributor

@wip-abramson: I don't think this BIPs repo is as strict on that as the Core repo is but one of the BIP editors can correct me if wrong. It is definitely better practice though for any future PRs made to this repo.

kallewoof merged this on Jun 7, 2022

kallewoof closed this on Jun 7, 2022

kallewoof commented at 6:43 AM on June 7, 2022: member

Typically not as strict, but no need to be overly committy ;)

Mertz22 cross-referenced this on Jun 30, 2022 from issue Revert "Fix incorrect signature test vectors in BIP322" by Mertz22