-f fix new Matt's comments

Author: Antoine Riard

lightning/src/ln/channelmanager.rs

@@ -53,7 +53,7 @@ use ln::onion_utils;
 use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, OptionalField};
 use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner};
 use util::config::UserConfig;
-use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureDescriptor};
+use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 use util::{byte_utils, events};
 use util::ser::{Readable, ReadableArgs, MaybeReadable, Writeable, Writer};
 use util::chacha20::{ChaCha20, ChaChaReader};
@@ -804,7 +804,7 @@ macro_rules! handle_error {
 						});
 					}
 					if let Some(channel_id) = chan_id {
-						$self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id,  err: ClosureDescriptor::ProcessingError { err: err.err.clone() } });
+						$self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id,  err: ClosureReason::ProcessingError { err: err.err.clone() } });
 					}
 				}
 
@@ -1427,6 +1427,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				if let Some(short_id) = chan.get().get_short_channel_id() {
 					channel_state.short_to_id.remove(&short_id);
 				}
+				let mut pending_events_lock = self.pending_events.lock().unwrap();
+				if peer_node_id.is_some() {
+					if let Some(peer_msg) = peer_msg {
+						pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, err: ClosureReason::CounterpartyForceClosed { peer_msg: Some(peer_msg.to_string()) } });
+					} else {
+						pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, err: ClosureReason::CounterpartyForceClosed { peer_msg: None } });
+					}
+				} else {
+					pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, err: ClosureReason::HolderForceClosed });
+				}
 				chan.remove_entry().1
 			} else {
 				return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
@@ -1440,7 +1450,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				msg: update
 			});
 		}
-		self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: *channel_id,  err: ClosureDescriptor::ForceClosed { peer_msg: if peer_msg.is_some() { Some(peer_msg.unwrap().clone()) } else { None }}});
 
 		Ok(chan.get_counterparty_node_id())
 	}
@@ -3507,7 +3516,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				});
 			}
 			//TODO: split between CounterpartyInitiated/LocallyInitiated
-			self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: msg.channel_id,  err: ClosureDescriptor::CooperativeClosure });
+			self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: msg.channel_id,  err: ClosureReason::CooperativeClosure });
 		}
 		Ok(())
 	}
@@ -3918,7 +3927,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								msg: update
 							});
 						}
-						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureDescriptor::CommitmentTxBroadcasted });
+						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureReason::CommitmentTxBroadcasted });
 						pending_msg_events.push(events::MessageSendEvent::HandleError {
 							node_id: chan.get_counterparty_node_id(),
 							action: msgs::ErrorAction::SendErrorMessage {
@@ -4454,7 +4463,7 @@ where
 							msg: update
 						});
 					}
-					self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: channel.channel_id(),  err: ClosureDescriptor::CommitmentTxBroadcasted });
+					self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: channel.channel_id(),  err: ClosureReason::CommitmentTxBroadcasted });
 					pending_msg_events.push(events::MessageSendEvent::HandleError {
 						node_id: channel.get_counterparty_node_id(),
 						action: msgs::ErrorAction::SendErrorMessage { msg: e },
@@ -4645,7 +4654,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 								msg: update
 							});
 						}
-						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureDescriptor::DisconnectedPeer });
+						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureReason::DisconnectedPeer });
 						false
 					} else {
 						true
@@ -4660,7 +4669,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 							if let Some(short_id) = chan.get_short_channel_id() {
 								short_to_id.remove(&short_id);
 							}
-							self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureDescriptor::DisconnectedPeer });
+							self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureReason::DisconnectedPeer });
 							return false;
 						} else {
 							no_channels_remain = false;
@@ -4757,7 +4766,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 			}
 		} else {
 			// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
-			let _ = self.force_close_channel_with_peer(&msg.channel_id, Some(counterparty_node_id), None);
+			let _ = self.force_close_channel_with_peer(&msg.channel_id, Some(counterparty_node_id), Some(&msg.data));
 		}
 	}
 }

lightning/src/util/events.rs

@@ -69,13 +69,15 @@ pub enum PaymentPurpose {
 }
 
 #[derive(Clone, Debug)]
-pub enum ClosureDescriptor {
-	/// Closure generated from ChannelManager::force_close_channel or receiving a peer error
-	/// message by ChannelManager::handle_error
-	ForceClosed {
-		/// If the error is coming from the peer, there should be a human-readable msg
+/// Some information provided on the closure source of the channel halting.
+pub enum ClosureReason {
+	/// Closure generated from receiving a peer error message by ChannelManager::handle_error
+	CounterpartyForceClosed {
+		/// The error is coming from the peer, there *might* be a human-readable msg
 		peer_msg: Option<String>,
 	},
+	/// Closure generated from ChannelManager::force_close_channel
+	HolderForceClosed,
 	/// Closure generated from receiving a peer's ClosingSigned message. Note the shutdown
 	/// sequence might have been initially initiated by us.
 	CooperativeClosure,
@@ -89,73 +91,14 @@ pub enum ClosureDescriptor {
 	DisconnectedPeer,
 }
 
-impl Writeable for ClosureDescriptor {
-	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
-		match self {
-			ClosureDescriptor::ForceClosed { peer_msg } => {
-				0u8.write(writer)?;
-				if let Some(peer_msg) = peer_msg {
-					0u8.write(writer)?;
-					let bytes = peer_msg.clone().into_bytes();
-					(bytes.len() as u64).write(writer)?;
-					for b in bytes.iter() {
-						b.write(writer)?;
-					}
-				} else {
-					1u8.write(writer)?;
-				}
-			}
-			ClosureDescriptor::CooperativeClosure => 1u8.write(writer)?,
-			ClosureDescriptor::CommitmentTxBroadcasted => 2u8.write(writer)?,
-			ClosureDescriptor::ProcessingError { err } => {
-				3u8.write(writer)?;
-				let bytes = err.clone().into_bytes();
-				(bytes.len() as u64).write(writer)?;
-				for b in bytes.iter() {
-					b.write(writer)?;
-				}
-			},
-			ClosureDescriptor::DisconnectedPeer => 4u8.write(writer)?,
-		}
-		Ok(())
-	}
-}
-
-impl Readable for ClosureDescriptor {
-	fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
-		Ok(match <u8 as Readable>::read(reader)? {
-			0 => {
-				let peer_msg = match <u8 as Readable>::read(reader)? {
-					0 => {
-						let bytes_len: u64 = Readable::read(reader)?;
-						let mut bytes: Vec<u8> = Vec::with_capacity(bytes_len as usize);
-						for _ in 0..bytes_len {
-							bytes.push(Readable::read(reader)?);
-						}
-						let err = String::from_utf8(bytes).unwrap();
-						Some(err)
-					},
-					1 => None,
-					_ => return Err(DecodeError::InvalidValue),
-				};
-				ClosureDescriptor::ForceClosed { peer_msg }
-			},
-			1 => ClosureDescriptor::CooperativeClosure,
-			2 => ClosureDescriptor::CommitmentTxBroadcasted,
-			3 => {
-				let bytes_len: u64 = Readable::read(reader)?;
-				let mut bytes: Vec<u8> = Vec::with_capacity(bytes_len as usize);
-				for _ in 0..bytes_len {
-					bytes.push(Readable::read(reader)?);
-				}
-				let err = String::from_utf8(bytes).unwrap();
-				ClosureDescriptor::ProcessingError { err }
-			},
-			4 => ClosureDescriptor::DisconnectedPeer,
-			_ => return Err(DecodeError::InvalidValue),
-		})
-	}
-}
+impl_writeable_tlv_based_enum_upgradable!(ClosureReason,
+	(0, CounterpartyForceClosed) => { (1, peer_msg, option) },
+	(2, HolderForceClosed) => {},
+	(6, CommitmentTxBroadcasted) => {},
+	(4, CooperativeClosure) => {},
+	(8, ProcessingError) => { (1, err, required) },
+	(10, DisconnectedPeer) => {},
+);
 
 /// An Event which you should probably take some action in response to.
 ///
@@ -263,16 +206,12 @@ pub enum Event {
 		claim_from_onchain_tx: bool,
 	},
 	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
-	/// Note that if you try to force-close multiple times a channel through
-	/// [`ChannelManager::force_close_channel`] before receiving the corresponding monitor
-	/// event for the broadcast of the commitment transaction, multiple `ChannelClosed` events
-	/// can be generated.
 	ChannelClosed  {
 		/// The channel_id which has been barren from further off-chain updates but
 		/// funding output might still be not resolved yet.
 		channel_id: [u8; 32],
 		/// A machine-readable error message
-		err: ClosureDescriptor
+		err: ClosureReason
 	}
 }
 
@@ -349,7 +288,7 @@ impl Writeable for Event {
 				});
 			},
 			&Event::ChannelClosed { ref channel_id, ref err } => {
-				9u8.write(writer)?;
+				8u8.write(writer)?;
 				channel_id.write(writer)?;
 				err.write(writer)?;
 				write_tlv_fields!(writer, {});
@@ -461,11 +400,12 @@ impl MaybeReadable for Event {
 				};
 				f()
 			},
-			9u8 => {
+			8u8 => {
 				let channel_id = Readable::read(reader)?;
-				let err = Readable::read(reader)?;
+				let err = MaybeReadable::read(reader)?;
 				read_tlv_fields!(reader, {});
-				Ok(Some(Event::ChannelClosed { channel_id, err}))
+				if err.is_none() { return Ok(None); }
+				Ok(Some(Event::ChannelClosed { channel_id, err: err.unwrap() }))
 			},
 			// Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue.
 			x if x % 2 == 1 => Ok(None),

lightning/src/util/ser.rs

@@ -867,3 +867,20 @@ impl Readable for () {
 		Ok(())
 	}
 }
+
+impl Writeable for String {
+	#[inline]
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		(self.len() as u16).write(w)?;
+		w.write_all(self.as_bytes())
+	}
+}
+
+impl Readable for String {
+	#[inline]
+	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+		let v: Vec<u8> = Readable::read(r)?;
+		let ret = String::from_utf8(v).map_err(|_| DecodeError::InvalidValue)?;
+		Ok(ret)
+	}
+}