From: Matt Corallo <git@bluematt.me>
Date: Wed, 30 Jun 2021 00:27:24 +0000 (+0000)
Subject: Ignore our own gossip if it is sent to us from our counterparty
X-Git-Tag: v0.0.99~4^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=ba600db7931d8f87a738ef077db3583554af60cb;p=rust-lightning

Ignore our own gossip if it is sent to us from our counterparty

If our channel party sends us our own channel_update message, we'll
erroneously use the information in that message to update our view
of the forwarding parameters our counterparty requires of us,
ultimately generating invoices with bogus forwarding information.

This fixes that behavior by checking the channel_update's
directionality before handling it.
---

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 88f35e666..47ea35644 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3408,7 +3408,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					}
 					return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a channel_update for a channel from the wrong node - it shouldn't know about our private channels!".to_owned(), chan_id));
 				}
-				try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
+				let were_node_one = self.get_our_node_id().serialize()[..] < chan.get().get_counterparty_node_id().serialize()[..];
+				let msg_from_node_one = msg.contents.flags & 1 == 0;
+				if were_node_one == msg_from_node_one {
+					return Ok(NotifyOption::SkipPersist);
+				} else {
+					try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
+				}
 			},
 			hash_map::Entry::Vacant(_) => unreachable!()
 		}
@@ -4984,6 +4990,31 @@ mod tests {
 		// At this point the channel info given by peers should still be the same.
 		assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info);
 		assert_eq!(nodes[1].node.list_channels()[0], node_b_chan_info);
+
+		// An earlier version of handle_channel_update didn't check the directionality of the
+		// update message and would always update the local fee info, even if our peer was
+		// (spuriously) forwarding us our own channel_update.
+		let as_node_one = nodes[0].node.get_our_node_id().serialize()[..] < nodes[1].node.get_our_node_id().serialize()[..];
+		let as_update = if as_node_one == (chan.0.contents.flags & 1 == 0 /* chan.0 is from node one */) { &chan.0 } else { &chan.1 };
+		let bs_update = if as_node_one == (chan.0.contents.flags & 1 == 0 /* chan.0 is from node one */) { &chan.1 } else { &chan.0 };
+
+		// First deliver each peers' own message, checking that the node doesn't need to be
+		// persisted and that its channel info remains the same.
+		nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &as_update);
+		nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &bs_update);
+		assert!(!nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
+		assert!(!nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
+		assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info);
+		assert_eq!(nodes[1].node.list_channels()[0], node_b_chan_info);
+
+		// Finally, deliver the other peers' message, ensuring each node needs to be persisted and
+		// the channel info has updated.
+		nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &bs_update);
+		nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &as_update);
+		assert!(nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
+		assert!(nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
+		assert_ne!(nodes[0].node.list_channels()[0], node_a_chan_info);
+		assert_ne!(nodes[1].node.list_channels()[0], node_b_chan_info);
 	}
 }