Handle partial-response UTXO impls or reorgs in chan_announcements

Mostly to add a big comment noting why we aren't "spec-compliant"

Author: TheBlueMatt

src/ln/router.rs

@@ -102,7 +102,21 @@ struct NetworkMap {
 	our_node_id: PublicKey,
 	nodes: HashMap<PublicKey, NodeInfo>,
 }
-
+struct MutNetworkMap<'a> {
+	#[cfg(feature = "non_bitcoin_chain_hash_routing")]
+	channels: &'a mut HashMap<(u64, Sha256dHash), ChannelInfo>,
+	#[cfg(not(feature = "non_bitcoin_chain_hash_routing"))]
+	channels: &'a mut HashMap<u64, ChannelInfo>,
+	nodes: &'a mut HashMap<PublicKey, NodeInfo>,
+}
+impl NetworkMap {
+	fn borrow_parts(&mut self) -> MutNetworkMap {
+		MutNetworkMap {
+			channels: &mut self.channels,
+			nodes: &mut self.nodes,
+		}
+	}
+}
 impl std::fmt::Display for NetworkMap {
 	fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
 		write!(f, "Node id {} network map\n[Channels]\n", log_pubkey!(self.our_node_id))?;
@@ -213,7 +227,7 @@ impl RoutingMessageHandler for Router {
 			panic!("Unknown-required-features ChannelAnnouncements should never deserialize!");
 		}
 
-		match self.chain_monitor.get_chain_utxo(msg.contents.chain_hash, msg.contents.short_channel_id) {
+		let checked_utxo = match self.chain_monitor.get_chain_utxo(msg.contents.chain_hash, msg.contents.short_channel_id) {
 			Ok((script_pubkey, _value)) => {
 				let expected_script = Builder::new().push_opcode(opcodes::All::OP_PUSHNUM_2)
 				                                    .push_slice(&msg.contents.bitcoin_key_1.serialize())
@@ -224,49 +238,67 @@ impl RoutingMessageHandler for Router {
 				}
 				//TODO: Check if value is worth storing, use it to inform routing, and compare it
 				//to the new HTLC max field in channel_update
+				true
 			},
 			Err(ChainError::NotSupported) => {
 				// Tentatively accept, potentially exposing us to DoS attacks
+				false
 			},
 			Err(ChainError::NotWatched) => {
 				return Err(HandleError{err: "Channel announced on an unknown chain", action: Some(ErrorAction::IgnoreError)});
 			},
 			Err(ChainError::UnknownTx) => {
 				return Err(HandleError{err: "Channel announced without corresponding UTXO entry", action: Some(ErrorAction::IgnoreError)});
 			},
-		}
+		};
 
-		let mut network = self.network_map.write().unwrap();
+		let mut network_lock = self.network_map.write().unwrap();
+		let network = network_lock.borrow_parts();
+
+		let chan_info = ChannelInfo {
+				features: msg.contents.features.clone(),
+				one_to_two: DirectionalChannelInfo {
+					src_node_id: msg.contents.node_id_1.clone(),
+					last_update: 0,
+					enabled: false,
+					cltv_expiry_delta: u16::max_value(),
+					htlc_minimum_msat: u64::max_value(),
+					fee_base_msat: u32::max_value(),
+					fee_proportional_millionths: u32::max_value(),
+				},
+				two_to_one: DirectionalChannelInfo {
+					src_node_id: msg.contents.node_id_2.clone(),
+					last_update: 0,
+					enabled: false,
+					cltv_expiry_delta: u16::max_value(),
+					htlc_minimum_msat: u64::max_value(),
+					fee_base_msat: u32::max_value(),
+					fee_proportional_millionths: u32::max_value(),
+				}
+			};
 
 		match network.channels.entry(NetworkMap::get_key(msg.contents.short_channel_id, msg.contents.chain_hash)) {
-			Entry::Occupied(_) => {
+			Entry::Occupied(mut entry) => {
 				//TODO: because asking the blockchain if short_channel_id is valid is only optional
 				//in the blockchain API, we need to handle it smartly here, though its unclear
 				//exactly how...
-				return Err(HandleError{err: "Already have knowledge of channel", action: Some(ErrorAction::IgnoreError)})
+				if checked_utxo {
+					// Either our UTXO provider is busted, there was a reorg, or the UTXO provider
+					// only sometimes returns results. In any case remove the previous entry. Note
+					// that the spec expects us to "blacklist" the node_ids involved, but we can't
+					// do that because
+					// a) we don't *require* a UTXO provider that always returns results.
+					// b) we don't track UTXOs of channels we know about and remove them if they
+					//    get reorg'd out.
+					// c) it's unclear how to do so without exposing ourselves to massive DoS risk.
+					Self::remove_channel_in_nodes(network.nodes, &entry.get(), msg.contents.short_channel_id);
+					*entry.get_mut() = chan_info;
+				} else {
+					return Err(HandleError{err: "Already have knowledge of channel", action: Some(ErrorAction::IgnoreError)})
+				}
 			},
 			Entry::Vacant(entry) => {
-				entry.insert(ChannelInfo {
-					features: msg.contents.features.clone(),
-					one_to_two: DirectionalChannelInfo {
-						src_node_id: msg.contents.node_id_1.clone(),
-						last_update: 0,
-						enabled: false,
-						cltv_expiry_delta: u16::max_value(),
-						htlc_minimum_msat: u64::max_value(),
-						fee_base_msat: u32::max_value(),
-						fee_proportional_millionths: u32::max_value(),
-					},
-					two_to_one: DirectionalChannelInfo {
-						src_node_id: msg.contents.node_id_2.clone(),
-						last_update: 0,
-						enabled: false,
-						cltv_expiry_delta: u16::max_value(),
-						htlc_minimum_msat: u64::max_value(),
-						fee_base_msat: u32::max_value(),
-						fee_proportional_millionths: u32::max_value(),
-					}
-				});
+				entry.insert(chan_info);
 			}
 		};