From: Matt Corallo Date: Fri, 19 Oct 2018 21:30:52 +0000 (-0400) Subject: Send announcement_signatures msgs out-of-band for ordered delivery X-Git-Tag: v0.0.12~285^2~5 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=e382a7b4b3e5f3e59a9300b9d8a4d8bff06366fe;p=rust-lightning Send announcement_signatures msgs out-of-band for ordered delivery --- diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 91bfec960..4777a3e81 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -1735,8 +1735,9 @@ impl ChannelManager { Ok(()) } - fn internal_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result, MsgHandleErrInternal> { - let mut channel_state = self.channel_state.lock().unwrap(); + fn internal_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<(), MsgHandleErrInternal> { + let mut channel_state_lock = self.channel_state.lock().unwrap(); + let channel_state = channel_state_lock.borrow_parts(); match channel_state.by_id.get_mut(&msg.channel_id) { Some(chan) => { if chan.get_their_node_id() != *their_node_id { @@ -1745,10 +1746,16 @@ impl ChannelManager { } chan.funding_locked(&msg) .map_err(|e| MsgHandleErrInternal::from_chan_maybe_close(e, msg.channel_id))?; - return Ok(self.get_announcement_sigs(chan)); + if let Some(announcement_sigs) = self.get_announcement_sigs(chan) { + channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures { + node_id: their_node_id.clone(), + msg: announcement_sigs, + }); + } + Ok(()) }, - None => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id)) - }; + None => Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id)) + } } fn internal_shutdown(&self, their_node_id: &PublicKey, msg: &msgs::Shutdown) -> Result<(Option, Option), MsgHandleErrInternal> { @@ -2336,12 +2343,16 @@ impl ChainListener for ChannelManager { channel_state.by_id.retain(|_, channel| { let chan_res = channel.block_connected(header, height, txn_matched, indexes_of_txn_matched); if let Ok(Some(funding_locked)) = chan_res { - let announcement_sigs = self.get_announcement_sigs(channel); pending_msg_events.push(events::MessageSendEvent::SendFundingLocked { node_id: channel.get_their_node_id(), msg: funding_locked, - announcement_sigs: announcement_sigs }); + if let Some(announcement_sigs) = self.get_announcement_sigs(channel) { + pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures { + node_id: channel.get_their_node_id(), + msg: announcement_sigs, + }); + } short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id()); } else if let Err(e) = chan_res { pending_msg_events.push(events::MessageSendEvent::HandleError { @@ -2480,7 +2491,7 @@ impl ChannelMessageHandler for ChannelManager { handle_error!(self, self.internal_funding_signed(their_node_id, msg), their_node_id) } - fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result, HandleError> { + fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &msgs::FundingLocked) -> Result<(), HandleError> { handle_error!(self, self.internal_funding_locked(their_node_id, msg), their_node_id) } @@ -2929,30 +2940,27 @@ mod tests { fn create_chan_between_nodes_with_value_confirm(node_a: &Node, node_b: &Node, tx: &Transaction) -> ((msgs::FundingLocked, msgs::AnnouncementSignatures), [u8; 32]) { confirm_transaction(&node_b.chain_monitor, &tx, tx.version); - let events_5 = node_b.node.get_and_clear_pending_msg_events(); - assert_eq!(events_5.len(), 1); - match events_5[0] { - MessageSendEvent::SendFundingLocked { ref node_id, ref msg, ref announcement_sigs } => { - assert_eq!(*node_id, node_a.node.get_our_node_id()); - assert!(announcement_sigs.is_none()); - node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), msg).unwrap() - }, - _ => panic!("Unexpected event"), - }; + node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &get_event_msg!(node_b, MessageSendEvent::SendFundingLocked, node_a.node.get_our_node_id())).unwrap(); let channel_id; confirm_transaction(&node_a.chain_monitor, &tx, tx.version); let events_6 = node_a.node.get_and_clear_pending_msg_events(); - assert_eq!(events_6.len(), 1); - (match events_6[0] { - MessageSendEvent::SendFundingLocked { ref node_id, ref msg, ref announcement_sigs } => { + assert_eq!(events_6.len(), 2); + ((match events_6[0] { + MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => { channel_id = msg.channel_id.clone(); assert_eq!(*node_id, node_b.node.get_our_node_id()); - (msg.clone(), announcement_sigs.clone().unwrap()) + msg.clone() + }, + _ => panic!("Unexpected event"), + }, match events_6[1] { + MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => { + assert_eq!(*node_id, node_b.node.get_our_node_id()); + msg.clone() }, _ => panic!("Unexpected event"), - }, channel_id) + }), channel_id) } fn create_chan_between_nodes_with_value_a(node_a: &Node, node_b: &Node, channel_value: u64, push_msat: u64) -> ((msgs::FundingLocked, msgs::AnnouncementSignatures), [u8; 32], Transaction) { @@ -2962,11 +2970,9 @@ mod tests { } fn create_chan_between_nodes_with_value_b(node_a: &Node, node_b: &Node, as_funding_msgs: &(msgs::FundingLocked, msgs::AnnouncementSignatures)) -> (msgs::ChannelAnnouncement, msgs::ChannelUpdate, msgs::ChannelUpdate) { - let bs_announcement_sigs = { - let bs_announcement_sigs = node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &as_funding_msgs.0).unwrap().unwrap(); - node_b.node.handle_announcement_signatures(&node_a.node.get_our_node_id(), &as_funding_msgs.1).unwrap(); - bs_announcement_sigs - }; + node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &as_funding_msgs.0).unwrap(); + let bs_announcement_sigs = get_event_msg!(node_b, MessageSendEvent::SendAnnouncementSignatures, node_a.node.get_our_node_id()); + node_b.node.handle_announcement_signatures(&node_a.node.get_our_node_id(), &as_funding_msgs.1).unwrap(); let events_7 = node_b.node.get_and_clear_pending_msg_events(); assert_eq!(events_7.len(), 1); @@ -5007,9 +5013,14 @@ mod tests { for chan_msgs in resp_1.drain(..) { if pre_all_htlcs { - let a = node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &chan_msgs.0.unwrap()); - let _announcement_sigs_opt = a.unwrap(); - //TODO: Test announcement_sigs re-sending when we've implemented it + node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap(); + let announcement_event = node_a.node.get_and_clear_pending_msg_events(); + if !announcement_event.is_empty() { + assert_eq!(announcement_event.len(), 1); + if let MessageSendEvent::SendAnnouncementSignatures { .. } = announcement_event[0] { + //TODO: Test announcement_sigs re-sending + } else { panic!("Unexpected event!"); } + } } else { assert!(chan_msgs.0.is_none()); } @@ -5056,8 +5067,14 @@ mod tests { for chan_msgs in resp_2.drain(..) { if pre_all_htlcs { - let _announcement_sigs_opt = node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap(); - //TODO: Test announcement_sigs re-sending when we've implemented it + node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap(); + let announcement_event = node_b.node.get_and_clear_pending_msg_events(); + if !announcement_event.is_empty() { + assert_eq!(announcement_event.len(), 1); + if let MessageSendEvent::SendAnnouncementSignatures { .. } = announcement_event[0] { + //TODO: Test announcement_sigs re-sending + } else { panic!("Unexpected event!"); } + } } else { assert!(chan_msgs.0.is_none()); } @@ -5363,9 +5380,8 @@ mod tests { let events_1 = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events_1.len(), 1); match events_1[0] { - MessageSendEvent::SendFundingLocked { ref node_id, msg: _, ref announcement_sigs } => { + MessageSendEvent::SendFundingLocked { ref node_id, msg: _ } => { assert_eq!(*node_id, nodes[1].node.get_our_node_id()); - assert!(announcement_sigs.is_none()); }, _ => panic!("Unexpected event"), } @@ -5374,9 +5390,8 @@ mod tests { let events_2 = nodes[1].node.get_and_clear_pending_msg_events(); assert_eq!(events_2.len(), 1); match events_2[0] { - MessageSendEvent::SendFundingLocked { ref node_id, msg: _, ref announcement_sigs } => { + MessageSendEvent::SendFundingLocked { ref node_id, msg: _ } => { assert_eq!(*node_id, nodes[0].node.get_our_node_id()); - assert!(announcement_sigs.is_none()); }, _ => panic!("Unexpected event"), } diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index 3ed0cde1f..d01595b72 100644 --- a/src/ln/msgs.rs +++ b/src/ln/msgs.rs @@ -536,7 +536,7 @@ pub trait ChannelMessageHandler : events::MessageSendEventsProvider + Send + Syn /// Handle an incoming funding_signed message from the given peer. fn handle_funding_signed(&self, their_node_id: &PublicKey, msg: &FundingSigned) -> Result<(), HandleError>; /// Handle an incoming funding_locked message from the given peer. - fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &FundingLocked) -> Result, HandleError>; + fn handle_funding_locked(&self, their_node_id: &PublicKey, msg: &FundingLocked) -> Result<(), HandleError>; // Channl close: /// Handle an incoming shutdown message from the given peer. diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index 15cda5342..9a2cee9dd 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -576,11 +576,7 @@ impl PeerManager { }, 36 => { let msg = try_potential_decodeerror!(msgs::FundingLocked::read(&mut reader)); - let resp_option = try_potential_handleerror!(self.message_handler.chan_handler.handle_funding_locked(&peer.their_node_id.unwrap(), &msg)); - match resp_option { - Some(resp) => encode_and_send_msg!(resp, 259), - None => {}, - } + try_potential_handleerror!(self.message_handler.chan_handler.handle_funding_locked(&peer.their_node_id.unwrap(), &msg)); }, 38 => { @@ -828,19 +824,25 @@ impl PeerManager { peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 35))); Self::do_attempt_write_data(&mut descriptor, peer); }, - MessageSendEvent::SendFundingLocked { ref node_id, ref msg, ref announcement_sigs } => { - log_trace!(self, "Handling SendFundingLocked event in peer_handler for node {}{} for channel {}", + MessageSendEvent::SendFundingLocked { ref node_id, ref msg } => { + log_trace!(self, "Handling SendFundingLocked event in peer_handler for node {} for channel {}", log_pubkey!(node_id), - if announcement_sigs.is_some() { " with announcement sigs" } else { "" }, log_bytes!(msg.channel_id)); let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, { //TODO: Do whatever we're gonna do for handling dropped messages }); peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 36))); - match announcement_sigs { - &Some(ref announce_msg) => peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(announce_msg, 259))), - &None => {}, - } + Self::do_attempt_write_data(&mut descriptor, peer); + }, + MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } => { + log_trace!(self, "Handling SendAnnouncementSignatures event in peer_handler for node {} for channel {})", + log_pubkey!(node_id), + log_bytes!(msg.channel_id)); + let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, { + //TODO: generate a DiscardFunding event indicating to the wallet that + //they should just throw away this funding transaction + }); + peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 259))); Self::do_attempt_write_data(&mut descriptor, peer); }, MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => { diff --git a/src/util/events.rs b/src/util/events.rs index 1ba3f68d1..d7312cd91 100644 --- a/src/util/events.rs +++ b/src/util/events.rs @@ -138,8 +138,13 @@ pub enum MessageSendEvent { node_id: PublicKey, /// The funding_locked message which should be sent. msg: msgs::FundingLocked, - /// An optional additional announcement_signatures message which should be sent. - announcement_sigs: Option, + }, + /// Used to indicate that an announcement_signatures message should be sent to the peer with the given node_id. + SendAnnouncementSignatures { + /// The node_id of the node which should receive these message(s) + node_id: PublicKey, + /// The announcement_signatures message which should be sent. + msg: msgs::AnnouncementSignatures, }, /// Used to indicate that a series of HTLC update messages, as well as a commitment_signed /// message should be sent to the peer with the given node_id. diff --git a/src/util/test_utils.rs b/src/util/test_utils.rs index 31aa77be1..f8959dcf1 100644 --- a/src/util/test_utils.rs +++ b/src/util/test_utils.rs @@ -98,7 +98,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler { fn handle_funding_signed(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingSigned) -> Result<(), HandleError> { Err(HandleError { err: "", action: None }) } - fn handle_funding_locked(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingLocked) -> Result, HandleError> { + fn handle_funding_locked(&self, _their_node_id: &PublicKey, _msg: &msgs::FundingLocked) -> Result<(), HandleError> { Err(HandleError { err: "", action: None }) } fn handle_shutdown(&self, _their_node_id: &PublicKey, _msg: &msgs::Shutdown) -> Result<(Option, Option), HandleError> {