Merge pull request #4064 from erickcestari/improve-check-signature-bolt11

refactor(invoice): align signature checks with BOLT11 semantics

Author: valentinewallace

lightning-invoice/src/de.rs

@@ -772,9 +772,6 @@ impl Display for Bolt11ParseError {
 			Bolt11ParseError::InvalidScriptHashLength => {
 				f.write_str("fallback script hash has a length unequal 32 bytes")
 			},
-			Bolt11ParseError::InvalidRecoveryId => {
-				f.write_str("recovery id is out of range (should be in [0,3])")
-			},
 			Bolt11ParseError::Skip => f.write_str(
 				"the tagged field has to be skipped because of an unexpected, but allowed property",
 			),

lightning-invoice/src/lib.rs

@@ -106,7 +106,6 @@ pub enum Bolt11ParseError {
 	InvalidSegWitProgramLength,
 	InvalidPubKeyHashLength,
 	InvalidScriptHashLength,
-	InvalidRecoveryId,
 	// Invalid length, with actual length, expected length, and name of the element
 	InvalidSliceLength(usize, usize, &'static str),
 
@@ -1011,31 +1010,19 @@ impl SignedRawBolt11Invoice {
 	}
 
 	/// Checks if the signature is valid for the included payee public key or if none exists if it's
-	/// valid for the recovered signature (which should always be true?).
+	/// possible to recover the public key from the signature.
 	pub fn check_signature(&self) -> bool {
-		let included_pub_key = self.raw_invoice.payee_pub_key();
+		match self.raw_invoice.payee_pub_key() {
+			Some(pk) => {
+				let hash = Message::from_digest(self.hash);
 
-		let mut recovered_pub_key = Option::None;
-		if recovered_pub_key.is_none() {
-			let recovered = match self.recover_payee_pub_key() {
-				Ok(pk) => pk,
-				Err(_) => return false,
-			};
-			recovered_pub_key = Some(recovered);
-		}
+				let secp_context = Secp256k1::new();
+				let verification_result =
+					secp_context.verify_ecdsa(&hash, &self.signature.to_standard(), pk);
 
-		let pub_key =
-			included_pub_key.or(recovered_pub_key.as_ref()).expect("One is always present");
-
-		let hash = Message::from_digest(self.hash);
-
-		let secp_context = Secp256k1::new();
-		let verification_result =
-			secp_context.verify_ecdsa(&hash, &self.signature.to_standard(), pub_key);
-
-		match verification_result {
-			Ok(()) => true,
-			Err(_) => false,
+				verification_result.is_ok()
+			},
+			None => self.recover_payee_pub_key().is_ok(),
 		}
 	}
 }
@@ -1410,19 +1397,8 @@ impl Bolt11Invoice {
 		}
 	}
 
-	/// Check that the invoice is signed correctly and that key recovery works
+	/// Check that the invoice is signed correctly
 	pub fn check_signature(&self) -> Result<(), Bolt11SemanticError> {
-		match self.signed_invoice.recover_payee_pub_key() {
-			Err(bitcoin::secp256k1::Error::InvalidRecoveryId) => {
-				return Err(Bolt11SemanticError::InvalidRecoveryId)
-			},
-			Err(bitcoin::secp256k1::Error::InvalidSignature) => {
-				return Err(Bolt11SemanticError::InvalidSignature)
-			},
-			Err(e) => panic!("no other error may occur, got {:?}", e),
-			Ok(_) => {},
-		}
-
 		if !self.signed_invoice.check_signature() {
 			return Err(Bolt11SemanticError::InvalidSignature);
 		}
@@ -1873,9 +1849,6 @@ pub enum Bolt11SemanticError {
 	/// The invoice's features are invalid
 	InvalidFeatures,
 
-	/// The recovery id doesn't fit the signature/pub key
-	InvalidRecoveryId,
-
 	/// The invoice's signature is invalid
 	InvalidSignature,
 
@@ -1893,7 +1866,6 @@ impl Display for Bolt11SemanticError {
 			Bolt11SemanticError::NoPaymentSecret => f.write_str("The invoice is missing the mandatory payment secret"),
 			Bolt11SemanticError::MultiplePaymentSecrets => f.write_str("The invoice contains multiple payment secrets"),
 			Bolt11SemanticError::InvalidFeatures => f.write_str("The invoice's features are invalid"),
-			Bolt11SemanticError::InvalidRecoveryId => f.write_str("The recovery id doesn't fit the signature/pub key"),
 			Bolt11SemanticError::InvalidSignature => f.write_str("The invoice's signature is invalid"),
 			Bolt11SemanticError::ImpreciseAmount => f.write_str("The invoice's amount was not a whole number of millisatoshis"),
 		}

lightning-invoice/tests/ser_de.rs

@@ -373,6 +373,26 @@ fn get_test_tuples() -> Vec<(String, SignedRawBolt11Invoice, bool, bool)> {
 			false, // Different features than set in InvoiceBuilder
 			true, // Some unknown fields
 		),
+		(
+			"lnbc1pvjluezsp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygspp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdpl2pkx2ctnv5sxxmmwwd5kgetjypeh2ursdae8g6twvus8g6rfwvs8qun0dfjkxaq9qrsgq357wnc5r2ueh7ck6q93dj32dlqnls087fxdwk8qakdyafkq3yap2r09nt4ndd0unm3z9u5t48y6ucv4r5sg7lk98c77ctvjczkspk5qprc90gx".to_owned(),
+			InvoiceBuilder::new(Currency::Bitcoin)
+				.duration_since_epoch(Duration::from_secs(1496314658))
+				.payment_secret(PaymentSecret([0x11; 32]))
+				.payment_hash(sha256::Hash::from_str(
+					"0001020304050607080900010203040506070809000102030405060708090102"
+				).unwrap())
+				.description("Please consider supporting this project".to_owned())
+				.build_raw()
+				.unwrap()
+				.sign(|_| {
+					RecoverableSignature::from_compact(
+						&<Vec<u8>>::from_hex("8d3ce9e28357337f62da0162d9454df827f83cfe499aeb1c1db349d4d8112742a1bcb35d66d6bf93dc445e51753935cc32a3a411efd8a7c7bd85b25815a01b50").unwrap(),
+						RecoveryId::from_i32(1).unwrap()
+					)
+				}).unwrap(),
+			false, // Different features than set in InvoiceBuilder
+			false, // Some unknown fields
+		),
 
 	]
 }