Skip to content

Commit 62ca481

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 a3f0cdf commit 62ca481

File tree

6 files changed

+64
-92
lines changed

6 files changed

+64
-92
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 & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -702,8 +702,8 @@ mod tests {
702702
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
703703
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
704704
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
705-
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
706-
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
705+
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
706+
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
707707

708708
}
709709
}

‎src/ln/channelmanager.rs

Lines changed: 45 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -944,10 +944,14 @@ impl ChannelManager {
944944
}
945945

946946
let mut events = self.pending_events.lock().unwrap();
947-
events.push(events::Event::SendHTLCs {
947+
events.push(events::Event::UpdateHTLCs {
948948
node_id: first_hop_node_id,
949-
msgs: vec![update_add],
950-
commitment_msg: commitment_signed,
949+
updates: msgs::CommitmentUpdate {
950+
update_add_htlcs: vec![update_add],
951+
update_fulfill_htlcs: Vec::new(),
952+
update_fail_htlcs: Vec::new(),
953+
commitment_signed,
954+
},
951955
});
952956
Ok(())
953957
}
@@ -1084,10 +1088,14 @@ impl ChannelManager {
10841088
continue;
10851089
},
10861090
};
1087-
new_events.push((Some(monitor), events::Event::SendHTLCs {
1091+
new_events.push((Some(monitor), events::Event::UpdateHTLCs {
10881092
node_id: forward_chan.get_their_node_id(),
1089-
msgs: add_htlc_msgs,
1090-
commitment_msg: commitment_msg,
1093+
updates: msgs::CommitmentUpdate {
1094+
update_add_htlcs: add_htlc_msgs,
1095+
update_fulfill_htlcs: Vec::new(),
1096+
update_fail_htlcs: Vec::new(),
1097+
commitment_signed: commitment_msg,
1098+
},
10911099
}));
10921100
}
10931101
} else {
@@ -1203,11 +1211,14 @@ impl ChannelManager {
12031211
}
12041212

12051213
let mut pending_events = self.pending_events.lock().unwrap();
1206-
//TODO: replace by HandleError ? UpdateFailHTLC in handle_update_add_htlc need also to build a CommitmentSigned
1207-
pending_events.push(events::Event::SendFailHTLC {
1214+
pending_events.push(events::Event::UpdateHTLCs {
12081215
node_id,
1209-
msg: msg,
1210-
commitment_msg: commitment_msg,
1216+
updates: msgs::CommitmentUpdate {
1217+
update_add_htlcs: Vec::new(),
1218+
update_fulfill_htlcs: Vec::new(),
1219+
update_fail_htlcs: vec![msg],
1220+
commitment_signed: commitment_msg,
1221+
},
12111222
});
12121223
},
12131224
None => {},
@@ -1299,10 +1310,14 @@ impl ChannelManager {
12991310

13001311
if let Some((msg, commitment_msg)) = fulfill_msgs.0 {
13011312
let mut pending_events = self.pending_events.lock().unwrap();
1302-
pending_events.push(events::Event::SendFulfillHTLC {
1313+
pending_events.push(events::Event::UpdateHTLCs {
13031314
node_id: node_id,
1304-
msg,
1305-
commitment_msg,
1315+
updates: msgs::CommitmentUpdate {
1316+
update_add_htlcs: Vec::new(),
1317+
update_fulfill_htlcs: vec![msg],
1318+
update_fail_htlcs: Vec::new(),
1319+
commitment_signed: commitment_msg,
1320+
}
13061321
});
13071322
}
13081323
true
@@ -2453,8 +2468,10 @@ mod tests {
24532468
impl SendEvent {
24542469
fn from_event(event: Event) -> SendEvent {
24552470
match event {
2456-
Event::SendHTLCs { node_id, msgs, commitment_msg } => {
2457-
SendEvent { node_id: node_id, msgs: msgs, commitment_msg: commitment_msg }
2471+
Event::UpdateHTLCs { node_id, updates: msgs::CommitmentUpdate { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, commitment_signed } } => {
2472+
assert!(update_fulfill_htlcs.is_empty());
2473+
assert!(update_fail_htlcs.is_empty());
2474+
SendEvent { node_id: node_id, msgs: update_add_htlcs, commitment_msg: commitment_signed }
24582475
},
24592476
_ => panic!("Unexpected event type!"),
24602477
}
@@ -2610,9 +2627,12 @@ mod tests {
26102627
let events = node.node.get_and_clear_pending_events();
26112628
assert_eq!(events.len(), 1);
26122629
match events[0] {
2613-
Event::SendFulfillHTLC { ref node_id, ref msg, ref commitment_msg } => {
2630+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => {
2631+
assert!(update_add_htlcs.is_empty());
2632+
assert_eq!(update_fulfill_htlcs.len(), 1);
2633+
assert!(update_fail_htlcs.is_empty());
26142634
expected_next_node = node_id.clone();
2615-
next_msgs = Some((msg.clone(), commitment_msg.clone()));
2635+
next_msgs = Some((update_fulfill_htlcs[0].clone(), commitment_signed.clone()));
26162636
},
26172637
_ => panic!("Unexpected event"),
26182638
};
@@ -2731,9 +2751,12 @@ mod tests {
27312751
let events = node.node.get_and_clear_pending_events();
27322752
assert_eq!(events.len(), 1);
27332753
match events[0] {
2734-
Event::SendFailHTLC { ref node_id, ref msg, ref commitment_msg } => {
2754+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => {
2755+
assert!(update_add_htlcs.is_empty());
2756+
assert!(update_fulfill_htlcs.is_empty());
2757+
assert_eq!(update_fail_htlcs.len(), 1);
27352758
expected_next_node = node_id.clone();
2736-
next_msgs = Some((msg.clone(), commitment_msg.clone()));
2759+
next_msgs = Some((update_fail_htlcs[0].clone(), commitment_signed.clone()));
27372760
},
27382761
_ => panic!("Unexpected event"),
27392762
};
@@ -3049,7 +3072,9 @@ mod tests {
30493072
let events = $node.node.get_and_clear_pending_events();
30503073
assert_eq!(events.len(), 1);
30513074
match events[0] {
3052-
Event::SendFulfillHTLC { ref node_id, .. } => {
3075+
Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, .. } } => {
3076+
assert!(update_add_htlcs.is_empty());
3077+
assert!(update_fail_htlcs.is_empty());
30533078
assert_eq!(*node_id, $prev_node.node.get_our_node_id());
30543079
},
30553080
_ => 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)