From ab00e4ccffba53f48de147d048ee7107fbb773ca Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 22 Aug 2018 12:09:11 -0400 Subject: [PATCH] Merge HTLC-update events, remove FailHTLC ErrorAction UpdateFailHTLC isn't really an error anymore now that its handled async after channel commitment (as required by BOLT 2), and since its unused this is free. To resolve the TODO which intended to use it for HTLC failure when trying to route forwards, we instead opt to merge all the HTLC update events into one UpdateHTLCs event which just contains a CommitmentUpdate object. --- fuzz/fuzz_targets/channel_target.rs | 1 - fuzz/fuzz_targets/full_stack_target.rs | 5 +- src/ln/channelmanager.rs | 65 ++++++++++++++++++-------- src/ln/msgs.rs | 5 -- src/ln/peer_handler.rs | 62 ++++++------------------ src/util/events.rs | 19 ++------ 6 files changed, 64 insertions(+), 93 deletions(-) diff --git a/fuzz/fuzz_targets/channel_target.rs b/fuzz/fuzz_targets/channel_target.rs index 37cf11a8c..04a3cacca 100644 --- a/fuzz/fuzz_targets/channel_target.rs +++ b/fuzz/fuzz_targets/channel_target.rs @@ -256,7 +256,6 @@ pub fn do_test(data: &[u8]) { Ok(r) => Some(r), Err(e) => match e.action { None => return, - Some(ErrorAction::UpdateFailHTLC {..}) => None, Some(ErrorAction::DisconnectPeer {..}) => return, Some(ErrorAction::IgnoreError) => None, Some(ErrorAction::SendErrorMessage {..}) => None, diff --git a/fuzz/fuzz_targets/full_stack_target.rs b/fuzz/fuzz_targets/full_stack_target.rs index 0205d8a22..e5c12539e 100644 --- a/fuzz/fuzz_targets/full_stack_target.rs +++ b/fuzz/fuzz_targets/full_stack_target.rs @@ -703,8 +703,7 @@ mod tests { assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendFundingLocked event in peer_handler for node 030200000000000000000000000000000000000000000000000000000000000000 for channel 3f00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 4 assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Encoding and sending message of type 133 to 030000000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&2)); // 5 assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Encoding and sending message of type 132 to 030000000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&2)); // 6 - assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendHTLCs event in peer_handler for node 030200000000000000000000000000000000000000000000000000000000000000 with 1 HTLCs for channel 3f00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&2)); // 7 - assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling SendFulfillHTLCs event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000000 with payment_preimage ff00888888888888888888888888888888888888888888888888888888888888 for channel 3d00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 8 - + assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling UpdateHTLCs event in peer_handler for node 030200000000000000000000000000000000000000000000000000000000000000 with 1 adds, 0 fulfills, 0 fails for channel 3f00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&2)); // 7 + assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Handling UpdateHTLCs event in peer_handler for node 030000000000000000000000000000000000000000000000000000000000000000 with 0 adds, 1 fulfills, 0 fails for channel 3d00000000000000000000000000000000000000000000000000000000000000".to_string())), Some(&1)); // 8 } } diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 12b784caa..c29635b89 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -952,10 +952,14 @@ impl ChannelManager { } let mut events = self.pending_events.lock().unwrap(); - events.push(events::Event::SendHTLCs { + events.push(events::Event::UpdateHTLCs { node_id: first_hop_node_id, - msgs: vec![update_add], - commitment_msg: commitment_signed, + updates: msgs::CommitmentUpdate { + update_add_htlcs: vec![update_add], + update_fulfill_htlcs: Vec::new(), + update_fail_htlcs: Vec::new(), + commitment_signed, + }, }); Ok(()) } @@ -1092,10 +1096,14 @@ impl ChannelManager { continue; }, }; - new_events.push((Some(monitor), events::Event::SendHTLCs { + new_events.push((Some(monitor), events::Event::UpdateHTLCs { node_id: forward_chan.get_their_node_id(), - msgs: add_htlc_msgs, - commitment_msg: commitment_msg, + updates: msgs::CommitmentUpdate { + update_add_htlcs: add_htlc_msgs, + update_fulfill_htlcs: Vec::new(), + update_fail_htlcs: Vec::new(), + commitment_signed: commitment_msg, + }, })); } } else { @@ -1211,11 +1219,14 @@ impl ChannelManager { } let mut pending_events = self.pending_events.lock().unwrap(); - //TODO: replace by HandleError ? UpdateFailHTLC in handle_update_add_htlc need also to build a CommitmentSigned - pending_events.push(events::Event::SendFailHTLC { + pending_events.push(events::Event::UpdateHTLCs { node_id, - msg: msg, - commitment_msg: commitment_msg, + updates: msgs::CommitmentUpdate { + update_add_htlcs: Vec::new(), + update_fulfill_htlcs: Vec::new(), + update_fail_htlcs: vec![msg], + commitment_signed: commitment_msg, + }, }); }, None => {}, @@ -1307,10 +1318,14 @@ impl ChannelManager { if let Some((msg, commitment_msg)) = fulfill_msgs.0 { let mut pending_events = self.pending_events.lock().unwrap(); - pending_events.push(events::Event::SendFulfillHTLC { + pending_events.push(events::Event::UpdateHTLCs { node_id: node_id, - msg, - commitment_msg, + updates: msgs::CommitmentUpdate { + update_add_htlcs: Vec::new(), + update_fulfill_htlcs: vec![msg], + update_fail_htlcs: Vec::new(), + commitment_signed: commitment_msg, + } }); } true @@ -2461,8 +2476,10 @@ mod tests { impl SendEvent { fn from_event(event: Event) -> SendEvent { match event { - Event::SendHTLCs { node_id, msgs, commitment_msg } => { - SendEvent { node_id: node_id, msgs: msgs, commitment_msg: commitment_msg } + Event::UpdateHTLCs { node_id, updates: msgs::CommitmentUpdate { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, commitment_signed } } => { + assert!(update_fulfill_htlcs.is_empty()); + assert!(update_fail_htlcs.is_empty()); + SendEvent { node_id: node_id, msgs: update_add_htlcs, commitment_msg: commitment_signed } }, _ => panic!("Unexpected event type!"), } @@ -2618,9 +2635,12 @@ mod tests { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::SendFulfillHTLC { ref node_id, ref msg, ref commitment_msg } => { + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => { + assert!(update_add_htlcs.is_empty()); + assert_eq!(update_fulfill_htlcs.len(), 1); + assert!(update_fail_htlcs.is_empty()); expected_next_node = node_id.clone(); - next_msgs = Some((msg.clone(), commitment_msg.clone())); + next_msgs = Some((update_fulfill_htlcs[0].clone(), commitment_signed.clone())); }, _ => panic!("Unexpected event"), }; @@ -2739,9 +2759,12 @@ mod tests { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::SendFailHTLC { ref node_id, ref msg, ref commitment_msg } => { + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => { + assert!(update_add_htlcs.is_empty()); + assert!(update_fulfill_htlcs.is_empty()); + assert_eq!(update_fail_htlcs.len(), 1); expected_next_node = node_id.clone(); - next_msgs = Some((msg.clone(), commitment_msg.clone())); + next_msgs = Some((update_fail_htlcs[0].clone(), commitment_signed.clone())); }, _ => panic!("Unexpected event"), }; @@ -3057,7 +3080,9 @@ mod tests { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::SendFulfillHTLC { ref node_id, .. } => { + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, .. } } => { + assert!(update_add_htlcs.is_empty()); + assert!(update_fail_htlcs.is_empty()); assert_eq!(*node_id, $prev_node.node.get_our_node_id()); }, _ => panic!("Unexpected event"), diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index 5d4767721..c3b997566 100644 --- a/src/ln/msgs.rs +++ b/src/ln/msgs.rs @@ -376,11 +376,6 @@ pub struct ChannelUpdate { /// Used to put an error message in a HandleError pub enum ErrorAction { - /// Indicates an inbound HTLC add resulted in a failure, and the UpdateFailHTLC provided in msg - /// should be sent back to the sender. - UpdateFailHTLC { - msg: UpdateFailHTLC - }, /// The peer took some action which made us think they were useless. Disconnect them. DisconnectPeer { msg: Option diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index f674b89fa..4ab039741 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -303,10 +303,6 @@ impl PeerManager { Err(e) => { if let Some(action) = e.action { match action { - msgs::ErrorAction::UpdateFailHTLC { msg } => { - encode_and_send_msg!(msg, 131); - continue; - }, msgs::ErrorAction::DisconnectPeer { msg: _ } => { //TODO: Try to push msg log_trace!(self, "Got Err handling message, disconnecting peer because {}", e.err); @@ -680,44 +676,26 @@ impl PeerManager { Self::do_attempt_write_data(&mut descriptor, peer); continue; }, - Event::SendHTLCs { ref node_id, ref msgs, ref commitment_msg } => { - log_trace!(self, "Handling SendHTLCs event in peer_handler for node {} with {} HTLCs for channel {}", + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => { + log_trace!(self, "Handling UpdateHTLCs event in peer_handler for node {} with {} adds, {} fulfills, {} fails for channel {}", log_pubkey!(node_id), - msgs.len(), - log_bytes!(commitment_msg.channel_id)); + update_add_htlcs.len(), + update_fulfill_htlcs.len(), + update_fail_htlcs.len(), + log_bytes!(commitment_signed.channel_id)); let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, { //TODO: Do whatever we're gonna do for handling dropped messages }); - for msg in msgs { + for msg in update_add_htlcs { peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 128))); } - peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_msg, 132))); - Self::do_attempt_write_data(&mut descriptor, peer); - continue; - }, - Event::SendFulfillHTLC { ref node_id, ref msg, ref commitment_msg } => { - log_trace!(self, "Handling SendFulfillHTLCs event in peer_handler for node {} with payment_preimage {} for channel {}", - log_pubkey!(node_id), - log_bytes!(msg.payment_preimage), - 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, 130))); - peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_msg, 132))); - Self::do_attempt_write_data(&mut descriptor, peer); - continue; - }, - Event::SendFailHTLC { ref node_id, ref msg, ref commitment_msg } => { - log_trace!(self, "Handling SendFailHTLCs event in peer_handler for node {} for HTLC ID {} for channel {}", - log_pubkey!(node_id), - msg.htlc_id, - 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, 131))); - peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_msg, 132))); + for msg in update_fulfill_htlcs { + peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 130))); + } + for msg in update_fail_htlcs { + peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 131))); + } + peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_signed, 132))); Self::do_attempt_write_data(&mut descriptor, peer); continue; }, @@ -775,18 +753,6 @@ impl PeerManager { Event::HandleError { ref node_id, ref action } => { if let Some(ref action) = *action { match *action { - msgs::ErrorAction::UpdateFailHTLC { ref msg } => { - log_trace!(self, "Handling UpdateFailHTLC HandleError event in peer_handler for node {} for HTLC ID {} for channel {}", - log_pubkey!(node_id), - msg.htlc_id, - 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, 131))); - Self::do_attempt_write_data(&mut descriptor, peer); - - }, msgs::ErrorAction::DisconnectPeer { ref msg } => { if let Some(mut descriptor) = peers.node_id_to_descriptor.remove(node_id) { if let Some(mut peer) = peers.peers.remove(&descriptor) { diff --git a/src/util/events.rs b/src/util/events.rs index 589c4be10..0dee714d2 100644 --- a/src/util/events.rs +++ b/src/util/events.rs @@ -70,24 +70,11 @@ pub enum Event { msg: msgs::FundingLocked, announcement_sigs: Option, }, - /// Used to indicate that a series of update_add_htlc messages, as well as a commitment_signed + /// 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. - SendHTLCs { + UpdateHTLCs { node_id: PublicKey, - msgs: Vec, - commitment_msg: msgs::CommitmentSigned, - }, - /// Used to indicate that we're ready to fulfill an htlc from the peer with the given node_id. - SendFulfillHTLC { - node_id: PublicKey, - msg: msgs::UpdateFulfillHTLC, - commitment_msg: msgs::CommitmentSigned, - }, - /// Used to indicate that we need to fail an htlc from the peer with the given node_id. - SendFailHTLC { - node_id: PublicKey, - msg: msgs::UpdateFailHTLC, - commitment_msg: msgs::CommitmentSigned, + updates: msgs::CommitmentUpdate, }, /// Used to indicate that a shutdown message should be sent to the peer with the given node_id. SendShutdown { -- 2.39.5