Fix the first two test vectors of verify fail test #1591

siv2r commented at 1:14 PM on May 11, 2024: contributor

Two vectors of the sign_verify test use different partial signatures even though their signer set, message, and nonces are the same. These are those two vectors:

Vector 1 (in valid_test_ cases):

"key_indices": [0, 1, 2],
"nonce_indices": [0, 1, 2],
"aggnonce_index": 0,
"msg_index": 0,
"signer_index": 0,
"expected": "012ABBCB52B3016AC03AD82395A1A415C48B93DEF78718E62A7A90052FE224FB"

Vector 2 (in verify_fail_test_cases):

"sig": "68537CC5234E505BD14061F8DA9E90C220A181855FD8BDB7F127BB12403B4D3B",
"key_indices": [0, 1, 2],
"nonce_indices": [0, 1, 2],
"msg_index": 0,
"signer_index": 1,
"comment": "Wrong signer"

In Vector 2, if we change the signer_index to 0 (the correct index), partial_sig_verify will still fail. Therefore, it has the incorrect partial signature. As a result, creating another vector by negating this paritalsig is also incorrect.

This PR fixes Vector 2 and the one created by negating Vector 2’s partial signature.

cc @jonasnick @real-or-random

jonasnick commented at 2:00 PM on May 11, 2024: contributor

Concept ACK, thanks @siv2r

Can confirm that the comments of vector 1 and 2 of "verify_fail_test_cases" are incorrect and that the suggested vector fixes this. Check existing and suggested vector 1 with this script:

def parse_hex(x):
    return int.from_bytes(bytes.fromhex(x), byteorder="big")

wrong_s = parse_hex("68537CC5234E505BD14061F8DA9E90C220A181855FD8BDB7F127BB12403B4D3B")
assert((n - wrong_s) == parse_hex("97AC833ADCB1AFA42EBF9E0725616F3C9A0D5B614F6FE283CEAAA37A8FFAF406"))

corr_s = parse_hex("012ABBCB52B3016AC03AD82395A1A415C48B93DEF78718E62A7A90052FE224FB")
assert((n - corr_s) == parse_hex("FED54434AD4CFE953FC527DC6A5E5BE8F6234907B7C187559557CE87A0541C46"))

I doubt it's worth incrementing the patch version of the spec but it would be nice if we could somehow reflect this in the change log, for example by adding a line to the top like

* 1.0.0 (2024-05-11):
    * Fix minor issue in two test vectors.

siv2r force-pushed on May 11, 2024

siv2r commented at 2:22 PM on May 11, 2024: contributor

I updated the change log as suggested. Also, the verify_error_test used the same wrong sig, so I updated those too.

jonatack added the label Pending acceptance on May 11, 2024

jonatack requested review from jonasnick on May 11, 2024

in bip-0327.mediawiki:788 in 921536b5ac outdated

 784 | @@ -785,6 +785,8 @@ An exception to this rule is <code>MAJOR</code> version zero (0.y.z) which is fo
 785 |  The <code>MINOR</code> version is incremented whenever the inputs or the output of an algorithm changes in a backward-compatible way or new backward-compatible functionality is added.
 786 |  The <code>PATCH</code> version is incremented for other changes that are noteworthy (bug fixes, test vectors, important clarifications, etc.).
 787 |  
 788 | +* '''1.0.0''' (2024-05-11):

jonasnick commented at 12:48 PM on May 13, 2024:

@real-or-random convinced me that it's better to make this 1.0.1.

in bip-0327.mediawiki:789 in 921536b5ac outdated

 784 | @@ -785,6 +785,8 @@ An exception to this rule is <code>MAJOR</code> version zero (0.y.z) which is fo
 785 |  The <code>MINOR</code> version is incremented whenever the inputs or the output of an algorithm changes in a backward-compatible way or new backward-compatible functionality is added.
 786 |  The <code>PATCH</code> version is incremented for other changes that are noteworthy (bug fixes, test vectors, important clarifications, etc.).
 787 |  
 788 | +* '''1.0.0''' (2024-05-11):
 789 | +** Fix minor issue in four test vectors.

jonasnick commented at 12:55 PM on May 13, 2024:

** Fix minor issue in ''PartialSigVerify'' test vectors.

siv2r force-pushed on May 13, 2024

siv2r commented at 1:05 PM on May 13, 2024: contributor

Addressed the change log comments

in bip-0327.mediawiki:788 in 83f831bcbb outdated

 784 | @@ -785,6 +785,8 @@ An exception to this rule is <code>MAJOR</code> version zero (0.y.z) which is fo
 785 |  The <code>MINOR</code> version is incremented whenever the inputs or the output of an algorithm changes in a backward-compatible way or new backward-compatible functionality is added.
 786 |  The <code>PATCH</code> version is incremented for other changes that are noteworthy (bug fixes, test vectors, important clarifications, etc.).
 787 |  
 788 | +* '''1.0.1''' (2024-05-11):

jonasnick commented at 1:19 PM on May 13, 2024:

The date doesn't match the date of the commit. Should be 2024-05-13.

siv2r commented at 2:12 AM on May 14, 2024:

Fixed

real-or-random approved

real-or-random commented at 2:05 PM on May 13, 2024: contributor

utACK 83f831bcbbdcf87b983b013eea248c19498a824f -- mod the date change (Yeah, it's better to set it to the commit date, but two days more or less don't make that big of a difference in the end :D

It took me a while to understand what's going on, but it all makes sense now. Nice catch!

jonatack added the label PR Author action required on May 13, 2024

jonatack removed the label Pending acceptance on May 13, 2024

Fix the four test vectors

- first two vectors of verify_fail_test
- first two vectors of verify_error_test

508e3a6a40

siv2r force-pushed on May 14, 2024

real-or-random approved

real-or-random commented at 7:37 AM on May 14, 2024: contributor

utACK 508e3a6a40a6e73c73cbfa8a33aa18a2bc7b9d91

jonatack removed the label PR Author action required on May 14, 2024

jonatack merged this on May 14, 2024

jonatack closed this on May 14, 2024

jonasnick commented at 8:19 AM on May 15, 2024: contributor

Yeah, it's better to set it to the commit date, but two days more or less don't make that big of a difference in the end

I think the purpose of the date is to make it easier for people to look up the changes in the commit history. However, the date is wrong now anyway because github UI does not show the correct date of the commit :shrug:.