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

Add high-level types for musig2 primitives provided by secp256k1-kmp #107

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

sstone
Copy link
Member

@sstone sstone commented Dec 14, 2023

This PR is based on #108 because of ACINQ/secp256k1-kmp#93

@t-bast
Copy link
Member

t-bast commented Jan 18, 2024

Why did you keep the prototype musig2 implementation (in the test folders)? Shouldn't we get rid of it entirely, we want to test the "real" implementation and have no use for the prototype one anymore?

It's dangerous to keep it around, as some of the tests in Musig2TestsCommon.kt only test that prototype implementation instead of testing the "real" code.

Or am I too early reviewing this, and it's not ready yet?

@sstone
Copy link
Member Author

sstone commented Jan 18, 2024

I kept it for reference but it's not supposed to be used in Musig2TestsCommon.kt I'll fix that.
I don't think it's too soon to start reviewing this PR as the API in ACINQ/secp256k1-kmp#93 should not change much (though we still have a few weeks before it gets merged).

* Add high-level helpers for using Musig2 with Taproot

When using Musig2 for a taproot key path, we can provide simpler helper
functions to collaboratively build a shared signature for the spending
transaction. Those helper functions hide the low-level details of using
an opaque key aggregation cache or signing session. This comes with a
small performance penalty, as we recompute the key aggregation cache.

We also document the exposed APIs, import more tests from the official
test vectors, and make APIs safe: they should never throw exceptions,
except when invalid public keys are provided as inputs (which should be
verified by the caller beforehand).

* Move taproot signing helpers to `Transaction.kt`

This is more consistent with the existing `signInput` helpers.
@t-bast t-bast marked this pull request as ready for review February 1, 2024 08:27
@@ -12,7 +12,7 @@ plugins {
val currentOs = org.gradle.internal.os.OperatingSystem.current()

group = "fr.acinq.bitcoin"
version = "0.17.0-SNAPSHOT"
version = "0.17.0-MUSIG2-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

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

Should we merge with that temporary version since we don't expect to do any other work on bitcoin-kmp that would require a quick release? And then once we can merge ACINQ/secp256k1-kmp#93 we'll be able to make clean releases and fix the versions?

@sstone sstone merged commit 6cf04b0 into master Feb 14, 2024
4 checks passed
@sstone sstone deleted the snapshot/musig2 branch February 14, 2024 14:27
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