bip327: minor fixes #1647

pull siv2r wants to merge 3 commits into bitcoin:master from siv2r:minor-fixes changing 4 files +20 −15
  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

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-11-06 04:10 UTC

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