Hide InvoiceFeatures behind InvoiceBuilder API

Instead of relying on users to set an invoice's features correctly,
enforce the semantics inside InvoiceBuilder. For instance, if the user
sets a PaymentSecret then InvoiceBuilder should ensure the appropriate
feature bits are set. Thus, for this example, the TaggedField
abstraction can be retained while still ensuring BOLT 11 semantics at
the builder abstraction.

diff --git a/lightning-invoice/src/lib.rs b/lightning-invoice/src/lib.rs
index 461ae9b92ad1a9e4d9bb993c13bae59353e89dd0..11406a4644dd0f0e0c5133dc89cf8c84c55f7bbb 100644 (file)
--- a/lightning-invoice/src/lib.rs
+++ b/lightning-invoice/src/lib.rs
@@ -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>,
@@ -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.
@@ -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 {
@@ -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,
@@ -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,
                }
        }
 
@@ -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) {
@@ -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> {
 
@@ -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),
@@ -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),
@@ -590,22 +580,34 @@ 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.
+       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<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.
@@ -644,6 +646,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)
        }
@@ -972,6 +975,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() {
@@ -1006,6 +1044,7 @@ impl Invoice {
                        signed_invoice: signed_invoice,
                };
                invoice.check_field_counts()?;
+               invoice.check_feature_bits()?;
                invoice.check_signature()?;
 
                Ok(invoice)
@@ -1298,6 +1337,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,
 
@@ -1312,6 +1357,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"),
                }

diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs
index 20247ed321a72d87180ccc6347c1cd7e6f9f55a2..c53afdee7b3e644d12d66ec8ab5fc119964e1072 100644 (file)
--- a/lightning-invoice/src/utils.rs
+++ b/lightning-invoice/src/utils.rs
@@ -6,7 +6,6 @@ use lightning::chain;
 use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use lightning::chain::keysinterface::{Sign, KeysInterface};
 use lightning::ln::channelmanager::{ChannelManager, MIN_FINAL_CLTV_EXPIRY};
-use lightning::ln::features::InvoiceFeatures;
 use lightning::routing::network_graph::RoutingFees;
 use lightning::routing::router::RouteHintHop;
 use lightning::util::logger::Logger;
@@ -65,7 +64,6 @@ where
                .payee_pub_key(our_node_pubkey)
                .payment_hash(Hash::from_slice(&payment_hash.0).unwrap())
                .payment_secret(payment_secret)
-               .features(InvoiceFeatures::known())
                .min_final_cltv_expiry(MIN_FINAL_CLTV_EXPIRY.into());
        if let Some(amt) = amt_msat {
                invoice = invoice.amount_pico_btc(amt * 10);

diff --git a/lightning-invoice/tests/ser_de.rs b/lightning-invoice/tests/ser_de.rs
index d641c54273c24e10f2abac5ed5b0ced61df267bc..442166e740e7568f0a25b24883c1f3ad0e016f2e 100644 (file)
--- a/lightning-invoice/tests/ser_de.rs
+++ b/lightning-invoice/tests/ser_de.rs
@@ -103,7 +103,7 @@ fn get_test_tuples() -> Vec<(String, SignedRawInvoice, Option<SemanticError>)> {
                        None
                ),
                (
-                       "lnbc20m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp59g4z52329g4z52329g4z52329g4z52329g4z52329g4z52329g4q9gkzyrw8zhfxmrcxsx7hj40yejq6lkvn75l9yjmapjv94haz8x8jy2tvmgex8rnyqkj825csd2t64fu0p4ctad2cf4tgy5gh2fns6ygp6pnc3y".to_owned(),
+                       "lnbc20m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp59g4z52329g4z52329g4z52329g4z52329g4z52329g4z52329g4q9qrsgqzfhag3vsafx4e5qssalvw4rn0phsvpp3e5h2xxyk9l8fxsutvndx9t840dqvdrlu2gqmk0q8apqrgnjy9amc07hmjl9e9yzqjks5w2gqgjnyms".to_owned(),
                        InvoiceBuilder::new(Currency::Bitcoin)
                                .payment_hash(sha256::Hash::from_hex(
                                        "0001020304050607080900010203040506070809000102030405060708090102"

diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
index 0b01c41690efd795ebf107f5db4dd80e76783f0c..99e5cd0994c9971cdc32383c70650184d024c879 100644 (file)
--- a/lightning/src/ln/features.rs
+++ b/lightning/src/ln/features.rs
@@ -646,11 +646,8 @@ impl<T: sealed::PaymentSecret> Features<T> {
        pub(crate) fn requires_payment_secret(&self) -> bool {
                <T as sealed::PaymentSecret>::requires_feature(&self.flags)
        }
-       // Note that we never need to test this since what really matters is the invoice - iff the
-       // invoice provides a payment_secret, we assume that we can use it (ie that the recipient
-       // supports payment_secret).
-       #[allow(dead_code)]
-       pub(crate) fn supports_payment_secret(&self) -> bool {
+       /// Returns whether the `payment_secret` feature is supported.
+       pub fn supports_payment_secret(&self) -> bool {
                <T as sealed::PaymentSecret>::supports_feature(&self.flags)
        }
 }