Review changes

Author: sourabhmarathe

lightning/src/routing/network_graph.rs

@@ -540,30 +540,30 @@ impl NetworkGraph {
 		None
 	}
 
-	/// Increments payment_sent_score for appropriate DirectionalChannelInfos on the network graph.
-	/// The appropriate directional channel is indentified by the RouteHop, which includes a
-	/// PublicKey field. This is matched against the two PublicKey fields in ChannelInfo and is
-	/// used to increment the score.
+	/// Increments payment_sent_score for both DirectionalChannelInfos on any channel that
+	/// cooperatively executed a successful payment. In the future, this could be more nuanced (eg
+	/// only score the direction that cooperates, bring in other parameters for the channel_score
+	/// field within DirectionalChannelInfo)
 	pub fn score_payment_sent_for_route(&mut self, route: Vec<Vec<RouteHop>>) -> Result<bool, LightningError> {
+		let mut channel_down: bool = false;
 		for path in route {
 			for route_hop in path {
 				let short_channel_id = route_hop.short_channel_id;
 				if let Some(channel) = self.channels.get_mut(&short_channel_id) {
-					let directional_channel_info = if route_hop.pubkey == channel.node_one {
-						channel.one_to_two.as_mut().unwrap()
-					} else {
-						channel.two_to_one.as_mut().unwrap()
-					};
-					let channel_score = &mut directional_channel_info.channel_score;
-					channel_score.payment_sent_score += 1;
+					channel.one_to_two.as_mut().unwrap().channel_score.payment_sent_score += 1;
+					channel.two_to_one.as_mut().unwrap().channel_score.payment_sent_score += 1;
 				} else {
-					return Err(LightningError {
-						err: "PaymentSent event occurred for this channel, but the channel was closed before scoring",
-						action: ErrorAction::IgnoreError
-					});
+					channel_down = true;
+					continue;
 				}
 			}
 		}
+		if channel_down {
+			return Err(LightningError {
+				err: "PaymentSent event occurred for this channel, but the channel was closed before scoring",
+				action: ErrorAction::IgnoreError
+			});
+		}
 		Ok(true)
 	}
 
@@ -573,9 +573,6 @@ impl NetworkGraph {
 	/// against a list of at fault nodes that determines whether the node's channel score should be
 	/// penalized for failing to execute or rewarded for executing properly.
 	pub fn score_payment_failed_for_route(&mut self, route: Vec<Vec<RouteHop>>, faultive_nodes: Vec<PublicKey>) -> Result<bool, LightningError> {
-		if faultive_nodes.len() == 0 {
-			return self.score_payment_sent_for_route(route)
-		}
 		for path in route {
 			for route_hop in path {
 				let short_channel_id = route_hop.short_channel_id;
@@ -1808,24 +1805,41 @@ mod tests {
 			let channel_info = network.channels.get_mut(&chan_id).unwrap();
 			// Assign a DirectionalChannelInfo and ChannelScore object to one_to_two of
 			// channel_info
-			let chan_score = ChannelScore {
+			let chan_score_1 = ChannelScore {
+				payment_sent_score: 0,
+				payment_failed_score: 0,
+			};
+			let chan_score_2 = ChannelScore {
 				payment_sent_score: 0,
 				payment_failed_score: 0,
 			};
+
 			let dummy_routing_fee = RoutingFees {
 				base_msat: 0,
 				proportional_millionths: 0,
 			};
-			let dir_chan_info = DirectionalChannelInfo {
+			let dir_chan_info_1 = DirectionalChannelInfo {
+				last_update: 0,
+				enabled: true,
+				cltv_expiry_delta: 0,
+				htlc_minimum_msat: 0,
+				fees: dummy_routing_fee,
+				last_update_message: None,
+				channel_score: chan_score_1,
+			};
+
+			let dir_chan_info_2 = DirectionalChannelInfo {
 				last_update: 0,
 				enabled: true,
 				cltv_expiry_delta: 0,
 				htlc_minimum_msat: 0,
 				fees: dummy_routing_fee,
 				last_update_message: None,
-				channel_score: chan_score,
+				channel_score: chan_score_2,
 			};
-			channel_info.one_to_two = Some(dir_chan_info);
+
+			channel_info.one_to_two = Some(dir_chan_info_1);
+			channel_info.two_to_one = Some(dir_chan_info_2);
 			let dir_one_to_two = channel_info.one_to_two.as_mut().unwrap();
 			assert_eq!(dir_one_to_two.channel_score.payment_sent_score, 0);
 			assert_eq!(dir_one_to_two.channel_score.payment_failed_score, 0);
@@ -1835,13 +1849,13 @@ mod tests {
 			// Assume a payment is made, and node 1 is learning of a PaymentSent event. It now calls
 			// its score_payment_sent_for_route on itself.
 			let route_hop = RouteHop {
-								pubkey: node_id_1,
-								node_features: NodeFeatures::empty(),
-								short_channel_id: 0,
-								channel_features: ChannelFeatures::empty(),
-								fee_msat: 0,
-								cltv_expiry_delta: 0,
-							};
+				pubkey: node_id_1,
+				node_features: NodeFeatures::empty(),
+				short_channel_id: 0,
+				channel_features: ChannelFeatures::empty(),
+				fee_msat: 0,
+				cltv_expiry_delta: 0,
+			};
 			let sent_paths: Vec<RouteHop> = vec![route_hop];
 			let sent_route: Vec<Vec<RouteHop>> = vec![sent_paths];
 			let mut network = net_graph_msg_handler.network_graph.write().unwrap();