Update messages for Taproot types.#2005
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2005 +/- ##
==========================================
- Coverage 91.38% 91.27% -0.12%
==========================================
Files 102 102
Lines 49767 49976 +209
Branches 49767 49976 +209
==========================================
+ Hits 45482 45618 +136
- Misses 4285 4358 +73
... and 14 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
lightning/src/ln/msgs.rs
Outdated
| /// 21 million * 10^8 * 1000 | ||
| pub(crate) const MAX_VALUE_MSAT: u64 = 21_000_000_0000_0000_000; | ||
|
|
||
| /// A public nonce used for Musig2, comprised of two ECPoints |
There was a problem hiding this comment.
I don't know if the name should be labeled MuSig2BIPPublicNonce (ideally the bip number but no one yet to the best of my knowledge) or whatever to dissociate that's the two-point nonces variant of MuSig2 we're using (in case we introduce a variant on musig2 itself in the future for optimisations whatever)
There was a problem hiding this comment.
that's actually a point I brought up in the musig2 bip itself, but for now we have solved it here by not declaring the type here, and instead using my (hopefully ephemeral) musig2 crate, which exposes it simply as PublicNonce.
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct Musig2SecretNonce(pub(crate) SecretKey, pub(crate) SecretKey); | ||
|
|
||
| /// A partial signature that also contains the Musig2 nonce its signer used |
There was a problem hiding this comment.
I think "channel-level signer" just not confused with a nested signer as even a partial signature could be generated by multiple "same-channel-side" signers. Or any notation making it clear which signer we're referring to.
| /// The signature of the channel initiator (funder) on the initial commitment transaction | ||
| pub signature: Signature, | ||
| /// The partial signature of the channel initiator (funder) | ||
| pub partial_signature_with_nonce: Option<PartialSignatureWithNonce>, |
There was a problem hiding this comment.
can be called initiator_partial_signature_with_nonce even if more verbose than spec
| /// The signature of the channel acceptor (fundee) on the initial commitment transaction | ||
| pub signature: Signature, | ||
| /// The partial signature of the channel acceptor (fundee) | ||
| pub partial_signature_with_nonce: Option<PartialSignatureWithNonce>, |
There was a problem hiding this comment.
can be called fundee_partial_signature_with_nonce even if more verbose than spec
| /// Signatures on the HTLC transactions | ||
| pub htlc_signatures: Vec<Signature>, | ||
| /// The partial Taproot signature on the commitment transaction | ||
| pub partial_signature_with_nonce: Option<PartialSignatureWithNonce>, |
There was a problem hiding this comment.
can be called commitment_partial_signature_with_nonce even if more verbose than spec
There was a problem hiding this comment.
hm, I disagree, because signature is not also called commitment_signature
TheBlueMatt
added
the
blocked on rust-bitcoin
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
3 times, most recently
from
62e40c9 to
dbcdfbe
Compare
|
Let's add a CI job for |
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
dbcdfbe to
363a922
Compare
lightning/src/ln/msgs.rs
Outdated
| /// A partial signature that also contains the Musig2 nonce its signer used | ||
| #[cfg(taproot)] | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct PartialSignatureWithNonce(pub(crate) musig2::types::PartialSignature, pub(crate) musig2::types::PublicNonce); |
There was a problem hiding this comment.
Do these need to be pub(crate) vs just pub?
There was a problem hiding this comment.
hm, perhaps at some point in the future we'll want to expose the inner details externally, but for now I'd leave it at pub(crate)
There was a problem hiding this comment.
I mean it doesn't matter because its behind a cfg flag anyway, but in general I think we should/are move to making all messages public so that users can build their own complete custom messages.
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
363a922 to
2f6c321
Compare
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
4 times, most recently
from
922799e to
7633e04
Compare
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
7633e04 to
3bd93db
Compare
TheBlueMatt
removed
the
blocked on rust-bitcoin
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
3bd93db to
8e800d1
Compare
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
8e800d1 to
b6b7b91
Compare
lightning/src/util/ser.rs
Outdated
| impl Readable for musig2::types::PublicNonce { | ||
| fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> { | ||
| let buf: [u8; PUBLIC_KEY_SIZE * 2] = Readable::read(r)?; | ||
| let nonce = musig2::types::PublicNonce::from_slice(&buf).unwrap(); |
There was a problem hiding this comment.
Oops, somehow I missed this - we can't unwrap during deserialization, you have a few of them. We have to map to an Err and return that.
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
b6b7b91 to
c6df720
Compare
arik-so
force-pushed
the
2023-01-taproot-message-types
branch
from
c6df720 to
bc97b82
Compare
5 participants
This PR creates Taproot-specific types, such as MuSig2 nonces as well as
PartialSignatureWithNonce. It also updates the channel opening and regular channel operation mechanism related messages to support these types using TLVs.This PR does not introduce Taproot type support for channel-close-related messages.