Respond to Matt's additional comments.

Author: arik-so

lightning-rapid-gossip-sync/src/error.rs

@@ -4,21 +4,17 @@ use lightning::ln::msgs::{DecodeError, LightningError};
 
 /// All-encompassing standard error type that processing can return
 pub enum GraphSyncError {
-	/// IO error wrapper, typically the result of an issue with the file system
-	IOError(std::io::Error),
 	/// Error trying to read the update data, typically due to an erroneous data length indication
 	/// that is greater than the actual amount of data provided
 	DecodeError(DecodeError),
 	/// Error applying the patch to the network graph, usually the result of updates that are too
 	/// old or missing prerequisite data to the application of updates out of order
 	LightningError(LightningError),
-	/// Some other error whose nature is indicated in its descriptor string
-	ProcessingError(String),
 }
 
 impl From<std::io::Error> for GraphSyncError {
 	fn from(error: std::io::Error) -> Self {
-		Self::IOError(error)
+		Self::DecodeError(DecodeError::Io(error.kind()))
 	}
 }
 
@@ -37,10 +33,8 @@ impl From<LightningError> for GraphSyncError {
 impl Debug for GraphSyncError {
 	fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
 		match self {
-			GraphSyncError::IOError(e) => f.write_fmt(format_args!("IOError: {:?}", e)),
 			GraphSyncError::DecodeError(e) => f.write_fmt(format_args!("DecodeError: {:?}", e)),
-			GraphSyncError::LightningError(e) => f.write_fmt(format_args!("LightningError: {:?}", e)),
-			GraphSyncError::ProcessingError(e) => f.write_fmt(format_args!("ProcessingError: {}", e))
+			GraphSyncError::LightningError(e) => f.write_fmt(format_args!("LightningError: {:?}", e))
 		}
 	}
 }

lightning-rapid-gossip-sync/src/lib.rs

@@ -10,11 +10,21 @@
 //! This crate exposes functionality to rapidly sync gossip data, aimed primarily at mobile
 //! devices.
 //!
-//! The (presumed) server sends a compressed response containing gossip data. The gossip data is
+//! The server sends a compressed response containing gossip data. The gossip data is
 //! formatted compactly, omitting signatures and opportunistically incremental where previous
 //! channel updates are known (a mechanism that is enabled when the timestamp of the last known
 //! channel update is communicated). A reference server implementation can be found
-//! [here](https://github.com/arik-so/rust-ln-sync).
+//! [here](https://github.com/lightningdevkit/rapid-gossip-sync-server).
+//!
+//! The primary benefit this syncing mechanism provides is that given a trusted server, a
+//! low-powered client can offload the validation of gossip signatures. This enables a client to
+//! privately calculate routes for payments, and do so much faster and earlier than requiring a full
+//! peer-to-peer gossip sync to complete.
+//!
+//! The reason the rapid sync server requires a modicum of trust is that it could provide bogus
+//! data, though at worst, all that would result in is a fake network topology, which wouldn't
+//! enable the server to steal or siphon off funds. It could, however, reduce the client's privacy
+//! by increasing the likelihood payments will are routed via channels the server controls.
 //!
 //! The way a server is meant to calculate this rapid gossip sync data is by using a `latest_seen`
 //! timestamp provided by the client. It's not included in either channel announcement or update,
@@ -67,12 +77,46 @@ mod tests {
 	use bitcoin::blockdata::constants::genesis_block;
 	use bitcoin::Network;
 
+	use lightning::ln::msgs::DecodeError;
 	use lightning::routing::network_graph::NetworkGraph;
 
 	use crate::sync_network_graph_with_file_path;
 
 	#[test]
 	fn test_sync_from_file() {
+		struct FileSyncTest {
+			directory: String,
+		}
+
+		impl FileSyncTest {
+			fn new(tmp_directory: &str, valid_response: &[u8]) -> FileSyncTest {
+				let test = FileSyncTest { directory: tmp_directory.to_owned() };
+
+				let graph_sync_test_directory = test.get_test_directory();
+				fs::create_dir_all(graph_sync_test_directory).unwrap();
+
+				let graph_sync_test_file = test.get_test_file_path();
+				fs::write(&graph_sync_test_file, valid_response).unwrap();
+
+				test
+			}
+			fn get_test_directory(&self) -> String {
+				let graph_sync_test_directory = self.directory.clone() + "/graph-sync-tests";
+				graph_sync_test_directory
+			}
+			fn get_test_file_path(&self) -> String {
+				let graph_sync_test_directory = self.get_test_directory();
+				let graph_sync_test_file = graph_sync_test_directory.to_owned() + "/test_data.lngossip";
+				graph_sync_test_file
+			}
+		}
+
+		impl Drop for FileSyncTest {
+			fn drop(&mut self) {
+				fs::remove_dir_all(self.directory.clone()).unwrap();
+			}
+		}
+
 		// same as incremental_only_update_fails_without_prior_same_direction_updates
 		let valid_response = vec![
 			76, 68, 75, 1, 111, 226, 140, 10, 182, 241, 179, 114, 193, 166, 162, 70, 174, 99, 247,
@@ -90,19 +134,16 @@ mod tests {
 			0, 1, 0, 0, 0, 125, 255, 2, 68, 226, 0, 6, 11, 0, 1, 5, 0, 0, 0, 0, 29, 129, 25, 192,
 		];
 
-		let tmp_directory = "./tmp";
-		let graph_sync_test_directory = tmp_directory.to_owned() + "/graph-sync-tests";
-		let graph_sync_test_file = graph_sync_test_directory.to_owned() + "/test_data.lngossip";
-		fs::create_dir_all(graph_sync_test_directory).unwrap();
-		fs::write(&graph_sync_test_file, valid_response).unwrap();
+		let tmp_directory = "./rapid-gossip-sync-tests-tmp";
+		let sync_test = FileSyncTest::new(tmp_directory, &valid_response);
+		let graph_sync_test_file = sync_test.get_test_file_path();
 
 		let block_hash = genesis_block(Network::Bitcoin).block_hash();
 		let network_graph = NetworkGraph::new(block_hash);
 
 		assert_eq!(network_graph.read_only().channels().len(), 0);
 
 		let sync_result = sync_network_graph_with_file_path(&network_graph, &graph_sync_test_file);
-		fs::remove_dir_all(tmp_directory).unwrap();
 
 		if sync_result.is_err() {
 			panic!("Unexpected sync result: {:?}", sync_result)
@@ -136,8 +177,8 @@ mod tests {
 		let start = std::time::Instant::now();
 		let sync_result =
 			sync_network_graph_with_file_path(&network_graph, "./res/full_graph.lngossip");
-		if let Err(crate::error::GraphSyncError::IOError(io_error)) = &sync_result {
-			let error_string = format!("Input file lightning-graph-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin\n\n{}", io_error);
+		if let Err(crate::error::GraphSyncError::DecodeError(DecodeError::Io(io_error))) = &sync_result {
+			let error_string = format!("Input file lightning-graph-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin\n\n{:?}", io_error);
 			#[cfg(not(require_route_graph_test))]
 			{
 				println!("{}", error_string);
@@ -160,6 +201,7 @@ pub mod bench {
 	use bitcoin::blockdata::constants::genesis_block;
 	use bitcoin::Network;
 
+	use lightning::ln::msgs::DecodeError;
 	use lightning::routing::network_graph::NetworkGraph;
 
 	use crate::sync_network_graph_with_file_path;
@@ -173,8 +215,8 @@ pub mod bench {
 				&network_graph,
 				"./res/full_graph.lngossip",
 			);
-			if let Err(crate::error::GraphSyncError::IOError(io_error)) = &sync_result {
-				let error_string = format!("Input file lightning-graph-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin\n\n{}", io_error);
+			if let Err(crate::error::GraphSyncError::DecodeError(DecodeError::Io(io_error))) = &sync_result {
+				let error_string = format!("Input file lightning-graph-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin\n\n{:?}", io_error);
 				#[cfg(not(require_route_graph_test))]
 				{
 					println!("{}", error_string);

lightning-rapid-gossip-sync/src/processing.rs

@@ -12,7 +12,6 @@ use lightning::routing::network_graph;
 use lightning::util::ser::{BigSize, Readable};
 
 use crate::error::GraphSyncError;
-use crate::GraphSyncError::ProcessingError;
 
 /// The purpose of this prefix is to identify the serialization format, should other rapid gossip
 /// sync formats arise in the future.
@@ -47,9 +46,7 @@ pub(crate) fn update_network_graph_from_byte_stream<R: Read>(
 	let _deserialize_unsigned = match prefix {
 		GOSSIP_PREFIX => true,
 		_ => {
-			return Err(GraphSyncError::ProcessingError(
-				"Prefix must equal 'LDK' and the version byte 1".to_string(),
-			));
+			return Err(DecodeError::UnknownVersion.into());
 		}
 	};
 
@@ -75,17 +72,13 @@ pub(crate) fn update_network_graph_from_byte_stream<R: Read>(
 		let scid_delta: BigSize = Readable::read(read_cursor)?;
 		let short_channel_id = previous_scid
 			.checked_add(scid_delta.0)
-			.ok_or(ProcessingError(
-				"Invalid announcement SCID delta producing u64 overflow.".to_string(),
-			))?;
+			.ok_or(DecodeError::InvalidValue)?;
 		previous_scid = short_channel_id;
 
 		let node_id_1_index: BigSize = Readable::read(read_cursor)?;
 		let node_id_2_index: BigSize = Readable::read(read_cursor)?;
 		if max(node_id_1_index.0, node_id_2_index.0) >= node_id_count as u64 {
-			return Err(GraphSyncError::ProcessingError(
-				"Node id index out of bounds".to_string(),
-			));
+			return Err(DecodeError::InvalidValue.into());
 		};
 		let node_id_1 = node_ids[node_id_1_index.0 as usize];
 		let node_id_2 = node_ids[node_id_2_index.0 as usize];
@@ -129,9 +122,7 @@ pub(crate) fn update_network_graph_from_byte_stream<R: Read>(
 		let scid_delta: BigSize = Readable::read(read_cursor)?;
 		let short_channel_id = previous_scid
 			.checked_add(scid_delta.0)
-			.ok_or(ProcessingError(
-				"Invalid update SCID delta producing u64 overflow.".to_string(),
-			))?;
+			.ok_or(DecodeError::InvalidValue)?;
 		previous_scid = short_channel_id;
 
 		let channel_flags: u8 = Readable::read(read_cursor)?;
@@ -225,14 +216,7 @@ pub(crate) fn update_network_graph_from_byte_stream<R: Read>(
 		network_graph.update_channel_unsigned(&synthetic_update)?;
 	}
 
-	let input_termination_check: Result<u8, DecodeError> = Readable::read(read_cursor);
-	if let Err(DecodeError::ShortRead) = input_termination_check {
-		Ok(())
-	} else {
-		Err(GraphSyncError::ProcessingError(
-			"unexpected data found".to_string(),
-		))
-	}
+	Ok(())
 }
 
 #[cfg(test)]
@@ -276,38 +260,6 @@ mod tests {
 		}
 	}
 
-	#[test]
-	fn network_graph_fails_to_update_from_extended_input() {
-		let block_hash = genesis_block(Network::Bitcoin).block_hash();
-		let network_graph = NetworkGraph::new(block_hash);
-
-		let example_input = vec![
-			76, 68, 75, 1, 111, 226, 140, 10, 182, 241, 179, 114, 193, 166, 162, 70, 174, 99, 247,
-			79, 147, 30, 131, 101, 225, 90, 8, 156, 104, 214, 25, 0, 0, 0, 0, 0, 97, 227, 98, 218,
-			0, 0, 0, 4, 2, 22, 7, 207, 206, 25, 164, 197, 231, 230, 231, 56, 102, 61, 250, 251,
-			187, 172, 38, 46, 79, 247, 108, 44, 155, 48, 219, 238, 252, 53, 192, 6, 67, 2, 36, 125,
-			157, 176, 223, 175, 234, 116, 94, 248, 201, 225, 97, 235, 50, 47, 115, 172, 63, 136,
-			88, 216, 115, 11, 111, 217, 114, 84, 116, 124, 231, 107, 2, 158, 1, 242, 121, 152, 106,
-			204, 131, 186, 35, 93, 70, 216, 10, 237, 224, 183, 89, 95, 65, 3, 83, 185, 58, 138,
-			181, 64, 187, 103, 127, 68, 50, 2, 201, 19, 17, 138, 136, 149, 185, 226, 156, 137, 175,
-			110, 32, 237, 0, 217, 90, 31, 100, 228, 149, 46, 219, 175, 168, 77, 4, 143, 38, 128,
-			76, 97, 0, 0, 0, 2, 0, 0, 255, 8, 153, 192, 0, 2, 27, 0, 0, 0, 1, 0, 0, 255, 2, 68,
-			226, 0, 6, 11, 0, 1, 2, 3, 0, 0, 0, 2, 0, 40, 0, 0, 0, 0, 0, 0, 3, 232, 0, 0, 0, 100,
-			0, 0, 2, 224, 0, 0, 0, 0, 29, 129, 25, 192, 255, 8, 153, 192, 0, 2, 27, 0, 0, 36, 0, 0,
-			0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 58, 85, 116, 216, 255, 2, 68, 226, 0, 6, 11, 0, 1, 24, 0,
-			0, 3, 232, 0, 0, 0, 1, 10,
-		];
-		let update_result = update_network_graph(&network_graph, &example_input[..]);
-		assert!(update_result.is_err());
-
-		let error_string = if let Err(GraphSyncError::ProcessingError(error)) = update_result {
-			error
-		} else {
-			panic!("Unexpected update result: {:?}", update_result)
-		};
-		assert!(error_string.contains("unexpected data found"));
-	}
-
 	#[test]
 	fn incremental_only_update_fails_without_prior_announcements() {
 		let incremental_update_input = vec![