Skip to content

Hide InvoiceFeatures behind InvoiceBuilder API #901

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

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
220 changes: 188 additions & 32 deletions lightning-invoice/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ pub fn check_platform() {
///
/// (C-not exported) as we likely need to manually select one set of boolean type parameters.
#[derive(Eq, PartialEq, Debug, Clone)]
pub struct InvoiceBuilder<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> {
pub struct InvoiceBuilder<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> {
currency: Currency,
amount: Option<u64>,
si_prefix: Option<SiPrefix>,
Expand All @@ -180,6 +180,7 @@ pub struct InvoiceBuilder<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> {
phantom_h: std::marker::PhantomData<H>,
phantom_t: std::marker::PhantomData<T>,
phantom_c: std::marker::PhantomData<C>,
phantom_s: std::marker::PhantomData<S>,
}

/// Represents a syntactically and semantically correct lightning BOLT11 invoice.
Expand Down Expand Up @@ -427,7 +428,7 @@ pub mod constants {
pub const TAG_FEATURES: u8 = 5;
}

impl InvoiceBuilder<tb::False, tb::False, tb::False, tb::False> {
impl InvoiceBuilder<tb::False, tb::False, tb::False, tb::False, tb::False> {
/// Construct new, empty `InvoiceBuilder`. All necessary fields have to be filled first before
/// `InvoiceBuilder::build(self)` becomes available.
pub fn new(currrency: Currency) -> Self {
Expand All @@ -443,14 +444,15 @@ impl InvoiceBuilder<tb::False, tb::False, tb::False, tb::False> {
phantom_h: std::marker::PhantomData,
phantom_t: std::marker::PhantomData,
phantom_c: std::marker::PhantomData,
phantom_s: std::marker::PhantomData,
}
}
}

impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C> {
impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, T, C, S> {
/// Helper function to set the completeness flags.
fn set_flags<DN: tb::Bool, HN: tb::Bool, TN: tb::Bool, CN: tb::Bool>(self) -> InvoiceBuilder<DN, HN, TN, CN> {
InvoiceBuilder::<DN, HN, TN, CN> {
fn set_flags<DN: tb::Bool, HN: tb::Bool, TN: tb::Bool, CN: tb::Bool, SN: tb::Bool>(self) -> InvoiceBuilder<DN, HN, TN, CN, SN> {
InvoiceBuilder::<DN, HN, TN, CN, SN> {
currency: self.currency,
amount: self.amount,
si_prefix: self.si_prefix,
Expand All @@ -462,6 +464,7 @@ impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T,
phantom_h: std::marker::PhantomData,
phantom_t: std::marker::PhantomData,
phantom_c: std::marker::PhantomData,
phantom_s: std::marker::PhantomData,
}
}

Expand All @@ -482,12 +485,6 @@ impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T,
self
}

/// Sets the payment secret
pub fn payment_secret(mut self, payment_secret: PaymentSecret) -> Self {
self.tagged_fields.push(TaggedField::PaymentSecret(payment_secret));
self
}

/// Sets the expiry time
pub fn expiry_time(mut self, expiry_time: Duration) -> Self {
match ExpiryTime::from_duration(expiry_time) {
Expand All @@ -511,16 +508,9 @@ impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T,
}
self
}

/// Adds a features field which indicates the set of supported protocol extensions which the
/// origin node supports.
pub fn features(mut self, features: InvoiceFeatures) -> Self {
self.tagged_fields.push(TaggedField::Features(features));
self
}
}

impl<D: tb::Bool, H: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, tb::True, C> {
impl<D: tb::Bool, H: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, tb::True, C, S> {
/// Builds a `RawInvoice` if no `CreationError` occurred while construction any of the fields.
pub fn build_raw(self) -> Result<RawInvoice, CreationError> {

Expand Down Expand Up @@ -553,9 +543,9 @@ impl<D: tb::Bool, H: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, tb::True, C> {
}
}

impl<H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<tb::False, H, T, C> {
impl<H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<tb::False, H, T, C, S> {
/// Set the description. This function is only available if no description (hash) was set.
pub fn description(mut self, description: String) -> InvoiceBuilder<tb::True, H, T, C> {
pub fn description(mut self, description: String) -> InvoiceBuilder<tb::True, H, T, C, S> {
match Description::new(description) {
Ok(d) => self.tagged_fields.push(TaggedField::Description(d)),
Err(e) => self.error = Some(e),
Expand All @@ -564,23 +554,23 @@ impl<H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<tb::False, H, T, C> {
}

/// Set the description hash. This function is only available if no description (hash) was set.
pub fn description_hash(mut self, description_hash: sha256::Hash) -> InvoiceBuilder<tb::True, H, T, C> {
pub fn description_hash(mut self, description_hash: sha256::Hash) -> InvoiceBuilder<tb::True, H, T, C, S> {
self.tagged_fields.push(TaggedField::DescriptionHash(Sha256(description_hash)));
self.set_flags()
}
}

impl<D: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, tb::False, T, C> {
impl<D: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<D, tb::False, T, C, S> {
/// Set the payment hash. This function is only available if no payment hash was set.
pub fn payment_hash(mut self, hash: sha256::Hash) -> InvoiceBuilder<D, tb::True, T, C> {
pub fn payment_hash(mut self, hash: sha256::Hash) -> InvoiceBuilder<D, tb::True, T, C, S> {
self.tagged_fields.push(TaggedField::PaymentHash(Sha256(hash)));
self.set_flags()
}
}

impl<D: tb::Bool, H: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, tb::False, C> {
impl<D: tb::Bool, H: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, tb::False, C, S> {
/// Sets the timestamp.
pub fn timestamp(mut self, time: SystemTime) -> InvoiceBuilder<D, H, tb::True, C> {
pub fn timestamp(mut self, time: SystemTime) -> InvoiceBuilder<D, H, tb::True, C, S> {
match PositiveTimestamp::from_system_time(time) {
Ok(t) => self.timestamp = Some(t),
Err(e) => self.error = Some(e),
Expand All @@ -590,22 +580,48 @@ impl<D: tb::Bool, H: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, tb::False, C> {
}

/// Sets the timestamp to the current UNIX timestamp.
pub fn current_timestamp(mut self) -> InvoiceBuilder<D, H, tb::True, C> {
pub fn current_timestamp(mut self) -> InvoiceBuilder<D, H, tb::True, C, S> {
let now = PositiveTimestamp::from_system_time(SystemTime::now());
self.timestamp = Some(now.expect("for the foreseeable future this shouldn't happen"));
self.set_flags()
}
}

impl<D: tb::Bool, H: tb::Bool, T: tb::Bool> InvoiceBuilder<D, H, T, tb::False> {
impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, T, tb::False, S> {
/// Sets `min_final_cltv_expiry`.
pub fn min_final_cltv_expiry(mut self, min_final_cltv_expiry: u64) -> InvoiceBuilder<D, H, T, tb::True> {
pub fn min_final_cltv_expiry(mut self, min_final_cltv_expiry: u64) -> InvoiceBuilder<D, H, T, tb::True, S> {
self.tagged_fields.push(TaggedField::MinFinalCltvExpiry(MinFinalCltvExpiry(min_final_cltv_expiry)));
self.set_flags()
}
}

impl InvoiceBuilder<tb::True, tb::True, tb::True, tb::True> {
impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::False> {
/// Sets the payment secret and relevant features.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, we still need to be able to set more features here, no? You may want to set some future features which is probably in known but not in the set that is required to understand the invoice here.

Copy link
Contributor Author

@jkczyz jkczyz Apr 28, 2021

Choose a reason for hiding this comment

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

Do you mean setting a feature as optional? If so, that is an open question that I had. For instance, would we want to have both basic_mpp_required and basic_mpp_optional methods? It wasn't entirely clear from the BOLT if either were possible:

  • if the basic_mpp feature is offered in the invoice:
    • MAY pay using Basic multi-part payments.
  • otherwise:
    • MUST NOT use Basic multi-part payments.

Does "offered" mean the odd bit (optional) was set? Is something similar possible for payment_secret? The BOLT seems to imply that if a secret is set then it must be used:

  • if there is a valid s field:
    • MUST use that as payment_secret

And what would optional mean here if basic_mpp is set as required as there is a dependency requirement?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean setting a feature as optional?

I meant a feature other than the two below. eg what if we have option_htlcs_colored_blue? Some users may really like blue HTLCs some users may not, and LDK may reasonably support receiving both. Thus, we need some way to provide a Features object, I think.

Does "offered" mean the odd bit (optional) was set?

I believe it means either. I think the handling of var_len_onion here is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant a feature other than the two below. eg what if we have option_htlcs_colored_blue? Some users may really like blue HTLCs some users may not, and LDK may reasonably support receiving both. Thus, we need some way to provide a Features object, I think.

Ah, so the additional type parameter S is for specifying that a secret can be set at most once. Additionally, it is used to condition whether basic_mpp can bet set because that requires the payment_secret feature and thus also a secret field set in the invoice.

Other features may have similar type parameters associated with them if they require some other fields set in the invoice. For example, if the hypothetical feature were instead option_htlcs_colored with an additional field specifying the color blue, then we would add an option_htlcs_colored method taking a color to set in the invoice. It would also set the feature bits as necessary.

Long story short is the purpose of InvoiceBuilder is to enforce BOLT 11 semantics at compile time. Thus, it can never result in a SemanticsError only a CreationError. Allowing features to be set arbitrarily breaks that contract.

Whether the features are set as required or optional is an orthogonal concern. Since these features are optional in InvoiceFeatures::known(), then we'd likely want to set them optional here. I think I had decided on required because the dependency between them and wasn't sure what it would mean for some to be optional and others to be required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmmmm, right, for "known" bits that makes sense. I do wonder how we can support "experimental" feature bits (eg DLC invoices or so), but we really need the ability to set them in InvoiceFeatures as well which would imply its a separate thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For experimental features, I'd imagine there could be a separate method to set these which could also check against any known features.

pub fn payment_secret(mut self, payment_secret: PaymentSecret) -> InvoiceBuilder<D, H, T, C, tb::True> {
let features = InvoiceFeatures::empty()
.set_variable_length_onion_required()
.set_payment_secret_required();
self.tagged_fields.push(TaggedField::PaymentSecret(payment_secret));
self.tagged_fields.push(TaggedField::Features(features));
self.set_flags()
}
}

impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::True> {
/// Sets the `basic_mpp` feature as optional.
pub fn basic_mpp(mut self) -> Self {
self.tagged_fields = self.tagged_fields
.drain(..)
.map(|field| match field {
TaggedField::Features(f) => TaggedField::Features(f.set_basic_mpp_optional()),
Copy link

Choose a reason for hiding this comment

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

Note, what if in the future we introduce a new tagged field requiring to set the 9 field without being dependent on payment secret. We might set mpp here without actually having payment_secret set in our feature bits ?

Maybe we should add a if features.supports_payment_secret { ... } ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is conditionally implemented when payment_secret is set (i.e., when the S type parameter is tb::True). So supporting such a feature should not be problem -- calling basic_mpp() would be a compilation error still.

_ => field,
})
.collect();
self
}
}

impl<S: tb::Bool> InvoiceBuilder<tb::True, tb::True, tb::True, tb::True, S> {
/// Builds and signs an invoice using the supplied `sign_function`. This function MAY NOT fail
/// and MUST produce a recoverable signature valid for the given hash and if applicable also for
/// the included payee public key.
Expand Down Expand Up @@ -644,6 +660,7 @@ impl InvoiceBuilder<tb::True, tb::True, tb::True, tb::True> {
};

invoice.check_field_counts().expect("should be ensured by type signature of builder");
invoice.check_feature_bits().expect("should be ensured by type signature of builder");

Ok(invoice)
}
Expand Down Expand Up @@ -972,6 +989,41 @@ impl Invoice {
Ok(())
}

/// Check that feature bits are set as required
fn check_feature_bits(&self) -> Result<(), SemanticError> {
// "If the payment_secret feature is set, MUST include exactly one s field."
let payment_secret_count = self.tagged_fields().filter(|&tf| match *tf {
TaggedField::PaymentSecret(_) => true,
_ => false,
}).count();
if payment_secret_count > 1 {
return Err(SemanticError::MultiplePaymentSecrets);
}

// "A writer MUST set an s field if and only if the payment_secret feature is set."
let has_payment_secret = payment_secret_count == 1;
let features = self.tagged_fields().find(|&tf| match *tf {
TaggedField::Features(_) => true,
_ => false,
});
match features {
None if has_payment_secret => Err(SemanticError::InvalidFeatures),
None => Ok(()),
Some(TaggedField::Features(features)) => {
if features.supports_payment_secret() && has_payment_secret {
Ok(())
} else if has_payment_secret {
Err(SemanticError::InvalidFeatures)
} else if features.supports_payment_secret() {
Err(SemanticError::InvalidFeatures)
} else {
Ok(())
}
},
Some(_) => unreachable!(),
}
}

/// Check that the invoice is signed correctly and that key recovery works
pub fn check_signature(&self) -> Result<(), SemanticError> {
match self.signed_invoice.recover_payee_pub_key() {
Expand Down Expand Up @@ -1006,6 +1058,7 @@ impl Invoice {
signed_invoice: signed_invoice,
};
invoice.check_field_counts()?;
invoice.check_feature_bits()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there cases where you may want to use this on an invoice not generated by us? Where we may want to accept a non-basic-mpp invoice but not generate them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is supported as written. check_feature_bits simply checks that BOLT 11 requirements are met. Note that the changes in this PR don't require that any feature bits are set. I know we discussed doing that offline yesterday, but I ended up finding a simpler way to enforce the semantics.

invoice.check_signature()?;

Ok(invoice)
Expand Down Expand Up @@ -1298,6 +1351,12 @@ pub enum SemanticError {
/// The invoice contains multiple descriptions and/or description hashes which isn't allowed
MultipleDescriptions,

/// The invoice contains multiple payment secrets
MultiplePaymentSecrets,

/// The invoice's features are invalid
InvalidFeatures,

/// The recovery id doesn't fit the signature/pub key
InvalidRecoveryId,

Expand All @@ -1312,6 +1371,8 @@ impl Display for SemanticError {
SemanticError::MultiplePaymentHashes => f.write_str("The invoice has multiple payment hashes which isn't allowed"),
SemanticError::NoDescription => f.write_str("No description or description hash are part of the invoice"),
SemanticError::MultipleDescriptions => f.write_str("The invoice contains multiple descriptions and/or description hashes which isn't allowed"),
SemanticError::MultiplePaymentSecrets => f.write_str("The invoice contains multiple payment secrets"),
SemanticError::InvalidFeatures => f.write_str("The invoice's features are invalid"),
SemanticError::InvalidRecoveryId => f.write_str("The recovery id doesn't fit the signature/pub key"),
SemanticError::InvalidSignature => f.write_str("The invoice's signature is invalid"),
}
Expand Down Expand Up @@ -1464,6 +1525,97 @@ mod test {
assert!(new_signed.check_signature());
}

#[test]
fn test_check_feature_bits() {
use TaggedField::*;
use lightning::ln::features::InvoiceFeatures;
use secp256k1::Secp256k1;
use secp256k1::key::SecretKey;
use {RawInvoice, RawHrp, RawDataPart, Currency, Sha256, PositiveTimestamp, Invoice,
SemanticError};

let private_key = SecretKey::from_slice(&[42; 32]).unwrap();
let payment_secret = lightning::ln::PaymentSecret([21; 32]);
let invoice_template = RawInvoice {
hrp: RawHrp {
currency: Currency::Bitcoin,
raw_amount: None,
si_prefix: None,
},
data: RawDataPart {
timestamp: PositiveTimestamp::from_unix_timestamp(1496314658).unwrap(),
tagged_fields: vec ! [
PaymentHash(Sha256(sha256::Hash::from_hex(
"0001020304050607080900010203040506070809000102030405060708090102"
).unwrap())).into(),
Description(
::Description::new(
"Please consider supporting this project".to_owned()
).unwrap()
).into(),
],
},
};

// Missing features
let invoice = {
let mut invoice = invoice_template.clone();
invoice.data.tagged_fields.push(PaymentSecret(payment_secret).into());
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::InvalidFeatures));

// Missing feature bits
let invoice = {
let mut invoice = invoice_template.clone();
invoice.data.tagged_fields.push(PaymentSecret(payment_secret).into());
invoice.data.tagged_fields.push(Features(InvoiceFeatures::empty()).into());
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::InvalidFeatures));

// Including payment secret and feature bits
let invoice = {
let mut invoice = invoice_template.clone();
invoice.data.tagged_fields.push(PaymentSecret(payment_secret).into());
invoice.data.tagged_fields.push(Features(InvoiceFeatures::known()).into());
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert!(Invoice::from_signed(invoice).is_ok());

// No payment secret or features
let invoice = {
let invoice = invoice_template.clone();
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert!(Invoice::from_signed(invoice).is_ok());

// No payment secret or feature bits
let invoice = {
let mut invoice = invoice_template.clone();
invoice.data.tagged_fields.push(Features(InvoiceFeatures::empty()).into());
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert!(Invoice::from_signed(invoice).is_ok());

// Missing payment secret
let invoice = {
let mut invoice = invoice_template.clone();
invoice.data.tagged_fields.push(Features(InvoiceFeatures::known()).into());
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::InvalidFeatures));

// Multiple payment secrets
let invoice = {
let mut invoice = invoice_template.clone();
invoice.data.tagged_fields.push(PaymentSecret(payment_secret).into());
invoice.data.tagged_fields.push(PaymentSecret(payment_secret).into());
invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
}.unwrap();
assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::MultiplePaymentSecrets));
}

#[test]
fn test_builder_amount() {
use ::*;
Expand Down Expand Up @@ -1621,14 +1773,16 @@ mod test {
.route(route_1.clone())
.route(route_2.clone())
.description_hash(sha256::Hash::from_slice(&[3;32][..]).unwrap())
.payment_hash(sha256::Hash::from_slice(&[21;32][..]).unwrap());
.payment_hash(sha256::Hash::from_slice(&[21;32][..]).unwrap())
.payment_secret(PaymentSecret([42; 32]))
.basic_mpp();

let invoice = builder.clone().build_signed(|hash| {
secp_ctx.sign_recoverable(hash, &private_key)
}).unwrap();

assert!(invoice.check_signature().is_ok());
assert_eq!(invoice.tagged_fields().count(), 8);
assert_eq!(invoice.tagged_fields().count(), 10);

assert_eq!(invoice.amount_pico_btc(), Some(123));
assert_eq!(invoice.currency(), Currency::BitcoinTestnet);
Expand All @@ -1646,6 +1800,8 @@ mod test {
InvoiceDescription::Hash(&Sha256(sha256::Hash::from_slice(&[3;32][..]).unwrap()))
);
assert_eq!(invoice.payment_hash(), &sha256::Hash::from_slice(&[21;32][..]).unwrap());
assert_eq!(invoice.payment_secret(), Some(&PaymentSecret([42; 32])));
assert_eq!(invoice.features(), Some(&InvoiceFeatures::known()));

let raw_invoice = builder.build_raw().unwrap();
assert_eq!(raw_invoice, *invoice.into_signed_raw().raw_invoice())
Expand Down
Loading