Skip to content

Commit b14baa0

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #253 from TheBlueMatt/2018-11-misc-tweaks
Misc Tweaks
2 parents 2258d2b + 9687203 commit b14baa0

File tree

2 files changed

+55
-33
lines changed

2 files changed

+55
-33
lines changed

‎src/ln/channel.rs

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1120,8 +1120,9 @@ impl Channel {
11201120
Ok(our_sig)
11211121
}
11221122

1123-
/// May return an IgnoreError, but should not, and will always return Ok(_) when
1124-
/// debug_assertions are turned on
1123+
/// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
1124+
/// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
1125+
/// Ok(_) if debug assertions are turned on and preconditions are met.
11251126
fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: [u8; 32]) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitor>), HandleError> {
11261127
// Either ChannelFunded got set (which means it wont bet unset) or there is no way any
11271128
// caller thought we could have something claimed (cause we wouldn't have accepted in an
@@ -1173,7 +1174,9 @@ impl Channel {
11731174
&HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
11741175
if htlc_id_arg == htlc_id {
11751176
debug_assert!(false, "Tried to fulfill an HTLC we already had a holding-cell failure on");
1176-
return Err(HandleError{err: "Unable to find a pending HTLC which matched the given HTLC ID", action: Some(msgs::ErrorAction::IgnoreError)});
1177+
// Return the new channel monitor in a last-ditch effort to hit the
1178+
// chain and claim the funds
1179+
return Ok((None, Some(self.channel_monitor.clone())));
11771180
}
11781181
},
11791182
_ => {}
@@ -1213,8 +1216,9 @@ impl Channel {
12131216
}
12141217
}
12151218

1216-
/// May return an IgnoreError, but should not, and will always return Ok(_) when
1217-
/// debug_assertions are turned on
1219+
/// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made.
1220+
/// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
1221+
/// Ok(_) if debug assertions are turned on and preconditions are met.
12181222
pub fn get_update_fail_htlc(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket) -> Result<Option<msgs::UpdateFailHTLC>, HandleError> {
12191223
if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
12201224
panic!("Was asked to fail an HTLC when channel was not in an operational state");
@@ -2821,6 +2825,16 @@ impl Channel {
28212825
self.channel_update_count += 1;
28222826
return Err(HandleError{err: "funding tx had wrong script/value", action: Some(ErrorAction::DisconnectPeer{msg: None})});
28232827
} else {
2828+
if self.channel_outbound {
2829+
for input in tx.input.iter() {
2830+
if input.witness.is_empty() {
2831+
// We generated a malleable funding transaction, implying we've
2832+
// just exposed ourselves to funds loss to our counterparty.
2833+
#[cfg(not(feature = "fuzztarget"))]
2834+
panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!");
2835+
}
2836+
}
2837+
}
28242838
self.funding_tx_confirmations = 1;
28252839
self.short_channel_id = Some(((height as u64) << (5*8)) |
28262840
((*index_in_block as u64) << (2*8)) |

‎src/ln/channelmanager.rs

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6209,7 +6209,7 @@ mod tests {
62096209

62106210
/// pending_htlc_adds includes both the holding cell and in-flight update_add_htlcs, whereas
62116211
/// for claims/fails they are separated out.
6212-
fn reconnect_nodes(node_a: &Node, node_b: &Node, pre_all_htlcs: bool, pending_htlc_adds: (i64, i64), pending_htlc_claims: (usize, usize), pending_cell_htlc_claims: (usize, usize), pending_cell_htlc_fails: (usize, usize), pending_raa: (bool, bool)) {
6212+
fn reconnect_nodes(node_a: &Node, node_b: &Node, send_funding_locked: (bool, bool), pending_htlc_adds: (i64, i64), pending_htlc_claims: (usize, usize), pending_cell_htlc_claims: (usize, usize), pending_cell_htlc_fails: (usize, usize), pending_raa: (bool, bool)) {
62136213
node_a.node.peer_connected(&node_b.node.get_our_node_id());
62146214
let reestablish_1 = get_chan_reestablish_msgs!(node_a, node_b);
62156215
node_b.node.peer_connected(&node_a.node.get_our_node_id());
@@ -6242,7 +6242,7 @@ mod tests {
62426242
(pending_htlc_adds.1 == 0 && pending_htlc_claims.1 == 0 && pending_cell_htlc_claims.1 == 0 && pending_cell_htlc_fails.1 == 0));
62436243

62446244
for chan_msgs in resp_1.drain(..) {
6245-
if pre_all_htlcs {
6245+
if send_funding_locked.0 {
62466246
node_a.node.handle_funding_locked(&node_b.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
62476247
let announcement_event = node_a.node.get_and_clear_pending_msg_events();
62486248
if !announcement_event.is_empty() {
@@ -6299,7 +6299,7 @@ mod tests {
62996299
}
63006300

63016301
for chan_msgs in resp_2.drain(..) {
6302-
if pre_all_htlcs {
6302+
if send_funding_locked.1 {
63036303
node_b.node.handle_funding_locked(&node_a.node.get_our_node_id(), &chan_msgs.0.unwrap()).unwrap();
63046304
let announcement_event = node_b.node.get_and_clear_pending_msg_events();
63056305
if !announcement_event.is_empty() {
@@ -6363,7 +6363,7 @@ mod tests {
63636363

63646364
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
63656365
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6366-
reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6366+
reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
63676367

63686368
let payment_preimage_1 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
63696369
let payment_hash_2 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).1;
@@ -6372,7 +6372,7 @@ mod tests {
63726372

63736373
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
63746374
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6375-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6375+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
63766376

63776377
let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
63786378
let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
@@ -6385,7 +6385,7 @@ mod tests {
63856385
claim_payment_along_route(&nodes[0], &vec!(&nodes[1], &nodes[2]), true, payment_preimage_3);
63866386
fail_payment_along_route(&nodes[0], &[&nodes[1], &nodes[2]], true, payment_hash_5);
63876387

6388-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (1, 0), (1, 0), (false, false));
6388+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (1, 0), (1, 0), (false, false));
63896389
{
63906390
let events = nodes[0].node.get_and_clear_pending_events();
63916391
assert_eq!(events.len(), 2);
@@ -6466,19 +6466,19 @@ mod tests {
64666466
if messages_delivered < 3 {
64676467
// Even if the funding_locked messages get exchanged, as long as nothing further was
64686468
// received on either side, both sides will need to resend them.
6469-
reconnect_nodes(&nodes[0], &nodes[1], true, (0, 1), (0, 0), (0, 0), (0, 0), (false, false));
6469+
reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 1), (0, 0), (0, 0), (0, 0), (false, false));
64706470
} else if messages_delivered == 3 {
64716471
// nodes[0] still wants its RAA + commitment_signed
6472-
reconnect_nodes(&nodes[0], &nodes[1], false, (-1, 0), (0, 0), (0, 0), (0, 0), (true, false));
6472+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (-1, 0), (0, 0), (0, 0), (0, 0), (true, false));
64736473
} else if messages_delivered == 4 {
64746474
// nodes[0] still wants its commitment_signed
6475-
reconnect_nodes(&nodes[0], &nodes[1], false, (-1, 0), (0, 0), (0, 0), (0, 0), (false, false));
6475+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (-1, 0), (0, 0), (0, 0), (0, 0), (false, false));
64766476
} else if messages_delivered == 5 {
64776477
// nodes[1] still wants its final RAA
6478-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, true));
6478+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, true));
64796479
} else if messages_delivered == 6 {
64806480
// Everything was delivered...
6481-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6481+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
64826482
}
64836483

64846484
let events_1 = nodes[1].node.get_and_clear_pending_events();
@@ -6490,7 +6490,7 @@ mod tests {
64906490

64916491
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
64926492
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6493-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6493+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
64946494

64956495
nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
64966496
nodes[1].node.process_pending_htlc_forwards();
@@ -6564,7 +6564,7 @@ mod tests {
65646564
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
65656565
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
65666566
if messages_delivered < 2 {
6567-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
6567+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
65686568
//TODO: Deduplicate PaymentSent events, then enable this if:
65696569
//if messages_delivered < 1 {
65706570
let events_4 = nodes[0].node.get_and_clear_pending_events();
@@ -6578,21 +6578,21 @@ mod tests {
65786578
//}
65796579
} else if messages_delivered == 2 {
65806580
// nodes[0] still wants its RAA + commitment_signed
6581-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, -1), (0, 0), (0, 0), (0, 0), (false, true));
6581+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, -1), (0, 0), (0, 0), (0, 0), (false, true));
65826582
} else if messages_delivered == 3 {
65836583
// nodes[0] still wants its commitment_signed
6584-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, -1), (0, 0), (0, 0), (0, 0), (false, false));
6584+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, -1), (0, 0), (0, 0), (0, 0), (false, false));
65856585
} else if messages_delivered == 4 {
65866586
// nodes[1] still wants its final RAA
6587-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (true, false));
6587+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (true, false));
65886588
} else if messages_delivered == 5 {
65896589
// Everything was delivered...
6590-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6590+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
65916591
}
65926592

65936593
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
65946594
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6595-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6595+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
65966596

65976597
// Channel should still work fine...
65986598
let payment_preimage_2 = send_along_route(&nodes[0], route, &[&nodes[1]], 1000000).0;
@@ -6633,20 +6633,28 @@ mod tests {
66336633
_ => panic!("Unexpected event"),
66346634
}
66356635

6636+
reconnect_nodes(&nodes[0], &nodes[1], (false, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6637+
6638+
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
6639+
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6640+
66366641
confirm_transaction(&nodes[1].chain_monitor, &tx, tx.version);
66376642
let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
6638-
assert_eq!(events_2.len(), 1);
6643+
assert_eq!(events_2.len(), 2);
66396644
match events_2[0] {
66406645
MessageSendEvent::SendFundingLocked { ref node_id, msg: _ } => {
66416646
assert_eq!(*node_id, nodes[0].node.get_our_node_id());
66426647
},
66436648
_ => panic!("Unexpected event"),
66446649
}
6650+
match events_2[1] {
6651+
MessageSendEvent::SendAnnouncementSignatures { ref node_id, msg: _ } => {
6652+
assert_eq!(*node_id, nodes[0].node.get_our_node_id());
6653+
},
6654+
_ => panic!("Unexpected event"),
6655+
}
66456656

6646-
reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6647-
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
6648-
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6649-
reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6657+
reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
66506658

66516659
// TODO: We shouldn't need to manually pass list_usable_chanels here once we support
66526660
// rebroadcasting announcement_signatures upon reconnect.
@@ -6849,7 +6857,7 @@ mod tests {
68496857
if disconnect {
68506858
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
68516859
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6852-
reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6860+
reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
68536861
}
68546862

68556863
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
@@ -6890,7 +6898,7 @@ mod tests {
68906898
if disconnect {
68916899
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
68926900
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
6893-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
6901+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
68946902
}
68956903

68966904
// ...and make sure we can force-close a TemporaryFailure channel with a PermanentFailure
@@ -7450,7 +7458,7 @@ mod tests {
74507458
nodes[0].node = Arc::new(nodes_0_deserialized);
74517459
check_added_monitors!(nodes[0], 1);
74527460

7453-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
7461+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
74547462

74557463
fail_payment(&nodes[0], &[&nodes[1]], our_payment_hash);
74567464
claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage);
@@ -7520,8 +7528,8 @@ mod tests {
75207528
nodes[0].node = Arc::new(nodes_0_deserialized);
75217529

75227530
// nodes[1] and nodes[2] have no lost state with nodes[0]...
7523-
reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
7524-
reconnect_nodes(&nodes[0], &nodes[2], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
7531+
reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
7532+
reconnect_nodes(&nodes[0], &nodes[2], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
75257533
//... and we can even still claim the payment!
75267534
claim_payment(&nodes[2], &[&nodes[0], &nodes[1]], our_payment_preimage);
75277535

0 commit comments

Comments
 (0)