Require payment secrets when building and readinv invoices

Author: TheBlueMatt

lightning-invoice/src/lib.rs

@@ -634,7 +634,7 @@ impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T,
 	}
 }
 
-impl<S: tb::Bool> InvoiceBuilder<tb::True, tb::True, tb::True, tb::True, S> {
+impl InvoiceBuilder<tb::True, tb::True, tb::True, tb::True, tb::True> {
 	/// 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.
@@ -1018,6 +1018,17 @@ impl Invoice {
 			return  Err(SemanticError::MultipleDescriptions);
 		}
 
+		// "A writer 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::NoPaymentSecret);
+		} else if payment_secret_count > 1 {
+			return Err(SemanticError::MultiplePaymentSecrets);
+		}
+
 		Ok(())
 	}
 
@@ -1033,32 +1044,30 @@ impl Invoice {
 
 	/// 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."
+		// "A writer 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 {
+		if payment_secret_count < 1 {
+			return Err(SemanticError::NoPaymentSecret);
+		} else 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;
+		// (this requirement has been since removed, and we now require the payment secret
+		// feature bit always).
 		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(()),
+			None => Err(SemanticError::InvalidFeatures),
 			Some(TaggedField::Features(features)) => {
 				if features.requires_unknown_bits() {
 					Err(SemanticError::InvalidFeatures)
-				} else if features.supports_payment_secret() && has_payment_secret {
-					Ok(())
-				} else if has_payment_secret {
-					Err(SemanticError::InvalidFeatures)
-				} else if features.supports_payment_secret() {
+				} else if !features.supports_payment_secret() {
 					Err(SemanticError::InvalidFeatures)
 				} else {
 					Ok(())
@@ -1147,8 +1156,8 @@ impl Invoice {
 	}
 
 	/// Get the payment secret if one was included in the invoice
-	pub fn payment_secret(&self) -> Option<&PaymentSecret> {
-		self.signed_invoice.payment_secret()
+	pub fn payment_secret(&self) -> &PaymentSecret {
+		self.signed_invoice.payment_secret().expect("was checked by constructor")
 	}
 
 	/// Get the invoice features if they were included in the invoice
@@ -1405,6 +1414,10 @@ pub enum SemanticError {
 	/// The invoice contains multiple descriptions and/or description hashes which isn't allowed
 	MultipleDescriptions,
 
+	/// The invoice is missing the mandatory payment secret, which all modern lightning nodes
+	/// should provide.
+	NoPaymentSecret,
+
 	/// The invoice contains multiple payment secrets
 	MultiplePaymentSecrets,
 
@@ -1428,6 +1441,7 @@ 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::NoPaymentSecret => f.write_str("The invoice is missing the mandatory payment secret"),
 			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"),
@@ -1644,23 +1658,23 @@ mod test {
 			let invoice = invoice_template.clone();
 			invoice.sign::<_, ()>(|hash| Ok(Secp256k1::new().sign_recoverable(hash, &private_key)))
 		}.unwrap();
-		assert!(Invoice::from_signed(invoice).is_ok());
+		assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::NoPaymentSecret));
 
 		// 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());
+		assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::NoPaymentSecret));
 
 		// 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));
+		assert_eq!(Invoice::from_signed(invoice), Err(SemanticError::NoPaymentSecret));
 
 		// Multiple payment secrets
 		let invoice = {
@@ -1746,6 +1760,7 @@ mod test {
 
 		let sign_error_res = builder.clone()
 			.description("Test".into())
+			.payment_secret(PaymentSecret([0; 32]))
 			.try_build_signed(|_| {
 				Err("ImaginaryError")
 			});
@@ -1858,7 +1873,7 @@ 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.payment_secret(), &PaymentSecret([42; 32]));
 		assert_eq!(invoice.features(), Some(&InvoiceFeatures::known()));
 
 		let raw_invoice = builder.build_raw().unwrap();
@@ -1874,6 +1889,7 @@ mod test {
 		let signed_invoice = InvoiceBuilder::new(Currency::Bitcoin)
 			.description("Test".into())
 			.payment_hash(sha256::Hash::from_slice(&[0;32][..]).unwrap())
+			.payment_secret(PaymentSecret([0; 32]))
 			.current_timestamp()
 			.build_raw()
 			.unwrap()

lightning-invoice/src/utils.rs

@@ -132,7 +132,7 @@ mod test {
 		let payment_event = {
 			let mut payment_hash = PaymentHash([0; 32]);
 			payment_hash.0.copy_from_slice(&invoice.payment_hash().as_ref()[0..32]);
-			nodes[0].node.send_payment(&route, payment_hash, &Some(invoice.payment_secret().unwrap().clone())).unwrap();
+			nodes[0].node.send_payment(&route, payment_hash, &Some(invoice.payment_secret().clone())).unwrap();
 			let mut added_monitors = nodes[0].chain_monitor.added_monitors.lock().unwrap();
 			assert_eq!(added_monitors.len(), 1);
 			added_monitors.clear();

lightning-invoice/tests/ser_de.rs

@@ -166,17 +166,17 @@ fn test_bolt_invaoid_invoices() {
 	assert_eq!(Invoice::from_str(
 		"LNBC2500u1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdpquwpc4curk03c9wlrswe78q4eyqc7d8d0xqzpuyk0sg5g70me25alkluzd2x62aysf2pyy8edtjeevuv4p2d5p76r4zkmneet7uvyakky2zr4cusd45tftc9c5fh0nnqpnl2jfll544esqchsrny"
 		), Err(ParseOrSemanticError::ParseError(ParseError::Bech32Error(bech32::Error::MixedCase))));
-	assert_eq!(Invoice::from_str(
+	assert!(Invoice::from_str(
 		"lnbc2500u1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5xysxxatsyp3k7enxv4jsxqzpuaxtrnwngzn3kdzw5hydlzf03qdgm2hdq27cqv3agm2awhz5se903vruatfhq77w3ls4evs3ch9zw97j25emudupq63nyw24cg27h2rspk28uwq"
-		), Err(ParseOrSemanticError::SemanticError(SemanticError::InvalidSignature)));
+		).is_err()); // This invoice is invalid for multiple reasons
 	assert_eq!(Invoice::from_str(
 		"lnbc1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdpl2pkx2ctnv5sxxmmwwd5kgetjypeh2ursdae8g6na6hlh"
 		), Err(ParseOrSemanticError::ParseError(ParseError::TooShortDataPart)));
 	assert_eq!(Invoice::from_str(
 		"lnbc2500x1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5xysxxatsyp3k7enxv4jsxqzpujr6jxr9gq9pv6g46y7d20jfkegkg4gljz2ea2a3m9lmvvr95tq2s0kvu70u3axgelz3kyvtp2ywwt0y8hkx2869zq5dll9nelr83zzqqpgl2zg"
 		), Err(ParseOrSemanticError::ParseError(ParseError::UnknownSiPrefix)));
 eprintln!("GO");
-	assert_eq!(Invoice::from_str(
+	assert!(Invoice::from_str(
 		"lnbc2500000001p1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5xysxxatsyp3k7enxv4jsxqzpu7hqtk93pkf7sw55rdv4k9z2vj050rxdr6za9ekfs3nlt5lr89jqpdmxsmlj9urqumg0h9wzpqecw7th56tdms40p2ny9q4ddvjsedzcplva53s"
-		), Err(ParseOrSemanticError::SemanticError(SemanticError::ImpreciseAmount)));
+		).is_err()); // This invoice is invalid for multiple reasons
 }