Handle DNSSECError messages when attempting DNSSEC resolution

lightning/blips#71 updated the DNSSEC
resolution bLIP to include an explicit error message when DNS(SEC)
resolution was attempted but failed, allowing for faster fallback
to LN-Address (for clients that do) and faster payment failure.

Here we add client-side support for the new message, accepting
`DNSSECError` messages and marking payments as failed when all our
queries have failed.

Largely writen by an LLM

Author: TheBlueMatt

lightning-dns-resolver/src/lib.rs

@@ -273,8 +273,6 @@ mod test {
 
 	#[tokio::test]
 	async fn resolution_test() {
-		let secp_ctx = Secp256k1::new();
-
 		let (resolver_messenger, resolver_id) = create_resolver();
 
 		let resolver_dest = Destination::Node(resolver_id);
@@ -307,25 +305,11 @@ mod test {
 		payer_messenger.peer_connected(resolver_id, &init_msg, true).unwrap();
 		resolver_messenger.get_om().peer_connected(payer_id, &init_msg, false).unwrap();
 
-		let (msg, context) =
-			payer.resolver.resolve_name(payment_id, name.clone(), &*payer_keys).unwrap();
-		let query_context = MessageContext::DNSResolver(context);
-		let receive_key = payer_keys.get_receive_auth_key();
-		let reply_path = BlindedMessagePath::one_hop(
-			payer_id,
-			receive_key,
-			query_context,
-			false,
-			&*payer_keys,
-			&secp_ctx,
-		);
-		payer.pending_messages.lock().unwrap().push((
-			DNSResolverMessage::DNSSECQuery(msg),
-			MessageSendInstructions::WithSpecifiedReplyPath {
-				destination: resolver_dest,
-				reply_path,
-			},
-		));
+		let messages = payer
+			.resolver
+			.resolve_name(payment_id, name.clone(), vec![resolver_dest], &*payer_keys)
+			.unwrap();
+		payer.pending_messages.lock().unwrap().extend(messages);
 
 		let query = payer_messenger.next_onion_message_for_peer(resolver_id).unwrap();
 		resolver_messenger.get_om().handle_onion_message(payer_id, &query);
@@ -351,8 +335,6 @@ mod test {
 	async fn failed_query_does_not_leak_pending_counter() {
 		use std::sync::atomic::Ordering;
 
-		let secp_ctx = Secp256k1::new();
-
 		// Resolver points at a port that should refuse TCP, so build_txt_proof_async
 		// returns Err quickly.
 		let resolver_keys = Arc::new(KeysManager::new(&[99; 32], 42, 43, true));
@@ -405,25 +387,11 @@ mod test {
 		payer_messenger.peer_connected(resolver_id, &init_msg, true).unwrap();
 		resolver_messenger.peer_connected(payer_id, &init_msg, false).unwrap();
 
-		let (msg, context) =
-			payer.resolver.resolve_name(payment_id, name.clone(), &*payer_keys).unwrap();
-		let query_context = MessageContext::DNSResolver(context);
-		let receive_key = payer_keys.get_receive_auth_key();
-		let reply_path = BlindedMessagePath::one_hop(
-			payer_id,
-			receive_key,
-			query_context,
-			false,
-			&*payer_keys,
-			&secp_ctx,
-		);
-		payer.pending_messages.lock().unwrap().push((
-			DNSResolverMessage::DNSSECQuery(msg),
-			MessageSendInstructions::WithSpecifiedReplyPath {
-				destination: resolver_dest,
-				reply_path,
-			},
-		));
+		let messages = payer
+			.resolver
+			.resolve_name(payment_id, name.clone(), vec![resolver_dest], &*payer_keys)
+			.unwrap();
+		payer.pending_messages.lock().unwrap().extend(messages);
 
 		let query = payer_messenger.next_onion_message_for_peer(resolver_id).unwrap();
 		resolver_messenger.handle_onion_message(payer_id, &query);

lightning/src/onion_message/dns_resolution.rs

@@ -40,12 +40,16 @@ use core::fmt;
 use core::ops::Deref;
 
 use crate::blinded_path::message::DNSResolverContext;
+#[cfg(feature = "dnssec")]
+use crate::blinded_path::message::MessageContext;
 use crate::io;
 #[cfg(feature = "dnssec")]
 use crate::ln::channelmanager::PaymentId;
 use crate::ln::msgs::DecodeError;
 #[cfg(feature = "dnssec")]
 use crate::offers::offer::Offer;
+#[cfg(feature = "dnssec")]
+use crate::onion_message::messenger::Destination;
 use crate::onion_message::messenger::{MessageSendInstructions, Responder, ResponseInstruction};
 use crate::onion_message::packet::OnionMessageContents;
 use crate::prelude::*;
@@ -375,6 +379,7 @@ struct PendingResolution {
 	context: DNSResolverContext,
 	name: HumanReadableName,
 	payment_id: PaymentId,
+	pending_queries: usize,
 }
 
 /// A stateful resolver which maps BIP 353 Human Readable Names to URIs and BOLT12 [`Offer`]s.
@@ -462,11 +467,18 @@ impl OMNameResolver {
 
 	/// Begins the process of resolving a BIP 353 Human Readable Name.
 	///
-	/// Returns a [`DNSSECQuery`] onion message and a [`DNSResolverContext`] which should be sent
-	/// to a resolver (with the context used to generate the blinded response path) on success.
+	/// Returns a list of [`DNSSECQuery`] onion messages and the [`MessageSendInstructions`] over
+	/// which each should be sent - one entry per provided `destination`. The query may be sent to
+	/// multiple resolvers (i.e. multiple `destinations`) to improve the chance of a timely
+	/// response; the resolution is only considered failed once a [`DNSSECError`] has been received
+	/// for every one of them (see [`Self::handle_dnssec_error`]).
+	///
+	/// Each [`MessageSendInstructions`] includes a reply path built from the same
+	/// [`DNSResolverContext`], so that responses can be matched back to this resolution.
 	pub fn resolve_name<ES: EntropySource + ?Sized>(
-		&self, payment_id: PaymentId, name: HumanReadableName, entropy_source: &ES,
-	) -> Result<(DNSSECQuery, DNSResolverContext), ()> {
+		&self, payment_id: PaymentId, name: HumanReadableName, destinations: Vec<Destination>,
+		entropy_source: &ES,
+	) -> Result<Vec<(DNSResolverMessage, MessageSendInstructions)>, ()> {
 		let dns_name =
 			Name::try_from(format!("{}.user._bitcoin-payment.{}.", name.user(), name.domain()));
 		debug_assert!(
@@ -477,11 +489,23 @@ impl OMNameResolver {
 		context.nonce.copy_from_slice(&entropy_source.get_secure_random_bytes()[..16]);
 		if let Ok(dns_name) = dns_name {
 			let start_height = self.latest_block_height.load(Ordering::Acquire) as u32;
+			let pending_queries = destinations.len();
+			let query = DNSResolverMessage::DNSSECQuery(DNSSECQuery(dns_name.clone()));
+			let messages = destinations
+				.into_iter()
+				.map(|destination| {
+					let instructions = MessageSendInstructions::WithReplyPath {
+						destination,
+						context: MessageContext::DNSResolver(context.clone()),
+					};
+					(query.clone(), instructions)
+				})
+				.collect();
 			let mut pending_resolves = self.pending_resolves.lock().unwrap();
-			let context_ret = context.clone();
-			let resolution = PendingResolution { start_height, context, name, payment_id };
-			pending_resolves.entry(dns_name.clone()).or_insert_with(Vec::new).push(resolution);
-			Ok((DNSSECQuery(dns_name), context_ret))
+			let resolution =
+				PendingResolution { start_height, context, name, payment_id, pending_queries };
+			pending_resolves.entry(dns_name).or_insert_with(Vec::new).push(resolution);
+			Ok(messages)
 		} else {
 			Err(())
 		}
@@ -607,12 +631,86 @@ impl OMNameResolver {
 		}
 		None
 	}
+
+	/// Handles a [`DNSSECError`] message, indicating that one of the resolvers we sent a
+	/// [`DNSSECQuery`] to was unable to provide a [`DNSSECProof`] for the requested name.
+	///
+	/// Each error counts as a failure of one of the queries we made for the matching resolution
+	/// (see the `num_queries` argument to [`Self::resolve_name`]). A resolution is only considered
+	/// failed once we have received a [`DNSSECError`] for *all* the queries we made for it, as a
+	/// [`DNSSECProof`] may still arrive from one of the other resolvers we queried. When a
+	/// resolution does fail, it is removed and its [`HumanReadableName`] and [`PaymentId`] (as
+	/// passed to [`Self::resolve_name`]) are included in the returned list. The caller should
+	/// consider those resolutions failed.
+	///
+	/// Note that [`DNSSECError::definitely_unresolvable`] is intentionally ignored - a single
+	/// resolver claiming a name is unresolvable does not preclude another resolver we queried from
+	/// successfully resolving it, so we always wait for all queries to fail.
+	///
+	/// As with [`Self::handle_dnssec_proof_for_uri`], the [`DNSResolverContext`] is checked against
+	/// the contexts of any pending resolutions for the name to ensure the error was received over a
+	/// blinded path we created when making the relevant [`DNSSECQuery`].
+	pub fn handle_dnssec_error(
+		&self, msg: DNSSECError, context: DNSResolverContext,
+	) -> Vec<(HumanReadableName, PaymentId)> {
+		let DNSSECError { name, .. } = msg;
+		let mut failed_resolutions = Vec::new();
+		let mut pending_resolves = self.pending_resolves.lock().unwrap();
+		if let hash_map::Entry::Occupied(mut entry) = pending_resolves.entry(name) {
+			entry.get_mut().retain_mut(|resolution| {
+				if resolution.context != context {
+					// This error was not received over the blinded path we created for this
+					// resolution (the contexts differ), so it does not pertain to this resolution.
+					return true;
+				}
+				// One of the queries we made for this resolution failed. Only fail (and remove) the
+				// resolution once all the queries we made for it have failed.
+				resolution.pending_queries = resolution.pending_queries.saturating_sub(1);
+				if resolution.pending_queries == 0 {
+					failed_resolutions.push((resolution.name, resolution.payment_id));
+					false
+				} else {
+					true
+				}
+			});
+			if entry.get().is_empty() {
+				entry.remove();
+			}
+		}
+		failed_resolutions
+	}
 }
 
 #[cfg(test)]
 mod tests {
 	use super::*;
 
+	#[cfg(feature = "dnssec")]
+	use crate::util::test_utils::pubkey;
+
+	#[cfg(feature = "dnssec")]
+	fn dest(b: u8) -> Destination {
+		Destination::Node(pubkey(b))
+	}
+
+	/// Extracts the DNS [`Name`] and [`DNSResolverContext`] from the messages returned by
+	/// [`OMNameResolver::resolve_name`].
+	#[cfg(feature = "dnssec")]
+	fn dns_name_and_context(
+		messages: &[(DNSResolverMessage, MessageSendInstructions)],
+	) -> (Name, DNSResolverContext) {
+		match &messages[0] {
+			(
+				DNSResolverMessage::DNSSECQuery(DNSSECQuery(name)),
+				MessageSendInstructions::WithReplyPath {
+					context: MessageContext::DNSResolver(context),
+					..
+				},
+			) => (name.clone(), context.clone()),
+			_ => panic!("Unexpected resolve_name output"),
+		}
+	}
+
 	#[test]
 	fn test_hrn_display_format() {
 		let user = "user";
@@ -651,6 +749,21 @@ mod tests {
 		assert!(HumanReadableName::new("user", &huge_domain).is_err());
 	}
 
+	#[test]
+	fn test_dnssec_error_roundtrip() {
+		let name = Name::try_from("test.user._bitcoin-payment.example.com.".to_owned()).unwrap();
+		for definitely_unresolvable in [false, true] {
+			let msg = DNSResolverMessage::DNSSECError(DNSSECError {
+				name: name.clone(),
+				definitely_unresolvable,
+			});
+			let mut buf = Vec::new();
+			msg.write(&mut buf).unwrap();
+			let read = DNSResolverMessage::read(&mut &buf[..], DNSSEC_ERROR_TYPE).unwrap();
+			assert_eq!(msg, read);
+		}
+	}
+
 	#[test]
 	#[cfg(feature = "dnssec")]
 	fn test_expiry() {
@@ -659,22 +772,22 @@ mod tests {
 		let name = HumanReadableName::new("user", "example.com").unwrap();
 
 		// Queue up a resolution
-		resolver.resolve_name(PaymentId([0; 32]), name.clone(), &keys).unwrap();
+		resolver.resolve_name(PaymentId([0; 32]), name.clone(), vec![dest(42)], &keys).unwrap();
 		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
 		// and check that it expires after two blocks
 		resolver.new_best_block(44, 42);
 		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 0);
 
 		// Queue up another resolution
-		resolver.resolve_name(PaymentId([1; 32]), name.clone(), &keys).unwrap();
+		resolver.resolve_name(PaymentId([1; 32]), name.clone(), vec![dest(42)], &keys).unwrap();
 		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
 		// it won't expire after one block
 		resolver.new_best_block(45, 42);
 		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
 		assert_eq!(resolver.pending_resolves.lock().unwrap().iter().next().unwrap().1.len(), 1);
 		// and queue up a second and third resolution of the same name
-		resolver.resolve_name(PaymentId([2; 32]), name.clone(), &keys).unwrap();
-		resolver.resolve_name(PaymentId([3; 32]), name.clone(), &keys).unwrap();
+		resolver.resolve_name(PaymentId([2; 32]), name.clone(), vec![dest(42)], &keys).unwrap();
+		resolver.resolve_name(PaymentId([3; 32]), name.clone(), vec![dest(42)], &keys).unwrap();
 		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
 		assert_eq!(resolver.pending_resolves.lock().unwrap().iter().next().unwrap().1.len(), 3);
 		// after another block the first will expire, but the second and third won't
@@ -689,4 +802,68 @@ mod tests {
 		resolver.new_best_block(47, 42);
 		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 0);
 	}
+
+	#[test]
+	#[cfg(feature = "dnssec")]
+	fn test_dnssec_error() {
+		let keys = crate::sign::KeysManager::new(&[33; 32], 0, 0, true);
+		let resolver = OMNameResolver::new(42, 42);
+		let name = HumanReadableName::new("user", "example.com").unwrap();
+
+		// Resolve a name, sending the query to two resolvers.
+		let messages = resolver
+			.resolve_name(PaymentId([0; 32]), name.clone(), vec![dest(1), dest(2)], &keys)
+			.unwrap();
+		assert_eq!(messages.len(), 2);
+		let (dns_name, context) = dns_name_and_context(&messages);
+		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
+
+		// An error whose context doesn't match any pending resolution is ignored entirely (and does
+		// not count against the resolution's queries), even if it claims the name is unresolvable.
+		let mut wrong_context = context.clone();
+		wrong_context.nonce[0] ^= 0x01;
+		let wrong = DNSSECError { name: dns_name.clone(), definitely_unresolvable: true };
+		assert!(resolver.handle_dnssec_error(wrong, wrong_context).is_empty());
+		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
+
+		// The first matching error fails one of the two queries but not the resolution itself - even
+		// though `definitely_unresolvable` is set - as a proof may yet arrive from the other query.
+		let err = DNSSECError { name: dns_name.clone(), definitely_unresolvable: true };
+		assert!(resolver.handle_dnssec_error(err, context.clone()).is_empty());
+		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 1);
+
+		// The second matching error fails the last query, so the resolution now fails - even though
+		// this error only indicates a transient failure.
+		let err = DNSSECError { name: dns_name, definitely_unresolvable: false };
+		let failed = resolver.handle_dnssec_error(err, context);
+		assert_eq!(failed, vec![(name, PaymentId([0; 32]))]);
+		assert_eq!(resolver.pending_resolves.lock().unwrap().len(), 0);
+	}
+
+	#[test]
+	#[cfg(feature = "dnssec")]
+	fn test_dnssec_error_only_fails_matching_resolution() {
+		// An error only counts against the resolution whose blinded path (context) it was received
+		// over; other resolutions for the same name (queued with a different `PaymentId`, and thus a
+		// different context) are left untouched.
+		let keys = crate::sign::KeysManager::new(&[33; 32], 0, 0, true);
+		let resolver = OMNameResolver::new(42, 42);
+		let name = HumanReadableName::new("user", "example.com").unwrap();
+
+		let messages =
+			resolver.resolve_name(PaymentId([0; 32]), name.clone(), vec![dest(1)], &keys).unwrap();
+		let (dns_name, context_a) = dns_name_and_context(&messages);
+		resolver.resolve_name(PaymentId([1; 32]), name.clone(), vec![dest(2)], &keys).unwrap();
+		assert_eq!(resolver.pending_resolves.lock().unwrap().iter().next().unwrap().1.len(), 2);
+
+		// A single error over `context_a` fails only payment 0's (single-query) resolution.
+		let err = DNSSECError { name: dns_name, definitely_unresolvable: false };
+		let failed = resolver.handle_dnssec_error(err, context_a);
+		assert_eq!(failed, vec![(name, PaymentId([0; 32]))]);
+
+		// Payment 1's resolution is still pending.
+		let pending = resolver.pending_resolves.lock().unwrap();
+		assert_eq!(pending.iter().next().unwrap().1.len(), 1);
+		assert_eq!(pending.iter().next().unwrap().1[0].payment_id, PaymentId([1; 32]));
+	}
 }