Skip to content

Commit 6d9085b

Browse files
TheBlueMattTheBlueMatt
committed
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.
1 parent 9c043d4 commit 6d9085b

File tree

6 files changed

+64
-93
lines changed

6 files changed

+64
-93
lines changed

‎fuzz/fuzz_targets/channel_target.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,6 @@ pub fn do_test(data: &[u8]) {
256256
Ok(r) => Some(r),
257257
Err(e) => match e.action {
258258
None => return,
259-
Some(ErrorAction::UpdateFailHTLC {..}) => None,
260259
Some(ErrorAction::DisconnectPeer {..}) => return,
261260
Some(ErrorAction::IgnoreError) => None,
262261
Some(ErrorAction::SendErrorMessage {..}) => None,

‎fuzz/fuzz_targets/full_stack_target.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -705,8 +705,7 @@ mod tests {
705705
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
706706
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
707707
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
708-
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
709-
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
710-
708+
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
709+
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
711710
}
712711
}

‎src/ln/channelmanager.rs

Lines changed: 45 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -952,10 +952,14 @@ impl ChannelManager {
952952
}
953953

954954
let mut events = self.pending_events.lock().unwrap();
955-
events.push(events::Event::SendHTLCs {
955+
events.push(events::Event::UpdateHTLCs {
956956
node_id: first_hop_node_id,
957-
msgs: vec![update_add],
958-
commitment_msg: commitment_signed,
957+
updates: msgs::CommitmentUpdate {
958+
update_add_htlcs: vec![update_add],
959+
update_fulfill_htlcs: Vec::new(),
960+
update_fail_htlcs: Vec::new(),
961+
commitment_signed,
962+
},
959963
});
960964
Ok(())
961965
}
@@ -1092,10 +1096,14 @@ impl ChannelManager {
10921096
continue;
10931097
},
10941098
};
1095-
new_events.push((Some(monitor), events::Event::SendHTLCs {
1099+
new_events.push((Some(monitor), events::Event::UpdateHTLCs {
10961100
node_id: forward_chan.get_their_node_id(),
1097-
msgs: add_htlc_msgs,
1098-
commitment_msg: commitment_msg,
1101+
updates: msgs::CommitmentUpdate {
1102+
update_add_htlcs: add_htlc_msgs,
1103+
update_fulfill_htlcs: Vec::new(),
1104+
update_fail_htlcs: Vec::new(),
1105+
commitment_signed: commitment_msg,
1106+
},
10991107
}));
11001108
}
11011109
} else {
@@ -1211,11 +1219,14 @@ impl ChannelManager {
12111219
}
12121220

12131221
let mut pending_events = self.pending_events.lock().unwrap();
1214-
//TODO: replace by HandleError ? UpdateFailHTLC in handle_update_add_htlc need also to build a CommitmentSigned
1215-
pending_events.push(events::Event::SendFailHTLC {
1222+
pending_events.push(events::Event::UpdateHTLCs {
12161223
node_id,
1217-
msg: msg,
1218-
commitment_msg: commitment_msg,
1224+
updates: msgs::CommitmentUpdate {
1225+
update_add_htlcs: Vec::new(),
1226+
update_fulfill_htlcs: Vec::new(),
1227+
update_fail_htlcs: vec![msg],
1228+
commitment_signed: commitment_msg,
1229+
},
12191230
});
12201231
},
12211232
None => {},
@@ -1307,10 +1318,14 @@ impl ChannelManager {
13071318

13081319
if let Some((msg, commitment_msg)) = fulfill_msgs.0 {
13091320
let mut pending_events = self.pending_events.lock().unwrap();
1310-
pending_events.push(events::Event::SendFulfillHTLC {
1321+
pending_events.push(events::Event::UpdateHTLCs {
13111322
node_id: node_id,
1312-
msg,
1313-
commitment_msg,
1323+
updates: msgs::CommitmentUpdate {
1324+
update_add_htlcs: Vec::new(),
1325+
update_fulfill_htlcs: vec![msg],
1326+
update_fail_htlcs: Vec::new(),
1327+
commitment_signed: commitment_msg,
1328+
}
13141329
});
13151330
}
13161331
true
@@ -2461,8 +2476,10 @@ mod tests {
24612476
impl SendEvent {
24622477
fn from_event(event: Event) -> SendEvent {
24632478
match event {
2464-
Event::SendHTLCs { node_id, msgs, commitment_msg } => {
2465-
SendEvent { node_id: node_id, msgs: msgs, commitment_msg: commitment_msg }
2479+
Event::UpdateHTLCs { node_id, updates: msgs::CommitmentUpdate { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, commitment_signed } } => {
2480+
assert!(update_fulfill_htlcs.is_empty());
2481+
assert!(update_fail_htlcs.is_empty());
2482+
SendEvent { node_id: node_id, msgs: update_add_htlcs, commitment_msg: commitment_signed }
24662483
},
24672484
_ => panic!("Unexpected event type!"),
24682485
}
@@ -2618,9 +2635,12 @@ mod tests {
26182635
let events = node.node.get_and_clear_pending_events();
26192636
assert_eq!(events.len(), 1);
26202637
match events[0] {
2621-
Event::SendFulfillHTLC { ref node_id, ref msg, ref commitment_msg } => {
2638+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => {
2639+
assert!(update_add_htlcs.is_empty());
2640+
assert_eq!(update_fulfill_htlcs.len(), 1);
2641+
assert!(update_fail_htlcs.is_empty());
26222642
expected_next_node = node_id.clone();
2623-
next_msgs = Some((msg.clone(), commitment_msg.clone()));
2643+
next_msgs = Some((update_fulfill_htlcs[0].clone(), commitment_signed.clone()));
26242644
},
26252645
_ => panic!("Unexpected event"),
26262646
};
@@ -2739,9 +2759,12 @@ mod tests {
27392759
let events = node.node.get_and_clear_pending_events();
27402760
assert_eq!(events.len(), 1);
27412761
match events[0] {
2742-
Event::SendFailHTLC { ref node_id, ref msg, ref commitment_msg } => {
2762+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => {
2763+
assert!(update_add_htlcs.is_empty());
2764+
assert!(update_fulfill_htlcs.is_empty());
2765+
assert_eq!(update_fail_htlcs.len(), 1);
27432766
expected_next_node = node_id.clone();
2744-
next_msgs = Some((msg.clone(), commitment_msg.clone()));
2767+
next_msgs = Some((update_fail_htlcs[0].clone(), commitment_signed.clone()));
27452768
},
27462769
_ => panic!("Unexpected event"),
27472770
};
@@ -3057,7 +3080,9 @@ mod tests {
30573080
let events = $node.node.get_and_clear_pending_events();
30583081
assert_eq!(events.len(), 1);
30593082
match events[0] {
3060-
Event::SendFulfillHTLC { ref node_id, .. } => {
3083+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, .. } } => {
3084+
assert!(update_add_htlcs.is_empty());
3085+
assert!(update_fail_htlcs.is_empty());
30613086
assert_eq!(*node_id, $prev_node.node.get_our_node_id());
30623087
},
30633088
_ => panic!("Unexpected event"),

‎src/ln/msgs.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -376,11 +376,6 @@ pub struct ChannelUpdate {
376376

377377
/// Used to put an error message in a HandleError
378378
pub enum ErrorAction {
379-
/// Indicates an inbound HTLC add resulted in a failure, and the UpdateFailHTLC provided in msg
380-
/// should be sent back to the sender.
381-
UpdateFailHTLC {
382-
msg: UpdateFailHTLC
383-
},
384379
/// The peer took some action which made us think they were useless. Disconnect them.
385380
DisconnectPeer {
386381
msg: Option<ErrorMessage>

‎src/ln/peer_handler.rs

Lines changed: 14 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -303,10 +303,6 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
303303
Err(e) => {
304304
if let Some(action) = e.action {
305305
match action {
306-
msgs::ErrorAction::UpdateFailHTLC { msg } => {
307-
encode_and_send_msg!(msg, 131);
308-
continue;
309-
},
310306
msgs::ErrorAction::DisconnectPeer { msg: _ } => {
311307
return Err(PeerHandleError{ no_connection_possible: false });
312308
},
@@ -675,44 +671,26 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
675671
Self::do_attempt_write_data(&mut descriptor, peer);
676672
continue;
677673
},
678-
Event::SendHTLCs { ref node_id, ref msgs, ref commitment_msg } => {
679-
log_trace!(self, "Handling SendHTLCs event in peer_handler for node {} with {} HTLCs for channel {}",
674+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => {
675+
log_trace!(self, "Handling UpdateHTLCs event in peer_handler for node {} with {} adds, {} fulfills, {} fails for channel {}",
680676
log_pubkey!(node_id),
681-
msgs.len(),
682-
log_bytes!(commitment_msg.channel_id));
677+
update_add_htlcs.len(),
678+
update_fulfill_htlcs.len(),
679+
update_fail_htlcs.len(),
680+
log_bytes!(commitment_signed.channel_id));
683681
let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
684682
//TODO: Do whatever we're gonna do for handling dropped messages
685683
});
686-
for msg in msgs {
684+
for msg in update_add_htlcs {
687685
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 128)));
688686
}
689-
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_msg, 132)));
690-
Self::do_attempt_write_data(&mut descriptor, peer);
691-
continue;
692-
},
693-
Event::SendFulfillHTLC { ref node_id, ref msg, ref commitment_msg } => {
694-
log_trace!(self, "Handling SendFulfillHTLCs event in peer_handler for node {} with payment_preimage {} for channel {}",
695-
log_pubkey!(node_id),
696-
log_bytes!(msg.payment_preimage),
697-
log_bytes!(msg.channel_id));
698-
let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
699-
//TODO: Do whatever we're gonna do for handling dropped messages
700-
});
701-
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 130)));
702-
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_msg, 132)));
703-
Self::do_attempt_write_data(&mut descriptor, peer);
704-
continue;
705-
},
706-
Event::SendFailHTLC { ref node_id, ref msg, ref commitment_msg } => {
707-
log_trace!(self, "Handling SendFailHTLCs event in peer_handler for node {} for HTLC ID {} for channel {}",
708-
log_pubkey!(node_id),
709-
msg.htlc_id,
710-
log_bytes!(msg.channel_id));
711-
let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
712-
//TODO: Do whatever we're gonna do for handling dropped messages
713-
});
714-
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 131)));
715-
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_msg, 132)));
687+
for msg in update_fulfill_htlcs {
688+
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 130)));
689+
}
690+
for msg in update_fail_htlcs {
691+
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 131)));
692+
}
693+
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_signed, 132)));
716694
Self::do_attempt_write_data(&mut descriptor, peer);
717695
continue;
718696
},
@@ -770,18 +748,6 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
770748
Event::HandleError { ref node_id, ref action } => {
771749
if let Some(ref action) = *action {
772750
match *action {
773-
msgs::ErrorAction::UpdateFailHTLC { ref msg } => {
774-
log_trace!(self, "Handling UpdateFailHTLC HandleError event in peer_handler for node {} for HTLC ID {} for channel {}",
775-
log_pubkey!(node_id),
776-
msg.htlc_id,
777-
log_bytes!(msg.channel_id));
778-
let (mut descriptor, peer) = get_peer_for_forwarding!(node_id, {
779-
//TODO: Do whatever we're gonna do for handling dropped messages
780-
});
781-
peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 131)));
782-
Self::do_attempt_write_data(&mut descriptor, peer);
783-
784-
},
785751
msgs::ErrorAction::DisconnectPeer { ref msg } => {
786752
if let Some(mut descriptor) = peers.node_id_to_descriptor.remove(node_id) {
787753
if let Some(mut peer) = peers.peers.remove(&descriptor) {

‎src/util/events.rs

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -70,24 +70,11 @@ pub enum Event {
7070
msg: msgs::FundingLocked,
7171
announcement_sigs: Option<msgs::AnnouncementSignatures>,
7272
},
73-
/// Used to indicate that a series of update_add_htlc messages, as well as a commitment_signed
73+
/// Used to indicate that a series of HTLC update messages, as well as a commitment_signed
7474
/// message should be sent to the peer with the given node_id.
75-
SendHTLCs {
75+
UpdateHTLCs {
7676
node_id: PublicKey,
77-
msgs: Vec<msgs::UpdateAddHTLC>,
78-
commitment_msg: msgs::CommitmentSigned,
79-
},
80-
/// Used to indicate that we're ready to fulfill an htlc from the peer with the given node_id.
81-
SendFulfillHTLC {
82-
node_id: PublicKey,
83-
msg: msgs::UpdateFulfillHTLC,
84-
commitment_msg: msgs::CommitmentSigned,
85-
},
86-
/// Used to indicate that we need to fail an htlc from the peer with the given node_id.
87-
SendFailHTLC {
88-
node_id: PublicKey,
89-
msg: msgs::UpdateFailHTLC,
90-
commitment_msg: msgs::CommitmentSigned,
77+
updates: msgs::CommitmentUpdate,
9178
},
9279
/// Used to indicate that a shutdown message should be sent to the peer with the given node_id.
9380
SendShutdown {

0 commit comments

Comments
 (0)