From: Matt Corallo Date: Wed, 17 Oct 2018 22:06:13 +0000 (-0400) Subject: Add message ordering return value to handling channel_reestablish X-Git-Tag: v0.0.12~290^2~2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=e86c84b2bebdd8c431bf87616a3d08d63c734be3;hp=270d1bd00636942fcf359b4abdc5681ebe15307d;p=rust-lightning Add message ordering return value to handling channel_reestablish --- diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 464bfc6e..c53cf252 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -14,7 +14,7 @@ use crypto::digest::Digest; use crypto::hkdf::{hkdf_extract,hkdf_expand}; use ln::msgs; -use ln::msgs::{ErrorAction, HandleError}; +use ln::msgs::{ErrorAction, HandleError, RAACommitmentOrder}; use ln::channelmonitor::ChannelMonitor; use ln::channelmanager::{PendingHTLCStatus, HTLCSource, PendingForwardHTLCInfo, HTLCFailReason, HTLCFailureMsg}; use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT}; @@ -2072,7 +2072,7 @@ impl Channel { /// May panic if some calls other than message-handling calls (which will all Err immediately) /// have been called between remove_uncommitted_htlcs_and_mark_paused and this call. - pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, Option), ChannelError> { + pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, Option, RAACommitmentOrder), ChannelError> { if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 { // While BOLT 2 doesn't indicate explicitly we should error this channel here, it // almost certainly indicates we are going to end up out-of-sync in some way, so we @@ -2120,6 +2120,8 @@ impl Channel { }) } else { None }; + let order = RAACommitmentOrder::RevokeAndACKFirst; + if msg.next_local_commitment_number == our_next_remote_commitment_number { if required_revoke.is_some() { log_debug!(self, "Reconnected channel {} with only lost outbound RAA", log_bytes!(self.channel_id())); @@ -2142,11 +2144,11 @@ impl Channel { panic!("Got non-channel-failing result from free_holding_cell_htlcs"); } }, - Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor))), - Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None)), + Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor), order)), + Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, order)), } } else { - return Ok((resend_funding_locked, required_revoke, None, None)); + return Ok((resend_funding_locked, required_revoke, None, None, order)); } } else if msg.next_local_commitment_number == our_next_remote_commitment_number - 1 { if required_revoke.is_some() { @@ -2206,7 +2208,7 @@ impl Channel { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, update_fail_malformed_htlcs, update_fee: None, //TODO: We need to support re-generating any update_fees in the last commitment_signed! commitment_signed: self.send_commitment_no_state_update().expect("It looks like we failed to re-generate a commitment_signed we had previously sent?").0, - }), None)); + }), None, order)); } else { return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction")); } diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 1e396612..e542a4d6 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -26,7 +26,7 @@ use ln::channel::{Channel, ChannelError, ChannelKeys}; use ln::channelmonitor::{ManyChannelMonitor, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS}; use ln::router::{Route,RouteHop}; use ln::msgs; -use ln::msgs::{HandleError,ChannelMessageHandler}; +use ln::msgs::{ChannelMessageHandler, HandleError, RAACommitmentOrder}; use util::{byte_utils, events, internal_traits, rng}; use util::sha2::Sha256; use util::ser::{Readable, Writeable}; @@ -2168,7 +2168,7 @@ impl ChannelManager { Ok(()) } - fn internal_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option), MsgHandleErrInternal> { + fn internal_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, RAACommitmentOrder), MsgHandleErrInternal> { let (res, chan_monitor) = { let mut channel_state = self.channel_state.lock().unwrap(); match channel_state.by_id.get_mut(&msg.channel_id) { @@ -2176,9 +2176,9 @@ impl ChannelManager { if chan.get_their_node_id() != *their_node_id { return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id)); } - let (funding_locked, revoke_and_ack, commitment_update, channel_monitor) = chan.channel_reestablish(msg) + let (funding_locked, revoke_and_ack, commitment_update, channel_monitor, order) = chan.channel_reestablish(msg) .map_err(|e| MsgHandleErrInternal::from_chan_maybe_close(e, msg.channel_id))?; - (Ok((funding_locked, revoke_and_ack, commitment_update)), channel_monitor) + (Ok((funding_locked, revoke_and_ack, commitment_update, order)), channel_monitor) }, None => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id)) } @@ -2448,7 +2448,7 @@ impl ChannelMessageHandler for ChannelManager { handle_error!(self, self.internal_announcement_signatures(their_node_id, msg), their_node_id) } - fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option), HandleError> { + fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, RAACommitmentOrder), HandleError> { handle_error!(self, self.internal_channel_reestablish(their_node_id, msg), their_node_id) } @@ -4938,6 +4938,7 @@ mod tests { assert!(chan_msgs.0.is_none()); } if pending_raa.0 { + assert!(chan_msgs.3 == msgs::RAACommitmentOrder::RevokeAndACKFirst); assert!(node_a.node.handle_revoke_and_ack(&node_b.node.get_our_node_id(), &chan_msgs.1.unwrap()).unwrap().is_none()); check_added_monitors!(node_a, 1); } else { @@ -4985,6 +4986,7 @@ mod tests { assert!(chan_msgs.0.is_none()); } if pending_raa.1 { + assert!(chan_msgs.3 == msgs::RAACommitmentOrder::RevokeAndACKFirst); assert!(node_b.node.handle_revoke_and_ack(&node_a.node.get_our_node_id(), &chan_msgs.1.unwrap()).unwrap().is_none()); check_added_monitors!(node_b, 1); } else { diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index 7b0254bd..bab2674e 100644 --- a/src/ln/msgs.rs +++ b/src/ln/msgs.rs @@ -500,6 +500,18 @@ pub enum HTLCFailChannelUpdate { } } +/// For events which result in both a RevokeAndACK and a CommitmentUpdate, by default they should +/// be sent in the order they appear in the return value, however sometimes the order needs to be +/// variable at runtime (eg handle_channel_reestablish needs to re-send messages in the order they +/// were originally sent). In those cases, this enum is also returned. +#[derive(Clone, PartialEq)] +pub enum RAACommitmentOrder { + /// Send the CommitmentUpdate messages first + CommitmentFirst, + /// Send the RevokeAndACK message first + RevokeAndACKFirst, +} + /// A trait to describe an object which can receive channel messages. /// /// Messages MAY be called in parallel when they originate from different their_node_ids, however @@ -554,7 +566,7 @@ pub trait ChannelMessageHandler : events::EventsProvider + Send + Sync { /// Handle a peer reconnecting, possibly generating channel_reestablish message(s). fn peer_connected(&self, their_node_id: &PublicKey) -> Vec; /// Handle an incoming channel_reestablish message from the given peer. - fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &ChannelReestablish) -> Result<(Option, Option, Option), HandleError>; + fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &ChannelReestablish) -> Result<(Option, Option, Option, RAACommitmentOrder), HandleError>; // Error: /// Handle an incoming error message from the given peer. diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index 81ef41cf..b629e7fa 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -658,30 +658,44 @@ impl PeerManager { }, 136 => { let msg = try_potential_decodeerror!(msgs::ChannelReestablish::read(&mut reader)); - let (funding_locked, revoke_and_ack, commitment_update) = try_potential_handleerror!(self.message_handler.chan_handler.handle_channel_reestablish(&peer.their_node_id.unwrap(), &msg)); + let (funding_locked, revoke_and_ack, commitment_update, order) = try_potential_handleerror!(self.message_handler.chan_handler.handle_channel_reestablish(&peer.their_node_id.unwrap(), &msg)); if let Some(lock_msg) = funding_locked { encode_and_send_msg!(lock_msg, 36); } - if let Some(revoke_msg) = revoke_and_ack { - encode_and_send_msg!(revoke_msg, 133); - } - match commitment_update { - Some(resps) => { - for resp in resps.update_add_htlcs { - encode_and_send_msg!(resp, 128); - } - for resp in resps.update_fulfill_htlcs { - encode_and_send_msg!(resp, 130); - } - for resp in resps.update_fail_htlcs { - encode_and_send_msg!(resp, 131); - } - if let Some(resp) = resps.update_fee { - encode_and_send_msg!(resp, 134); - } - encode_and_send_msg!(resps.commitment_signed, 132); + macro_rules! handle_raa { () => { + if let Some(revoke_msg) = revoke_and_ack { + encode_and_send_msg!(revoke_msg, 133); + } + } } + macro_rules! handle_cu { () => { + match commitment_update { + Some(resps) => { + for resp in resps.update_add_htlcs { + encode_and_send_msg!(resp, 128); + } + for resp in resps.update_fulfill_htlcs { + encode_and_send_msg!(resp, 130); + } + for resp in resps.update_fail_htlcs { + encode_and_send_msg!(resp, 131); + } + if let Some(resp) = resps.update_fee { + encode_and_send_msg!(resp, 134); + } + encode_and_send_msg!(resps.commitment_signed, 132); + }, + None => {}, + } + } } + match order { + msgs::RAACommitmentOrder::RevokeAndACKFirst => { + handle_raa!(); + handle_cu!(); + }, + msgs::RAACommitmentOrder::CommitmentFirst => { + handle_cu!(); + handle_raa!(); }, - None => {}, } }, diff --git a/src/util/test_utils.rs b/src/util/test_utils.rs index f8341e88..8ab02b26 100644 --- a/src/util/test_utils.rs +++ b/src/util/test_utils.rs @@ -128,7 +128,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler { fn handle_announcement_signatures(&self, _their_node_id: &PublicKey, _msg: &msgs::AnnouncementSignatures) -> Result<(), HandleError> { Err(HandleError { err: "", action: None }) } - fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option), HandleError> { + fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelReestablish) -> Result<(Option, Option, Option, msgs::RAACommitmentOrder), HandleError> { Err(HandleError { err: "", action: None }) } fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {}