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.

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

BIP-0340: Miscellaneous fixups #206

Merged
merged 3 commits into from
Jul 22, 2020
Merged

BIP-0340: Miscellaneous fixups #206

merged 3 commits into from
Jul 22, 2020

Conversation

jonasnick
Copy link

This is a result of Alan Szepieniec feedback and @sipa's responses.

- key prefixing means prefixing the message
- array indexing starts with 0
- 'Gennaro' is spelled with two n's
- has_even_y definition takes P as argument

Thanks to Alan Szepieniec for pointing out these issues.
Jacobi symbol can be confusing because it may suggest that the modulus is
composite.

Thanks to Alan Szepieniec for pointing out this issue.
@jonasnick jonasnick changed the title Miscellaneous fixups BIP-0340: Miscellaneous fixups Jul 18, 2020
Copy link

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Concept ACK

@@ -97,6 +97,8 @@ This proposal suggests to include the tag by prefixing the hashed data with ''SH

'''Final scheme''' As a result, our final scheme ends up using public key ''pk'' which is the X coordinate of a point ''P'' on the curve whose Y coordinate is even and signatures ''(r,s)'' where ''r'' is the X coordinate of a point ''R'' whose Y coordinate is a square. The signature satisfies ''s⋅G = R + tagged_hash(r || pk || m)⋅P''.

We note that adapting this specification to other elliptic curves is not straightforward and can result in an insecure scheme<ref>Among other pitfalls, using the specification with a curve whose order is not close to the output of the nonce derivation is insecure.</ref>.

Choose a reason for hiding this comment

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

Suggested change
We note that adapting this specification to other elliptic curves is not straightforward and can result in an insecure scheme<ref>Among other pitfalls, using the specification with a curve whose order is not close to the output of the nonce derivation is insecure.</ref>.
We note that adapting this specification to other elliptic curves is not straightforward and can result in an insecure scheme<ref>Among other pitfalls, using the specification with a curve whose order is not close to the size of the range of the nonce derivation function is insecure.</ref>.

I'd move it somewhere to the top of the Specification section to make it more prominent. I guess it fits after the introductory sentence there.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed your nit. I had the sentence at the top of the Spec section before, but the Design section seems slightly more fitting.

Choose a reason for hiding this comment

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

Do you think it fits better there? I believe the opposite is true, he sentence specifically talks about the specification. (No pun intended.) I somehow fear people could miss it because they simply ignore the Design section. But I guess for every position in the BIP, you'll find some person who'll ignore it.

Copy link
Author

Choose a reason for hiding this comment

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

moved it to the spec

@sipa
Copy link
Owner

sipa commented Jul 20, 2020

Concept ACK. As these BIPs are already published, PR it directly to https://github.com/bitcoin/bips ?

@jonasnick
Copy link
Author

I was thinking that there may be more changes to the BIP soon (#195 #204 #205) and it would be simpler to merge them here first and then PR them against upstream all at once. At least this is what we did last time.

@sipa
Copy link
Owner

sipa commented Jul 20, 2020

@jonasnick Sounds good.

@sipa
Copy link
Owner

sipa commented Jul 20, 2020

Also ping @aszepieniec.

Copy link

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK

@sipa
Copy link
Owner

sipa commented Jul 22, 2020

ACK

@sipa sipa merged commit e331aad into sipa:bip-taproot Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants