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

BIP 340 & 341: use consistent definition of lift_x #1355

Merged
merged 2 commits into from
Aug 25, 2022
Merged

BIP 340 & 341: use consistent definition of lift_x #1355

merged 2 commits into from
Aug 25, 2022

Conversation

jonasnick
Copy link
Contributor

@jonasnick jonasnick commented Aug 16, 2022 •
edited
Loading

As pointed out by @Randy808 here.

CC @sipa @ajtowns

@jonasnick
Copy link
Contributor Author

I noticed that this PR alone does not fix the problem: lift_x has different definitions in the BIP-340 mediawiki specification and the reference code. The first commit in this PR makes taproot_tweak_pubkey correct with respect to lift_x in the mediawiki spec but incorrect wrt the reference code. I think the best option is to harmonize both definitions lift_x and added a commit that changes lift_x in the reference code.

CC @real-or-random

@jonasnick jonasnick changed the title BIP 341: add missing conversion from bytes to int BIP 340 & 341: use consistent definition of lift_x Aug 19, 2022
@real-or-random
Copy link
Contributor

tACK

Should we add a Changelog that tracks (non-trivial) changes? I think it won't hurt but I'm not convinced that it's worth the effort. The git log may be good enough to be honest.
https://github.com/bitcoin/bips/commits/master?before=64aba767e2d422b5d471509acc340750432613ae+35&branch=master&path%5B%5D=bip-0340.mediawiki&qualified_name=refs%2Fheads%2Fmaster
We'd need to decide on when to start it, maybe pragmatically after 3b1fb96?

As pointed out by @Randy808 here.
wrong link target?

@jonasnick
Copy link
Contributor Author

Sorry, fixed the link.

@jonasnick
Copy link
Contributor Author

Added a changelog akin to sipa#221.

@jonasnick
BIP 340: fix function signature of lift_x in reference code
3998dbb 
bip-0340.mediawiki defines lift_x as taking an integer argument. This commit
changes the argument of lift_x in the reference code to be identical to the
specification. Previously it took a byte array.
@sipa
Copy link
Member

sipa commented Aug 23, 2022

ACK 3998dbb

real-or-random
real-or-random approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@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 3998dbb

@real-or-random
Copy link
Contributor

Ok let's get this merged here now that we have the ACKs and then we can batch-PR a few more changes here later (which are currently only PRed to @sipa's fork).

@kallewoof kallewoof merged commit 52f68fe into bitcoin:master Aug 25, 2022
@jackromo888
Copy link

@jackromo888
Copy link

52f68fecd8ec9604672e26392468e7e7edf25a5ehttps://github.com/bitcoin/bips/commit/52f68fecd8ec9604672e26392468e7e7edf25a5e

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

@haleemshal haleemshal haleemshal approved these changes

@real-or-random real-or-random real-or-random approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jonasnick @real-or-random @sipa @jackromo888 @haleemshal @kallewoof