Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bip327: minor fixes #1647

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

bip327: minor fixes #1647

wants to merge 3 commits into from
+20 −15

Conversation

siv2r
Copy link
Contributor

@siv2r siv2r commented Jul 17, 2024 •
edited
Loading

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

@siv2r
Copy link
Contributor Author

siv2r commented Jul 18, 2024 •
edited
Loading

Additionally, I removed the following P is None check, since cpoint never returns None.

bips/bip-0327/reference.py

Lines 442 to 444 in 812907c

P = cpoint(pk)
if P is None:
return False

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.

stratospher
stratospher reviewed Jul 19, 2024
View reviewed changes
Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2fc3ef4.

bip-0327/vectors/sig_agg_vectors.json
@@ -143,7 +143,8 @@
],
"error": {
"type": "invalid_contribution",
"signer": 1
"signer": 1,
"contrib": "psig"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@siv2r siv2r Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes. I missed it. Thanks!
Fixed now.

bip-0327/reference.py Outdated
@@ -598,7 +596,9 @@ def test_sign_verify_vectors() -> None:

aggnonces = fromhex_all(test_data["aggnonces"])
# The aggregate of the first three elements of pnonce is at index 0
assert(aggnonces[0] == nonce_agg([pnonce[0], pnonce[1], pnonce[2]]))
assert (aggnonces[0] == nonce_agg([pnonce[0], pnonce[1], pnonce[2]]))
# The aggregate of the first and fourth elements of pnonce is at index 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@siv2r
bip327: minor fixes
1c6ac0c 
- 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
@siv2r
remove P = None check as cpoint never returns None
0d79b5e
jonasnick
jonasnick reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@murchandamus murchandamus added Proposed BIP modification PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author labels Jul 22, 2024
@jonasnick @siv2r
bip-0327: 1.0.1 -> 1.0.2
2379058 
(cherry picked from commit 4f2e6e7)
@siv2r
Copy link
Contributor Author

siv2r commented Jul 22, 2024 •
edited
Loading

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.

jonasnick
jonasnick approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2379058

@siv2r
Copy link
Contributor Author

siv2r commented Jul 22, 2024

Please don't merge it yet. The changelog says PartialSigVerify vector was updated. It should instead be PartialSigAgg. I'll fix it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratospher stratospher stratospher left review comments

@jonasnick jonasnick jonasnick approved these changes

Assignees
No one assigned
Labels
PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author Proposed BIP modification
Projects
None yet
Milestone
No milestone
4 participants
@siv2r @jonasnick @stratospher @murchandamus