signrpc: switch to version v1.0 of musig2

[Roasbeef]

So the 1.0 version of musig was recently tagged! 🥳

Today in lnd, via btcsuite/btcec/v2 we expose musig 0.4.0 which was the last version before the larger breaking change of using compressed keys as input to key aggregation. For the most part the set of changes weren't too invasive, and there're even more test vectors now which is great.

For the 0.16 release (potentially sooner since it's an isolated change) we want to expose (for now) both version 1.0 and 0.4.0. In an ideal world, we'd be able to just import two versions of the same module, but Go doesn't really allow that.

Instead, I think we can do something like:

• Make a new internal/musig2 package and move the existing code in btcec/schnorr/musig2 there.
  • Making it an internal package means it can't be used outside of lnd for anything (no public import).
• Update the btcec package to target musig 1.0 and tag a new go module version of that.
• Update lnd to point to the new Go module.
• Add a new argument in the API to expose a version (experimental and final is prob all we need for now)
• ???
• profit

I think the only open question is: should the default version be 0.4.0 or 1.0? Restating the question, we can either break the API for existing users (default is 1.0) or keep it and carry along the two versions for some indeterminate period of time. I vote for making 1.0 the default, and leaving it to any projects that were using the existing API to make sure they're passing the versions around properly.

[guggero]

My idea for approaching this was:

• Instead of handling this in lnd, we instead add a version to the MuSig2 API (btcec/schnorr/musig2) itself.
• By default, the version would always be the latest one implemented (e.g. MUSIG2_VERSION_1_0) or it would be tied to the Go module's version, but it could be changed when creating a context (e.g. WithVersion(MUSIG2_VERSION_0_4).
• The parts that are affected by the version changes have switch statements on the version and implement the logic according to the context's version.
• The version flag is stored in the context and exposed all the way up to lnd and its RPC servers.

Pros

• Minimal change required on the API and RPC level.
• No Go module headaches (I really almost feel pre-PTSD by only thinking about doing any kind of internal/musig2 module magic. If we decide to do such a thing, we'll need to do the same in all projects that want to be backward compatible, e.g. Pool, since we also pull in the go module and don't just use the RPC alone)!
• Differences between the MuSig2 versions are easily visible (just look for the switch statements)

Cons

• More complex implementation in the btcec/schnorr/musig2 package, might even need to version the test vectors.
• Implicit default version. But we could tie the btcec/schnorr/musig2 module's version to the default version we choose when creating a context (which can always be overwritten with WithVersion())

To gauge the complexity of implementing both versions in the same code base, I took a quick look at the changelog and tried to categorize the changes necessary (only looking at 0.4.0...1.0.0):

Breaking changes (need explicit code path):

• 0.8.0 (2022-08-26): Switch from X-only to plain public key inputs. This requires updating a large portion of the test vectors. (https://github.com/jonasnick/bips/pull/37)
• 0.7.0 (2022-07-31): Change NonceGen such that output when message is not present is different from when message is present but has length 0. (https://github.com/jonasnick/bips/pull/24)
• 0.6.0 (2022-07-31): Allow variable length messages, change order of arguments and serialization of the message in the NonceGen hash function, and add test vectors (https://github.com/jonasnick/bips/pull/24, same as for 0.7.0)

Looking at those breaking changes it looks to me (this part I'm not familiar enough with the API to judge, so please correct me if I'm wrong) like they only require a few small-ish changes in isolated areas (pubkey handling and empty message handling).

The rest of the changes can be applied to the whole codebase (e.g. renaming things) or don't affect the code at all (example code functions were renamed or refactored or text was re-written to be more precise). Most of the test vectors also seem to apply to all versions, only those with specific public keys or message lengths might need to be versioned.

Non-breaking changes (either formatting, text or test vector changes only, or additional/optional features):

• 1.0.0 (2022-10-03): Submit draft BIP to the BIPs repository (https://github.com/jonasnick/bips/pull/73)
• 0.8.6 (2022-09-15): Clarify that implementations do not need to support every features and add a test vector for signing with a tweaked key (https://github.com/jonasnick/bips/pull/73)
• 0.8.5 (2022-09-05): Rename some functions to improve clarity. (https://github.com/jonasnick/bips/pull/50)
• 0.8.4 (2022-09-02): Make naming of nonce variants R in specification and reference code easier to read and more consistent. (https://github.com/jonasnick/bips/pull/64)
• 0.8.3 (2022-09-01): Overwrite secnonce in sign reference implementation to help prevent accidental reuse and add test vector for invalid secnonce. (https://github.com/jonasnick/bips/pull/60)
• 0.8.2 (2022-08-30): Fix KeySort input length and add test vectors (https://github.com/jonasnick/bips/pull/61)
• 0.8.1 (2022-08-26): Add DeterministicSign algorithm (https://github.com/jonasnick/bips/pull/11)
• 0.7.2 (2022-08-17): Add NonceGen and Sign/PartialSigVerify test vectors for messages longer than 32 bytes. (https://github.com/jonasnick/bips/pull/52)
• 0.7.1 (2022-08-10): Extract test vectors into separate JSON file. (https://github.com/jonasnick/bips/pull/33)
• 0.5.2 (2022-06-26): Fix aggpk in NonceGen test vectors. (https://github.com/jonasnick/bips/pull/30)
• 0.5.1 (2022-06-22): Rename "ordinary" tweaking to "plain" tweaking. (https://github.com/jonasnick/bips/pull/29)
• 0.5.0 (2022-06-21): Separate ApplyTweak from KeyAgg and introduce KeyGen Context. (https://github.com/jonasnick/bips/pull/15)

[sputn1ck]

What I like about your solution @guggero is that it allows the musig2 codebase to be adaptible to future musig2 (e.g. breaking 1.X)changes.

However I personally don't think that we should encourage keeping the 0.4 version for more than a couple of months and should rather quickly move to the 1.0.0 standard that is universally supported between musig2 implementations. (Does 0.4 have some vulnerabilities?)

Is the internal/musig2 approach really modules hell? Wouldn't it look like this:

import (
  musig2 "github.com/btcsuite/btcd/btcec/v2/schnorr/musig2"
  musig204 "github.com/lightningnetwork/lnd/internal/musig2"
)

func DoStuff(Musig2Version version) {
  switch version {
    case MUSIG2_VERSION_0_4:
       musig204.DoStuff()
    case MUSIG2_VERSION_1_0:
       musig2.DoStuff()
  }
}

[guggero]

Is the internal/musig2 approach really modules hell? Wouldn't it look like this:

Yeah, I agree, it should be pretty simple. I was worried because we use the musig2 code in Pool as well. Not just over lnd's RPC but directly as a library. So there it would've become a bit tricky. But it turned out (after I investigated further) that we only pull in a few constants from the library that are version independent. So I now also think that it should work in a way similar to how you wrote the code above.

[Roasbeef]

Ok a new btcec tag has been pushed: https://github.com/btcsuite/btcd/releases/tag/btcec%2Fv2.3.0

@guggero will take over the signrpc updates.