Merge pull request #2897 from TheBlueMatt/2024-02-fix-route-ser

Fix `Route` serialization round-trip

lightning/src/ln/outbound_payment.rs

@@ -2283,65 +2283,6 @@ mod tests {
 		assert!(pending_events.lock().unwrap().is_empty());
 	}
 
-	#[test]
-	fn fails_paying_for_bolt12_invoice() {
-		let logger = test_utils::TestLogger::new();
-		let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, &logger));
-		let scorer = RwLock::new(test_utils::TestScorer::new());
-		let router = test_utils::TestRouter::new(network_graph, &logger, &scorer);
-		let keys_manager = test_utils::TestKeysInterface::new(&[0; 32], Network::Testnet);
-
-		let pending_events = Mutex::new(VecDeque::new());
-		let outbound_payments = OutboundPayments::new();
-		let payment_id = PaymentId([0; 32]);
-		let expiration = StaleExpiration::AbsoluteTimeout(Duration::from_secs(100));
-
-		let invoice = OfferBuilder::new("foo".into(), recipient_pubkey())
-			.amount_msats(1000)
-			.build().unwrap()
-			.request_invoice(vec![1; 32], payer_pubkey()).unwrap()
-			.build().unwrap()
-			.sign(payer_sign).unwrap()
-			.respond_with_no_std(payment_paths(), payment_hash(), now()).unwrap()
-			.build().unwrap()
-			.sign(recipient_sign).unwrap();
-
-		assert!(
-			outbound_payments.add_new_awaiting_invoice(
-				payment_id, expiration, Retry::Attempts(0),
-				Some(invoice.amount_msats() / 100 + 50_000)
-			).is_ok()
-		);
-		assert!(outbound_payments.has_pending_payments());
-
-		let route_params = RouteParameters::from_payment_params_and_value(
-			PaymentParameters::from_bolt12_invoice(&invoice),
-			invoice.amount_msats(),
-		);
-		router.expect_find_route(
-			route_params.clone(), Ok(Route { paths: vec![], route_params: Some(route_params) })
-		);
-
-		assert_eq!(
-			outbound_payments.send_payment_for_bolt12_invoice(
-				&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
-				&&keys_manager, 0, &&logger, &pending_events, |_| panic!()
-			),
-			Ok(()),
-		);
-		assert!(!outbound_payments.has_pending_payments());
-
-		let payment_hash = invoice.payment_hash();
-		let reason = Some(PaymentFailureReason::UnexpectedError);
-
-		assert!(!pending_events.lock().unwrap().is_empty());
-		assert_eq!(
-			pending_events.lock().unwrap().pop_front(),
-			Some((Event::PaymentFailed { payment_id, payment_hash, reason }, None)),
-		);
-		assert!(pending_events.lock().unwrap().is_empty());
-	}
-
 	#[test]
 	fn sends_payment_for_bolt12_invoice() {
 		let logger = test_utils::TestLogger::new();

lightning/src/routing/router.rs

@@ -505,28 +505,28 @@ impl Writeable for Route {
 		write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
 		(self.paths.len() as u64).write(writer)?;
 		let mut blinded_tails = Vec::new();
-		for path in self.paths.iter() {
+		for (idx, path) in self.paths.iter().enumerate() {
 			(path.hops.len() as u8).write(writer)?;
-			for (idx, hop) in path.hops.iter().enumerate() {
+			for hop in path.hops.iter() {
 				hop.write(writer)?;
-				if let Some(blinded_tail) = &path.blinded_tail {
-					if blinded_tails.is_empty() {
-						blinded_tails = Vec::with_capacity(path.hops.len());
-						for _ in 0..idx {
-							blinded_tails.push(None);
-						}
-					}
-					blinded_tails.push(Some(blinded_tail));
-				} else if !blinded_tails.is_empty() { blinded_tails.push(None); }
 			}
+			if let Some(blinded_tail) = &path.blinded_tail {
+				if blinded_tails.is_empty() {
+					blinded_tails = Vec::with_capacity(path.hops.len());
+					for _ in 0..idx {
+						blinded_tails.push(None);
+					}
+				}
+				blinded_tails.push(Some(blinded_tail));
+			} else if !blinded_tails.is_empty() { blinded_tails.push(None); }
 		}
 		write_tlv_fields!(writer, {
 			// For compatibility with LDK versions prior to 0.0.117, we take the individual
 			// RouteParameters' fields and reconstruct them on read.
 			(1, self.route_params.as_ref().map(|p| &p.payment_params), option),
 			(2, blinded_tails, optional_vec),
 			(3, self.route_params.as_ref().map(|p| p.final_value_msat), option),
-			(5, self.route_params.as_ref().map(|p| p.max_total_routing_fee_msat), option),
+			(5, self.route_params.as_ref().and_then(|p| p.max_total_routing_fee_msat), option),
 		});
 		Ok(())
 	}

lightning/src/util/test_utils.rs

@@ -143,7 +143,9 @@ impl<'a> Router for TestRouter<'a> {
 		&self, payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>,
 		inflight_htlcs: InFlightHtlcs
 	) -> Result<Route, msgs::LightningError> {
-		if let Some((find_route_query, find_route_res)) = self.next_routes.lock().unwrap().pop_front() {
+		let route_res;
+		let next_route_opt = self.next_routes.lock().unwrap().pop_front();
+		if let Some((find_route_query, find_route_res)) = next_route_opt {
 			assert_eq!(find_route_query, *params);
 			if let Ok(ref route) = find_route_res {
 				assert_eq!(route.route_params, Some(find_route_query));
@@ -201,10 +203,18 @@ impl<'a> Router for TestRouter<'a> {
 					}
 				}
 			}
-			return find_route_res;
-		}
+			route_res = find_route_res;
+		} else {
+			route_res = self.router.find_route(payer, params, first_hops, inflight_htlcs);
+		};
 
-		self.router.find_route(payer, params, first_hops, inflight_htlcs)
+		if let Ok(route) = &route_res {
+			// Previously, `Route`s failed to round-trip through serialization due to a write/read
+			// mismatch. Thus, here we test all test-generated routes round-trip:
+			let ser = route.encode();
+			assert_eq!(Route::read(&mut &ser[..]).unwrap(), *route);
+		}
+		route_res
 	}
 
 	fn create_blinded_payment_paths<