Add high-level helpers for using Musig2 with Taproot #114

t-bast · 2024-01-25T14:19:16Z

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).

I removed the musig2 prototype as I don't see any valid reason to keep it: it doesn't even have the same API as the official implementation anymore so it's rather confusing, and can be found in the git history if necessary.

This is a PR targeting #107

src/commonMain/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2.kt

sstone

I'm not convinced that it makes the code that uses musig2 simpler or more compact. For now, this code is encapsulated in specific swap-in protocol objects (for both lightning-kmp and eclair), this is where we can hide musig2 complexity from the rest of the codebase.
Also, the really tricky part imho (same with singing regular transaction inputs) is how to properly set up a musig2 signing session and this requires users to pass the right transaction, index, nonces etc... and it can't really be made simpler.

sstone · 2024-01-25T16:34:27Z

src/commonMain/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2.kt

-        public fun add(pubkeys: List<PublicKey>, cache: KeyAggCache?): Pair<XonlyPublicKey, KeyAggCache> {
-            val localCache = cache?.data?.toByteArray() ?: ByteArray(Secp256k1.MUSIG2_PUBLIC_KEYAGG_CACHE_SIZE)
-            val aggkey = Secp256k1.musigPubkeyAgg(pubkeys.map { it.value.toByteArray() }.toTypedArray(), localCache)
+        public fun create(publicKeys: List<PublicKey>): Pair<XonlyPublicKey, KeyAggCache> {


This means that we cannot add new public keys to an existing key aggregation cache, which was possible before.

Yes, it was possible, but not used anywhere. I don't see any scenario where that is useful: why create a key aggregation cache before you have all public keys, since you need to have all of them later? A general rule of library design is to start simple and only introduce more complexity when it's necessary, and here we were really doing the opposite...

... A general rule of library design is to start simple and only introduce more complexity when it's necessary, and here we were really doing the opposite...

No, we keep things simple by exposing the musig2 API as-is and not second-guessing how it could be improved.
We've had very few bugs in this library, and always because we (it's a royal "we" here) were trying to be "smarter" than bitcoin-core.

No, we keep things simple by exposing the musig2 API as-is and not second-guessing how it could be improved.
We've had very few bugs in this library, and always because we (it's a royal "we" here) were trying to be "smarter" than bitcoin-core.

I disagree: the musig2 API exposed in secp256k1 needs to be much more generic than what we're exposing, and doesn't want to compromise on performance. We have a more restricted set of use-cases, which allow us to provide higher-level APIs that are simpler to use, harder to misuse. Without those higher-level APIs, it is really hard and footgunny to use musig2.

src/commonMain/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2.kt

sstone · 2024-01-25T16:49:04Z

src/commonTest/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2TestsCommon.kt

+        val publicKeyX = uncompressedPublicKey.drop(1).take(32).reversed().toByteArray()
+        val publicKeyY = uncompressedPublicKey.takeLast(32).reversed().toByteArray()
+        val magic = Hex.decode("220EDCF1")
+        return SecretNonce(magic + serialized.take(64) + publicKeyX + publicKeyY)


This will likely break when we update secp256k1 through secp256k1-kmp. If we really want to add these low-level checks (not sure we need to), they belong in secp256k1-kmp.

You mean that secp256k1 may break the format they use for SecretNonces? If so yes, we'll need to update those tests, but this is only used for tests, to make sure we're able to validate the official test vectors. I like validating the test vectors at the bitcoin-kmp layer rather than in secp256k1-kmp, because they could pass in secp256k1-kmp, but bitcoin-kmp may use incorrectly the functions exposed, and then we wouldn't have tests to catch it.

Verifying that the public nonce is correct should be enough here.

It's true for the secret_nonce_gen test vectors, but without that function we cannot run the signing test vectors, nor the aggregation test vectors, which are pretty important test vectors to have!

It should be possible to generate the secret nonces used in these tests (though it does not seem to be documented), I'll investigate.

t-bast · 2024-01-25T17:14:27Z

I'm not convinced that it makes the code that uses musig2 simpler or more compact. For now, this code is encapsulated in specific swap-in protocol objects (for both lightning-kmp and eclair), this is where we can hide musig2 complexity from the rest of the codebase.

I really don't understand how you can't be convinced. We're now exposing an API that makes it trivial to spend any output that uses musig2 for its key path with any script tree, without having to know or care about low-level musig2 details (key aggregation cache and opaque session data). That's exactly the abstraction that every use-case of musig2 needs? So instead of duplicating this code in lightning-kmp and eclair, we now have a single abstraction in bitcoin-kmp that we can easily use for both swap-in-potentiam, taproot channels and PTLCs.

The musig2 complexity should be hidden in bitcoin-kmp as much as possible, otherwise we're not providing any useful abstraction and multiple users of bitcoin-kmp will have to write the same code many times.

Also, the really tricky part imho (same with singing regular transaction inputs) is how to properly set up a musig2 signing session and this requires users to pass the right transaction, index, nonces etc... and it can't really be made simpler.

I don't understand what you mean: a signer only has to generate his secret nonce, receive public nonces from other participants, and then the existing functions make it trivial to spend the utxos. I believe the swap-in-potentiam unit test showcases that pretty well?

src/commonMain/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2.kt

sstone · 2024-01-29T10:49:24Z

src/commonMain/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2.kt

+        inputIndex: Int,
+        inputs: List<TxOut>,
+        publicKeys: List<PublicKey>,
+        publicNonces: List<IndividualNonce>,


This means that all public nonces and public keys must be available. This may not be practical, users could have discarded public nonces and just have kept the aggregated nonce. This is what we currently do in ACINQ/lightning-kmp#563.

This is what we currently do in ACINQ/lightning-kmp#563.

But it's much simpler if eclair/lightning-kmp don't have to care about nonce aggregation at all, isn't it? It's simpler to just store the local and remote nonces and then pass them to the signing helper (and those nonces are most likely already stored internally as part of the interactive-tx types). This gets rid of one "internal detail" of musig2 that higher-level applications don't even need to know about, that's part of encapsulating the complexity inside bitcoin-kmp.

I have updated lightning-kmp to simply keep the local and remote nonce instead of aggregating them in ACINQ/lightning-kmp#591 and it works well.

We use the musig2 helpers exposed by ACINQ/bitcoin-kmp#114 to simplify the swap-in protocol and hide all of the musig2 internal details (key aggregation cache, control block, internal taproot key, opaque session object, nonce aggregation). The code is simpler to reason about and signing is more similar to signing normal single-sig inputs.

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).

This is more consistent with the existing `signInput` helpers.

sstone

I updated and tested lightning-kmp/eclair/phoenix with these changes and everything works, and it seems that it makes the code in lightning-kmp more readable so we can go with this. We'll end up with an API that is a bit more restrictive than the raw musig2 API but we can always revisit this later.

We use the musig2 helpers exposed by ACINQ/bitcoin-kmp#114 to simplify the swap-in protocol and hide all of the musig2 internal details (key aggregation cache, control block, internal taproot key, opaque session object, nonce aggregation). The code is simpler to reason about and signing is more similar to signing normal single-sig inputs.

* Move swap-in related methods into their own class * Add musig2-based swap-in protocol * Use different user keys for the common and refund paths This allows us to easily rotate swap-in addresses and generate a single generic taproot descriptor (for bitcoin core 26 and newer) that can be used to recover swap-in funds once the refund delay has passed, assuming that: - user and server keys are static - user refund keys follow BIP derivation * Add a musig2 secret nonce field to local/remote musing2 swap-in classes It makes the code cleaner and we get rid of the secret nonces map. These nonces are replaced with dummy values whenever this classes are serialized, which is safe since they're never reused for signing txs. * Rework TxComplete to use implicit ordering for musig2 nonces Instead of sending an explicit serialId -> nonce map, we send a list of public nonces ordered by serial id. This matches how signatures are sent in TxSignatures. * Address review comments - add a pubkey script to the SharedInput() class (we don't need the full TxOut which we can recreate) - remove aggregate nonce check ins FullySignedTx: code already handles transactions that are not properly signed - generate musig2 nonces when we send TxAddInput * Use musig2 helpers to simplify swap-in protocol (#592) We use the musig2 helpers exposed by ACINQ/bitcoin-kmp#114 to simplify the swap-in protocol and hide all of the musig2 internal details (key aggregation cache, control block, internal taproot key, opaque session object, nonce aggregation). The code is simpler to reason about and signing is more similar to signing normal single-sig inputs. * Rework recovery procedure The current recovery process needed to be updated to derive the correct master priv key from the seed by specifying our custom BIP32 path (m/52h/0h/2h/0) when we create the wallet. We also export 2 descriptor methods: one to get the private swap-in wallet descriptor, which can be used as-is, and the other to get the public swap-in wallet descriptor, which can be used to create a watch-only wallet to monitor swap-in funds and to recovery funds using our recovery procedure. Both descriptor use the refund master key, and not the master key itself because we use hardened paths to derive the refund key, which means that it is not possible to compute the refund master public key from the master public: importing the descriptor would fail. --------- Co-authored-by: Bastien Teinturier <[email protected]>

t-bast requested a review from sstone January 25, 2024 14:19

t-bast commented Jan 25, 2024

View reviewed changes

src/commonMain/kotlin/fr/acinq/bitcoin/crypto/musig2/Musig2.kt Outdated Show resolved Hide resolved

sstone reviewed Jan 25, 2024

View reviewed changes

sstone reviewed Jan 29, 2024

View reviewed changes

t-bast force-pushed the musig2-abstractions branch from 355959e to 4ead0d7 Compare January 30, 2024 06:23

t-bast mentioned this pull request Jan 31, 2024

Use musig2 helpers for swap-in-potentiam using taproot ACINQ/lightning-kmp#591

Closed

t-bast force-pushed the musig2-abstractions branch 2 times, most recently from b377703 to 8e0194f Compare January 31, 2024 15:10

sstone force-pushed the snapshot/musig2 branch from 09ff67a to eb10a83 Compare January 31, 2024 16:14

t-bast added 2 commits February 1, 2024 07:35

Move taproot signing helpers to Transaction.kt

b7ca44e

This is more consistent with the existing `signInput` helpers.

t-bast force-pushed the musig2-abstractions branch from 8e0194f to b7ca44e Compare February 1, 2024 06:36

sstone approved these changes Feb 1, 2024

View reviewed changes

t-bast merged commit 73b455b into snapshot/musig2 Feb 1, 2024
2 checks passed

t-bast deleted the musig2-abstractions branch February 1, 2024 08:27

t-bast mentioned this pull request Feb 2, 2024

Use musig2 helpers to simplify swap-in protocol ACINQ/lightning-kmp#592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add high-level helpers for using Musig2 with Taproot #114

Add high-level helpers for using Musig2 with Taproot #114

t-bast commented Jan 25, 2024

sstone left a comment

sstone Jan 25, 2024

t-bast Jan 25, 2024

sstone Jan 25, 2024 •

edited

Loading

t-bast Jan 25, 2024

sstone Jan 25, 2024

t-bast Jan 25, 2024

sstone Jan 25, 2024

t-bast Jan 25, 2024

sstone Jan 29, 2024

t-bast commented Jan 25, 2024

sstone Jan 29, 2024

t-bast Jan 29, 2024 •

edited

Loading

t-bast Feb 1, 2024

sstone left a comment •

edited

Loading

Add high-level helpers for using Musig2 with Taproot #114

Add high-level helpers for using Musig2 with Taproot #114

Conversation

t-bast commented Jan 25, 2024

sstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstone Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast commented Jan 25, 2024

Choose a reason for hiding this comment

t-bast Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstone left a comment • edited Loading

Choose a reason for hiding this comment

sstone Jan 25, 2024 •

edited

Loading

t-bast Jan 29, 2024 •

edited

Loading

sstone left a comment •

edited

Loading