Skip to content

Commit 40b7615

Browse files
committed
f test success-then-fail and correct handling
1 parent 3b4d809 commit 40b7615

File tree

2 files changed

+50
-20
lines changed

2 files changed

+50
-20
lines changed

lightning/src/ln/chanmon_update_fail_tests.rs

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2222,7 +2222,7 @@ enum HTLCStatusAtDupClaim {
22222222
HoldingCell,
22232223
Cleared,
22242224
}
2225-
fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim) {
2225+
fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_fails: bool) {
22262226
// When receiving an update_fulfill_htlc message, we immediately forward the claim backwards
22272227
// along the payment path before waiting for a full commitment_signed dance. This is great, but
22282228
// can cause duplicative claims if a node sends an update_fulfill_htlc message, disconnects,
@@ -2235,9 +2235,9 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim) {
22352235
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
22362236

22372237
create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
2238-
create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
2238+
let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known()).2;
22392239

2240-
let preimage = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100_000).0;
2240+
let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 100_000);
22412241

22422242
let mut as_raa = None;
22432243
if htlc_status == HTLCStatusAtDupClaim::HoldingCell {
@@ -2263,19 +2263,33 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim) {
22632263
as_raa = Some(get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id()));
22642264
}
22652265

2266-
assert!(nodes[2].node.claim_funds(preimage));
2267-
check_added_monitors!(nodes[2], 1);
2268-
let cs_updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
2269-
assert_eq!(cs_updates.update_fulfill_htlcs.len(), 1);
2270-
nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
2266+
let fulfill_msg = msgs::UpdateFulfillHTLC {
2267+
channel_id: chan_2,
2268+
htlc_id: 0,
2269+
payment_preimage,
2270+
};
2271+
if second_fails {
2272+
assert!(nodes[2].node.fail_htlc_backwards(&payment_hash));
2273+
expect_pending_htlcs_forwardable!(nodes[2]);
2274+
check_added_monitors!(nodes[2], 1);
2275+
get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
2276+
} else {
2277+
assert!(nodes[2].node.claim_funds(payment_preimage));
2278+
check_added_monitors!(nodes[2], 1);
2279+
let cs_updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
2280+
assert_eq!(cs_updates.update_fulfill_htlcs.len(), 1);
2281+
// Check that the message we're about to deliver matches the one generated:
2282+
assert_eq!(fulfill_msg, cs_updates.update_fulfill_htlcs[0]);
2283+
}
2284+
nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &fulfill_msg);
22712285
check_added_monitors!(nodes[1], 1);
22722286

22732287
let mut bs_updates = None;
22742288
if htlc_status != HTLCStatusAtDupClaim::HoldingCell {
22752289
bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()));
22762290
assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1);
22772291
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]);
2278-
expect_payment_sent!(nodes[0], preimage);
2292+
expect_payment_sent!(nodes[0], payment_preimage);
22792293
if htlc_status == HTLCStatusAtDupClaim::Cleared {
22802294
commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false);
22812295
}
@@ -2286,7 +2300,12 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim) {
22862300
nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id(), false);
22872301
nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
22882302

2289-
reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
2303+
if second_fails {
2304+
reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (0, 0), (1, 0), (0, 0), (0, 0), (false, false));
2305+
expect_pending_htlcs_forwardable!(nodes[1]);
2306+
} else {
2307+
reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (0, 0), (false, false));
2308+
}
22902309

22912310
if htlc_status == HTLCStatusAtDupClaim::HoldingCell {
22922311
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa.unwrap());
@@ -2296,7 +2315,7 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim) {
22962315
bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()));
22972316
assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1);
22982317
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]);
2299-
expect_payment_sent!(nodes[0], preimage);
2318+
expect_payment_sent!(nodes[0], payment_preimage);
23002319
}
23012320
if htlc_status != HTLCStatusAtDupClaim::Cleared {
23022321
commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false);
@@ -2305,7 +2324,10 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim) {
23052324

23062325
#[test]
23072326
fn test_reconnect_dup_htlc_claims() {
2308-
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Received);
2309-
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::HoldingCell);
2310-
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Cleared);
2327+
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Received, false);
2328+
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::HoldingCell, false);
2329+
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Cleared, false);
2330+
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Received, true);
2331+
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::HoldingCell, true);
2332+
do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Cleared, true);
23112333
}

lightning/src/ln/channel.rs

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1264,7 +1264,7 @@ impl<Signer: Sign> Channel<Signer> {
12641264
if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
12651265
} else {
12661266
log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id()));
1267-
debug_assert!(false, "Tried to fulfill an HTLC that was already fail/fulfilled");
1267+
debug_assert!(false, "Tried to fulfill an HTLC that was already failed");
12681268
}
12691269
return Ok((None, None));
12701270
},
@@ -1390,8 +1390,11 @@ impl<Signer: Sign> Channel<Signer> {
13901390
if htlc.htlc_id == htlc_id_arg {
13911391
match htlc.state {
13921392
InboundHTLCState::Committed => {},
1393-
InboundHTLCState::LocalRemoved(_) => {
1394-
debug_assert!(false, "Tried to fail an HTLC that was already fail/fulfilled");
1393+
InboundHTLCState::LocalRemoved(ref reason) => {
1394+
if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
1395+
} else {
1396+
debug_assert!(false, "Tried to fail an HTLC that was already failed");
1397+
}
13951398
return Ok(None);
13961399
},
13971400
_ => {
@@ -1403,7 +1406,11 @@ impl<Signer: Sign> Channel<Signer> {
14031406
}
14041407
}
14051408
if pending_idx == core::usize::MAX {
1406-
return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID".to_owned()));
1409+
#[cfg(any(test, feature = "fuzztarget"))]
1410+
// If we failed to find an HTLC to fail, make sure it was previously fulfilled and this
1411+
// is simply a duplicate fail, not previously failed and we failed-back too early.
1412+
debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
1413+
return Ok(None);
14071414
}
14081415

14091416
// Now update local state:
@@ -1412,8 +1419,9 @@ impl<Signer: Sign> Channel<Signer> {
14121419
match pending_update {
14131420
&HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => {
14141421
if htlc_id_arg == htlc_id {
1415-
debug_assert!(false, "Tried to fail an HTLC that was already fulfilled");
1416-
return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID".to_owned()));
1422+
#[cfg(any(test, feature = "fuzztarget"))]
1423+
debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
1424+
return Ok(None);
14171425
}
14181426
},
14191427
&HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {

0 commit comments

Comments
 (0)