From: Matt Corallo Date: Wed, 22 Aug 2018 16:09:11 +0000 (-0400) Subject: Merge HTLC-update events, remove FailHTLC ErrorAction X-Git-Tag: v0.0.12~341^2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=ab00e4ccffba53f48de147d048ee7107fbb773ca;p=rust-lightning 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. --- diff --git a/fuzz/fuzz_targets/channel_target.rs b/fuzz/fuzz_targets/channel_target.rs index 37cf11a8..04a3cacc 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 0205d8a2..e5c12539 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 12b784ca..c29635b8 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 5d476772..c3b99756 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 f674b89f..4ab03974 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 589c4be1..0dee714d 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 {