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

Pre-work for BOLT 12 invoices #1927

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 51 additions & 9 deletions lightning/src/ln/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ use core::marker::PhantomData;
use bitcoin::bech32;
use bitcoin::bech32::{Base32Len, FromBase32, ToBase32, u5, WriteBase32};
use crate::ln::msgs::DecodeError;
use crate::util::ser::{Readable, Writeable, Writer};
use crate::util::ser::{Readable, WithoutLength, Writeable, Writer};

mod sealed {
use crate::prelude::*;
Expand Down Expand Up @@ -159,6 +159,15 @@ mod sealed {
]);
define_context!(OfferContext, []);
define_context!(InvoiceRequestContext, []);
define_context!(Bolt12InvoiceContext, [
// Byte 0
,
// Byte 1
,
// Byte 2
BasicMPP,
]);
define_context!(BlindedHopContext, []);
// This isn't a "real" feature context, and is only used in the channel_type field in an
// `OpenChannel` message.
define_context!(ChannelTypeContext, [
Expand Down Expand Up @@ -342,7 +351,7 @@ mod sealed {
define_feature!(15, PaymentSecret, [InitContext, NodeContext, InvoiceContext],
"Feature flags for `payment_secret`.", set_payment_secret_optional, set_payment_secret_required,
supports_payment_secret, requires_payment_secret);
define_feature!(17, BasicMPP, [InitContext, NodeContext, InvoiceContext],
define_feature!(17, BasicMPP, [InitContext, NodeContext, InvoiceContext, Bolt12InvoiceContext],
"Feature flags for `basic_mpp`.", set_basic_mpp_optional, set_basic_mpp_required,
supports_basic_mpp, requires_basic_mpp);
define_feature!(19, Wumbo, [InitContext, NodeContext],
Expand All @@ -369,7 +378,7 @@ mod sealed {

#[cfg(test)]
define_feature!(123456789, UnknownFeature,
[NodeContext, ChannelContext, InvoiceContext, OfferContext, InvoiceRequestContext],
[NodeContext, ChannelContext, InvoiceContext, OfferContext, InvoiceRequestContext, Bolt12InvoiceContext, BlindedHopContext],
"Feature flags for an unknown feature used in testing.", set_unknown_feature_optional,
set_unknown_feature_required, supports_unknown_test_feature, requires_unknown_test_feature);
}
Expand Down Expand Up @@ -432,6 +441,10 @@ pub type InvoiceFeatures = Features<sealed::InvoiceContext>;
pub type OfferFeatures = Features<sealed::OfferContext>;
/// Features used within an `invoice_request`.
pub type InvoiceRequestFeatures = Features<sealed::InvoiceRequestContext>;
/// Features used within an `invoice`.
pub type Bolt12InvoiceFeatures = Features<sealed::Bolt12InvoiceContext>;
/// Features used within BOLT 4 encrypted_data_tlv and BOLT 12 blinded_payinfo
pub type BlindedHopFeatures = Features<sealed::BlindedHopContext>;

/// Features used within the channel_type field in an OpenChannel message.
///
Expand Down Expand Up @@ -719,32 +732,47 @@ impl_feature_len_prefixed_write!(InitFeatures);
impl_feature_len_prefixed_write!(ChannelFeatures);
impl_feature_len_prefixed_write!(NodeFeatures);
impl_feature_len_prefixed_write!(InvoiceFeatures);
impl_feature_len_prefixed_write!(BlindedHopFeatures);

// Some features only appear inside of TLVs, so they don't have a length prefix when serialized.
macro_rules! impl_feature_tlv_write {
($features: ident) => {
impl Writeable for $features {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
self.write_be(w)
WithoutLength(self).write(w)
}
}
impl Readable for $features {
fn read<R: io::Read>(r: &mut R) -> Result<Self, DecodeError> {
let v = io_extras::read_to_end(r)?;
Ok(Self::from_be_bytes(v))
Ok(WithoutLength::<Self>::read(r)?.0)
}
}
}
}

impl_feature_tlv_write!(ChannelTypeFeatures);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we not change this too? I guess it can wait to avoid conflicting with #1860, but we should probably avoid having two paths for this, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, noted in response to Val as well, we probably want to drop impl_feature_tlv_write and make the encoding explicit. Would require allowing encodings in impl_writeable_msg, which I think would mostly just work.

impl_feature_tlv_write!(OfferFeatures);
impl_feature_tlv_write!(InvoiceRequestFeatures);
Comment on lines -741 to -742
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are passing for me on #1926 with keeping use of the macro for these sets of features:

diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
index a3c1b7f6..335dad03 100644
--- a/lightning/src/ln/features.rs
+++ b/lightning/src/ln/features.rs
@@ -751,6 +751,8 @@ macro_rules! impl_feature_tlv_write {
 }

 impl_feature_tlv_write!(ChannelTypeFeatures);
+impl_feature_tlv_write!(OfferFeatures);
+impl_feature_tlv_write!(InvoiceRequestFeatures);

 // Some features may appear both in a TLV record and as part of a TLV subtype sequence. The latter
 // requires a length but the former does not.
@@ -872,11 +874,11 @@ mod tests {
                assert_eq!(features.flags.len(), 8);

                let mut serialized_features = Vec::new();
-               WithoutLength(&features).write(&mut serialized_features).unwrap();
+               features.write(&mut serialized_features).unwrap();
                assert_eq!(serialized_features.len(), 8);

                let deserialized_features =
-                       WithoutLength::<OfferFeatures>::read(&mut &serialized_features[..]).unwrap().0;
+                       OfferFeatures::read(&mut &serialized_features[..]).unwrap();
                assert_eq!(features, deserialized_features);
        }

diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs
index fc2b90f8..46f7b626 100644
--- a/lightning/src/offers/invoice_request.rs
+++ b/lightning/src/offers/invoice_request.rs
@@ -408,7 +408,7 @@ impl Writeable for InvoiceRequestContents {
 tlv_stream!(InvoiceRequestTlvStream, InvoiceRequestTlvStreamRef, 80..160, {
        (80, chain: ChainHash),
        (82, amount: (u64, HighZeroBytesDroppedBigSize)),
-       (84, features: (InvoiceRequestFeatures, WithoutLength)),
+       (84, features: InvoiceRequestFeatures),
        (86, quantity: (u64, HighZeroBytesDroppedBigSize)),
        (88, payer_id: PublicKey),
        (89, payer_note: (String, WithoutLength)),
diff --git a/lightning/src/offers/offer.rs b/lightning/src/offers/offer.rs
index d92d0d8b..04acc4da 100644
--- a/lightning/src/offers/offer.rs
+++ b/lightning/src/offers/offer.rs
@@ -578,7 +578,7 @@ tlv_stream!(OfferTlvStream, OfferTlvStreamRef, 1..80, {
        (6, currency: CurrencyCode),
        (8, amount: (u64, HighZeroBytesDroppedBigSize)),
        (10, description: (String, WithoutLength)),
-       (12, features: (OfferFeatures, WithoutLength)),
+       (12, features: OfferFeatures),
        (14, absolute_expiry: (u64, HighZeroBytesDroppedBigSize)),
        (16, paths: (Vec<BlindedPath>, WithoutLength)),
        (18, issuer: (String, WithoutLength)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is expected because impl_feature_tlv_write should be equivalent. I suppose ideally we get rid of the macro and make it explicit for ChannelTypeFeatures.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why get rid of the macro, it seems cleaner to have it and avoid WithoutLength in the offers code 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems confusing if Features encoding were determined by the type of features rather than the context in which they are encoded. While features for the BOLT 12 message are currently only encoded inside a TLV record, that may not always be the case. Better to have the context be explicit about the encoding than to decide now on how the features must be encoded in all contexts, even unknown future contexts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't all features encoded the same everywhere except this weird blinded payinfo one? Seems weird to make several callsites uglier because of one exception, or at least better to do them all on one go rather than have a few flexible exceptions for offers-related feature sets only. Don't think it's weird enough to hold up the PR but just trying to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two cases where the length needs to be included with the features:

  • Non-TLV message fields (e.g., features in init, node_announcement, and channel_announcement messages)
  • TLV records whose value is a collection of a subtype, each containing features (e.g., features in the blinded_payinfo subtype)

The second case is more generally necessary when the subtype contains any variable-length field (e.g., the witness program in fallback addresses). Otherwise, there is no way to tell the length of each element in the collection. See last paragraph in https://github.com/lightning/bolts/blob/master/01-messaging.md#rationale-1.

For BOLT 12, yes, only features in blinded_payinfo need the length as they fall under the second case above. But that's not to say that any of the other feature types won't be used in a similar scenario in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I just meant that for any given feature set X with the exception of blinded_payinfo_features, X will always be encoded the same way.

But that's not to say that any of the other feature types won't be used in a similar scenario in the future.

I just don't see what that scenario would be 🤔 this seems like a one-off weird one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At very least, if there is a such a scenario, the compiler would complain. Whereas it won't complain when defining the serialization with impl_feature_tlv_write.


// Some features may appear both in a TLV record and as part of a TLV subtype sequence. The latter
// requires a length but the former does not.

impl<T: sealed::Context> Writeable for WithoutLength<&Features<T>> {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
self.0.write_be(w)
}
}

impl<T: sealed::Context> Readable for WithoutLength<Features<T>> {
fn read<R: io::Read>(r: &mut R) -> Result<Self, DecodeError> {
let v = io_extras::read_to_end(r)?;
Ok(WithoutLength(Features::<T>::from_be_bytes(v)))
}
}

#[cfg(test)]
mod tests {
use super::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, InvoiceFeatures, NodeFeatures, sealed};
use super::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, InvoiceFeatures, NodeFeatures, OfferFeatures, sealed};
use bitcoin::bech32::{Base32Len, FromBase32, ToBase32, u5};
use crate::util::ser::{Readable, WithoutLength, Writeable};

#[test]
fn sanity_test_unknown_bits() {
Expand Down Expand Up @@ -838,6 +866,20 @@ mod tests {
assert!(features.supports_payment_secret());
}

#[test]
fn encodes_features_without_length() {
let features = OfferFeatures::from_le_bytes(vec![1, 2, 3, 4, 5, 42, 100, 101]);
assert_eq!(features.flags.len(), 8);

let mut serialized_features = Vec::new();
WithoutLength(&features).write(&mut serialized_features).unwrap();
assert_eq!(serialized_features.len(), 8);

let deserialized_features =
WithoutLength::<OfferFeatures>::read(&mut &serialized_features[..]).unwrap().0;
assert_eq!(features, deserialized_features);
}

#[test]
fn invoice_features_encoding() {
let features_as_u5s = vec![
Expand Down
39 changes: 23 additions & 16 deletions lightning/src/offers/invoice_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,11 @@ impl<'a> UnsignedInvoiceRequest<'a> {
unsigned_tlv_stream.write(&mut bytes).unwrap();

let pubkey = self.invoice_request.payer_id;
let signature = Some(merkle::sign_message(sign, SIGNATURE_TAG, &bytes, pubkey)?);
let signature = merkle::sign_message(sign, SIGNATURE_TAG, &bytes, pubkey)?;

// Append the signature TLV record to the bytes.
let signature_tlv_stream = SignatureTlvStreamRef {
signature: signature.as_ref(),
signature: Some(&signature),
};
signature_tlv_stream.write(&mut bytes).unwrap();

Expand All @@ -249,7 +249,7 @@ impl<'a> UnsignedInvoiceRequest<'a> {
pub struct InvoiceRequest {
pub(super) bytes: Vec<u8>,
contents: InvoiceRequestContents,
signature: Option<Signature>,
signature: Signature,
}

/// The contents of an [`InvoiceRequest`], which may be shared with an `Invoice`.
Expand Down Expand Up @@ -311,7 +311,7 @@ impl InvoiceRequest {
/// Signature of the invoice request using [`payer_id`].
///
/// [`payer_id`]: Self::payer_id
pub fn signature(&self) -> Option<Signature> {
pub fn signature(&self) -> Signature {
self.signature
}

Expand All @@ -320,7 +320,7 @@ impl InvoiceRequest {
let (payer_tlv_stream, offer_tlv_stream, invoice_request_tlv_stream) =
self.contents.as_tlv_stream();
let signature_tlv_stream = SignatureTlvStreamRef {
signature: self.signature.as_ref(),
signature: Some(&self.signature),
};
(payer_tlv_stream, offer_tlv_stream, invoice_request_tlv_stream, signature_tlv_stream)
}
Expand Down Expand Up @@ -371,7 +371,7 @@ impl Writeable for InvoiceRequestContents {
tlv_stream!(InvoiceRequestTlvStream, InvoiceRequestTlvStreamRef, 80..160, {
(80, chain: ChainHash),
(82, amount: (u64, HighZeroBytesDroppedBigSize)),
(84, features: InvoiceRequestFeatures),
(84, features: (InvoiceRequestFeatures, WithoutLength)),
(86, quantity: (u64, HighZeroBytesDroppedBigSize)),
(88, payer_id: PublicKey),
(89, payer_note: (String, WithoutLength)),
Expand Down Expand Up @@ -421,9 +421,11 @@ impl TryFrom<Vec<u8>> for InvoiceRequest {
(payer_tlv_stream, offer_tlv_stream, invoice_request_tlv_stream)
)?;

if let Some(signature) = &signature {
merkle::verify_signature(signature, SIGNATURE_TAG, &bytes, contents.payer_id)?;
}
let signature = match signature {
None => return Err(ParseError::InvalidSemantics(SemanticError::MissingSignature)),
Some(signature) => signature,
};
merkle::verify_signature(&signature, SIGNATURE_TAG, &bytes, contents.payer_id)?;

Ok(InvoiceRequest { bytes, contents, signature })
}
Expand Down Expand Up @@ -471,7 +473,7 @@ impl TryFrom<PartialInvoiceRequestTlvStream> for InvoiceRequestContents {

#[cfg(test)]
mod tests {
use super::{InvoiceRequest, InvoiceRequestTlvStreamRef};
use super::{InvoiceRequest, InvoiceRequestTlvStreamRef, SIGNATURE_TAG};

use bitcoin::blockdata::constants::ChainHash;
use bitcoin::network::constants::Network;
Expand All @@ -483,7 +485,7 @@ mod tests {
use core::time::Duration;
use crate::ln::features::InvoiceRequestFeatures;
use crate::ln::msgs::{DecodeError, MAX_VALUE_MSAT};
use crate::offers::merkle::{SignError, SignatureTlvStreamRef};
use crate::offers::merkle::{SignError, SignatureTlvStreamRef, self};
use crate::offers::offer::{Amount, OfferBuilder, OfferTlvStreamRef, Quantity};
use crate::offers::parse::{ParseError, SemanticError};
use crate::offers::payer::PayerTlvStreamRef;
Expand Down Expand Up @@ -536,7 +538,11 @@ mod tests {
assert_eq!(invoice_request.quantity(), None);
assert_eq!(invoice_request.payer_id(), payer_pubkey());
assert_eq!(invoice_request.payer_note(), None);
assert!(invoice_request.signature().is_some());
assert!(
merkle::verify_signature(
&invoice_request.signature, SIGNATURE_TAG, &invoice_request.bytes, payer_pubkey()
).is_ok()
);

assert_eq!(
invoice_request.as_tlv_stream(),
Expand All @@ -563,7 +569,7 @@ mod tests {
payer_id: Some(&payer_pubkey()),
payer_note: None,
},
SignatureTlvStreamRef { signature: invoice_request.signature().as_ref() },
SignatureTlvStreamRef { signature: Some(&invoice_request.signature()) },
),
);

Expand Down Expand Up @@ -1222,7 +1228,7 @@ mod tests {
}

#[test]
fn parses_invoice_request_without_signature() {
fn fails_parsing_invoice_request_without_signature() {
let mut buffer = Vec::new();
OfferBuilder::new("foo".into(), recipient_pubkey())
.amount_msats(1000)
Expand All @@ -1232,8 +1238,9 @@ mod tests {
.invoice_request
.write(&mut buffer).unwrap();

if let Err(e) = InvoiceRequest::try_from(buffer) {
panic!("error parsing invoice_request: {:?}", e);
match InvoiceRequest::try_from(buffer) {
Ok(_) => panic!("expected error"),
Err(e) => assert_eq!(e, ParseError::InvalidSemantics(SemanticError::MissingSignature)),
}
}

Expand Down
2 changes: 1 addition & 1 deletion lightning/src/offers/offer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ tlv_stream!(OfferTlvStream, OfferTlvStreamRef, 1..80, {
(6, currency: CurrencyCode),
(8, amount: (u64, HighZeroBytesDroppedBigSize)),
(10, description: (String, WithoutLength)),
(12, features: OfferFeatures),
(12, features: (OfferFeatures, WithoutLength)),
(14, absolute_expiry: (u64, HighZeroBytesDroppedBigSize)),
(16, paths: (Vec<BlindedPath>, WithoutLength)),
(18, issuer: (String, WithoutLength)),
Expand Down
2 changes: 2 additions & 0 deletions lightning/src/offers/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ pub enum SemanticError {
MissingPayerMetadata,
/// A payer id was expected but was missing.
MissingPayerId,
/// A signature was expected but was missing.
MissingSignature,
}

impl From<bech32::Error> for ParseError {
Expand Down
9 changes: 6 additions & 3 deletions lightning/src/offers/refund.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ pub struct RefundBuilder {
}

impl RefundBuilder {
/// Creates a new builder for a refund using the [`Refund::payer_id`] for signing invoices. Use
/// a different pubkey per refund to avoid correlating refunds.
/// Creates a new builder for a refund using the [`Refund::payer_id`] for the public node id to
/// send to if no [`Refund::paths`] are set. Otherwise, it may be a transient pubkey.
///
/// Additionally, sets the required [`Refund::description`], [`Refund::metadata`], and
/// [`Refund::amount_msats`].
Expand Down Expand Up @@ -285,7 +285,10 @@ impl Refund {
&self.contents.features
}

/// A possibly transient pubkey used to sign the refund.
/// A public node id to send to in the case where there are no [`paths`]. Otherwise, a possibly
/// transient pubkey.
///
/// [`paths`]: Self::paths
pub fn payer_id(&self) -> PublicKey {
self.contents.payer_id
}
Expand Down