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

Properly handle BIP-340 Schnorr signatures with or without BIP-341 taproot tweaked keys #94

Merged
merged 42 commits into from
Nov 15, 2024

Conversation

xoloki
Copy link
Collaborator

@xoloki xoloki commented Oct 17, 2024

In the early days of sBTC-Alpha, we had no idea how Taproot worked. I started by implementing BIP-340, only to discover that I really needed to implement BIP-341 as well to get tweaked public keys. At that point there was no way to do BIP-340 alone.

We discovered recently that TapScript public key spends need to use BIP-340 without BIP-341, since the whole point of tweaking the keys is to prevent hidden script spends, and we're already inside the script at that point. So this change enables that, from the low level Signer/Aggregator code all the way up to the network packets and state machines.

@xoloki xoloki requested a review from djordon October 18, 2024 05:11
@xoloki xoloki marked this pull request as ready for review October 18, 2024 05:11
Copy link

@djordon djordon left a comment

Choose a reason for hiding this comment

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

I still have more to look at, but figured I'd submit what I've already seen. So far, looks good, these are all minor suggestions.

src/state_machine/coordinator/frost.rs Outdated Show resolved Hide resolved
src/net.rs Outdated Show resolved Hide resolved
src/state_machine/coordinator/frost.rs Show resolved Hide resolved
src/compute.rs Outdated Show resolved Hide resolved
src/state_machine/signer/mod.rs Outdated Show resolved Hide resolved
src/v1.rs Outdated Show resolved Hide resolved
Copy link

@djordon djordon left a comment

Choose a reason for hiding this comment

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

This largely looks good. But I think we should add a bit more documentation for the sign functions so that we can easily verify that things are computed correctly. I wasn't entirely sure which procedures I should be following for some of the sign functions, so I also want this so that I can give a more meaningful review.

I know we have the tests but I still think we should have the documentation, plus tests can be "faulty". Not saying that these are, they seem fine.

src/compute.rs Show resolved Hide resolved
src/state_machine/coordinator/mod.rs Outdated Show resolved Hide resolved
src/state_machine/coordinator/mod.rs Show resolved Hide resolved
src/v1.rs Show resolved Hide resolved
@xoloki xoloki changed the title Allow BIP-340 Schnorr signatures without BIP-341 taproot tweaked keys Properly handle BIP-340 Schnorr signatures with or without BIP-341 taproot tweaked keys Nov 4, 2024
Copy link

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good.

As a follow-up we should do #24. We can pull in the setup code from the sBTC repo to make that all easy. That way we can be very confident that this works.

src/v1.rs Show resolved Hide resolved
src/v2.rs Outdated Show resolved Hide resolved
@xoloki xoloki merged commit ebd7d77 into main Nov 15, 2024
5 checks passed
@xoloki xoloki deleted the simple-signature-type branch November 15, 2024 16:36
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.

2 participants