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

multi: add ability to specify local nonces for musig2 signer rpc, add itest for remote signer taproot chans #7994

Merged

Conversation

Roasbeef
Copy link
Member

In this PR, we update the set of protos to accept the local secret
nonces over RPC. This is actually a 97 byte value, as it includes the
two 32 byte nonces, as well as the 33 byte value of the public key of
the signer.

This is needed in order to be able to open taproot channels over the RPC
interface.

Along the way, we modify the musig2 interfaces to instead use an
explicit value for the local nonces. Before this commit, we used the
functional option, but we want to also support specifying this value
over RPC for the remote signer. The functional option pattern is opaque,
so we can't get the nonce value we need. To get around this, we'll just
make this an explicit pointer, then map this to the functional option at
the very last moment.

Finally, we add a test case to ensure that taproot chans work w/ the
remote signer.

@Roasbeef Roasbeef added rpc Related to the RPC interface signrpc taproot musig2 labels Sep 16, 2023
@Roasbeef Roasbeef added this to the v0.17.0 milestone Sep 16, 2023
@saubyk
Copy link
Collaborator

saubyk commented Sep 18, 2023

This is needed in order to be able to open taproot channels over the RPC interface.

Can you please expound on this a bit more? Not clear what RPC interface you're referring to.

@Roasbeef
Copy link
Member Author

Can you please expound on this a bit more? Not clear what RPC interface you're referring to.

This is needed to be able to open taproot channels with a remote signer configuration.

@Roasbeef Roasbeef requested review from ProofOfKeags and Crypt-iQ and removed request for yyforyongyu September 18, 2023 18:11
Copy link
Collaborator

@ProofOfKeags ProofOfKeags 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. Added some rewording suggestions to comments and had one question about the specialized pub nonce derivation logic.

lnrpc/signrpc/signer.proto Outdated Show resolved Hide resolved
lnrpc/signrpc/signer.swagger.json Outdated Show resolved Hide resolved
lnrpc/signrpc/signer.proto Show resolved Hide resolved
Comment on lines +889 to +898
var r1, r2 btcec.JacobianPoint
btcec.ScalarBaseMultNonConst(&k1Mod, &r1)
btcec.ScalarBaseMultNonConst(&k2Mod, &r2)

// Next, we'll convert the key in jacobian format to a normal public
// key expressed in affine coordinates.
r1.ToAffine()
r2.ToAffine()
r1Pub := btcec.NewPublicKey(&r1.X, &r1.Y)
r2Pub := btcec.NewPublicKey(&r2.X, &r2.Y)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'm somewhat surprised that the singular version of this isn't its own function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just mapping a secret nonce to a public nonce? It's in the musig2 package, but unadvertised. Reason is that it tries to force a "safe" API by always getting in strong randomness, then applying all the musig2-bip specific item like: appending the public key to the secret nonces itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could've exported it, but then would've had to commit to that addition to the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I guess what I see here is a function that takes a full secnonce encoding, breaks it into two secret u256's and converts them to ECPoints and then reassembling them into a pubnonce encoding. Intuitively I'd expect the u256 -> ECPoint conversion to be an already existing function that got reused here. It's not a huge deal since it's probs a 3 line fn and this method is still small enough to reason about.

lnrpc/signrpc/signer_server.go Show resolved Hide resolved
lnrpc/signrpc/signer_server.go Outdated Show resolved Hide resolved
lnrpc/signrpc/signer.pb.go Outdated Show resolved Hide resolved
lnrpc/signrpc/signer.pb.go Outdated Show resolved Hide resolved
lnrpc/signrpc/signer.pb.go Show resolved Hide resolved
…nonces

In this commit, we modify the musig2 interfaces to instead use an
explicit value for the local nonces. Before this commit, we used the
functional option, but we want to also support specifying this value
over RPC for the remote signer. The functional option pattern is opaque,
so we can't get the nonce value we need. To get around this, we'll just
make this an explicit pointer, then map this to the functional option at
the very last moment.
In this commit, we update the set of protos to accept the local secret
nonces over RPC. This is actually a 97 byte value, as it includes the
two 32 byte nonces, as well as the 33 byte value of the public key of
the signer.

This is needed in order to be able to open taproot channels over the RPC
interface.
This ensures that taproot chans can be used with the remote signer
configuration.
@Roasbeef Roasbeef merged commit 7412482 into lightningnetwork:master Sep 18, 2023
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
musig2 rpc Related to the RPC interface signrpc taproot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants