bip327: minor fixes #1647

1. 
   siv2r commented at 10:08 am on July 17, 2024: contributor

   I made some minor corrections, which I believe are beneficial. Please let me know if any of them are incorrect or need modification.

   Changes:

   1. An error test vector doesn’t specify the InvalidContributionError type
   2. In DeterministicSign, use GetXonlyPubkey instead of GetPubkey (doesn’t exist)
   3. The key_agg_and_tweak fn doesn’t specify the return type
   4. In partial_sig_verify_internal, the pubkey arg type should be PlainPk rather than bytes
   5. Remove unnecessary enumerate() calls
      • I suspect this was initially used in debugging to find the failing test vector, and then it was forgotten.
   6. In test_sign_verify, add an additional assert statement that checks if two pubnonces add up to the infinity aggnonce.

   cc @jonasnick @real-or-random

2. 
   siv2r commented at 11:47 am on July 18, 2024: contributor

   Additionally, I removed the following P is None check, since cpoint never returns None. https://github.com/bitcoin/bips/blob/812907c2b00b92ee31e2b638622a4fe14a428aee/bip-0327/reference.py#L442-L444

   We should probably return False when cpoint throws an exception when parsing P, R_s1, and R_s2. Currently, the partial_sig_verify_internal simply crashes.

3. in bip-0327/vectors/sig_agg_vectors.json:147 in 2fc3ef4bb2 outdated

   142@@ -143,7 +143,8 @@
   143             ],
   144             "error": {
   145                 "type": "invalid_contribution",
   146-                "signer": 1
   147+                "signer": 1,
   148+                "contrib": "psig"
   

   ---

   ---

   

   stratospher commented at 10:56 am on July 19, 2024:

   b6d7143: you’ll also need to make this change in the python file used to generate the json file - bip-0327/gen_vectors_helper.py

   ---

   

   siv2r commented at 6:24 pm on July 19, 2024:

   Ahh yes. I missed it. Thanks! Fixed now.
4. in bip-0327/reference.py:600 in 2fc3ef4bb2 outdated

   595@@ -598,7 +596,9 @@ def test_sign_verify_vectors() -> None:
   596 
   597     aggnonces = fromhex_all(test_data["aggnonces"])
   598     # The aggregate of the first three elements of pnonce is at index 0
   599-    assert(aggnonces[0] == nonce_agg([pnonce[0], pnonce[1], pnonce[2]]))
   600+    assert (aggnonces[0] == nonce_agg([pnonce[0], pnonce[1], pnonce[2]]))
   601+    # The aggregate of the first and fourth elements of pnonce is at index 1
   

   ---

   ---

   

   stratospher commented at 11:24 am on July 19, 2024:

   b6d7143: nit: maybe mention context about them adding to infinity like in the PR writeup? (it wasn’t immediately obvious to me.)

   ---

   

   siv2r commented at 6:25 pm on July 19, 2024:

   Fixed
5. 
   stratospher commented at 11:28 am on July 19, 2024: contributor

   ACK 2fc3ef4.
6. bip327: minor fixes

   - An error test vector doesn’t specify the InvalidContributionError type
   - In *DeterministicSign*, use GetXonlyPubkey instead of GetPubkey
   - The key_agg_and_tweak fn doesn’t specify the return type
   - In partial_sig_verify_internal, the pubkey arg should be PlainPk
   - Remove unused enumerate() fn calls
   - In test_sign_verify, add an additional assert statement

   1c6ac0c4cf
7. remove P = None check as cpoint never returns None 0d79b5eeb5
8. siv2r force-pushed on Jul 19, 2024
9. 
   jonasnick commented at 2:19 pm on July 22, 2024: contributor

   Thanks @siv2r! Here’s a commit that update the BIP to 1.0.2: https://github.com/jonasnick/bips/commits/minor-fixes-jn/. Feel free to cherry-pick.

   We should probably return False when cpoint throws an exception when parsing P, R_s1, and R_s2. Currently, the partial_sig_verify_internal simply crashes.

   That would also be possible, but it wouldn’t have an effect on users of the API. Right now, partial_sig_verify_internal raises a ValueError that cannot be triggered by any of the public functions and that is tested by test vectors.

10. murchandamus added the label Proposed BIP modification on Jul 22, 2024
11. murchandamus added the label PR Author action required on Jul 22, 2024
12. 
    siv2r commented at 5:35 pm on July 22, 2024: contributor

    Feel free to cherry-pick.

    Thanks, I cherry-picked it! Just changed my name.

    That would also be possible, but it wouldn’t have an effect on users of the API.

    Hmm, we can avoid making this change. If we proceed, we would also need to place the get_session_key_agg_coeff function call in a try-except block. Too many try-except blocks would make the code less readable.

    I initially wanted this change because partial_sig_verify_internal was called directly (not through verify) in one of the tests. However, it should be fine since it’s just a test code.

13. jonasnick approved
14. 
    jonasnick commented at 6:14 pm on July 22, 2024: contributor

    ACK 23790580e99f57bf75b2747aa35601d7c550f263
15. 
    siv2r commented at 7:24 pm on July 22, 2024: contributor

    Please don’t merge it yet. The changelog says PartialSigVerify vector was updated. It should instead be PartialSigAgg. I’ll fix it later.
16. bip-0327: 1.0.1 -> 1.0.2

    (cherry picked from commit 4f2e6e7ffbd2fdc095ab8d59827be9da18b790be)

    26bb1d8ea3
17. siv2r force-pushed on Jul 23, 2024
18. 
    siv2r commented at 7:42 am on July 23, 2024: contributor

    Fixed the changelog to say PartialSigAgg
19. murchandamus removed the label PR Author action required on Jul 23, 2024
20. murchandamus added the label Pending acceptance on Jul 23, 2024
21. 
    murchandamus commented at 11:52 am on July 23, 2024: contributor

    @jonasnick: Could you take another look?
22. jonasnick approved
23. 
    jonasnick commented at 1:25 pm on July 23, 2024: contributor

    ACK 26bb1d8ea3e2f0f7e02e1ec37a4b70fbc0781f85
24. jonatack removed the label Pending acceptance on Jul 23, 2024
25. murchandamus merged this on Jul 23, 2024
26. murchandamus closed this on Jul 23, 2024

    ---

27. siv2r deleted the branch on Jul 24, 2024

Contributors
siv2r stratospher jonasnick murchandamus

Labels
Proposed BIP modification