Skip to content

Commit ac1508f

Browse files
committed
Fix responding to RAAs when monitor updating had already failed
1 parent 417e1c4 commit ac1508f

File tree

2 files changed

+107
-1
lines changed

2 files changed

+107
-1
lines changed

src/ln/chanmon_update_fail_tests.rs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,3 +1275,93 @@ fn monitor_failed_no_reestablish_response() {
12751275

12761276
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
12771277
}
1278+
1279+
#[test]
1280+
fn first_message_on_recv_ordering() {
1281+
// Test that if the initial generator of a monitor-update-frozen state doesn't generate
1282+
// messages, we're willing to flip the order of response messages if neccessary in resposne to
1283+
// a commitment_signed which needs to send an RAA first.
1284+
// At a high level, our goal is to fail monitor updating in response to an RAA which needs no
1285+
// response and then handle a CS while in the failed state, requiring an RAA followed by a CS
1286+
// response. To do this, we start routing two payments, with the final RAA for the first being
1287+
// delivered while B is in AwaitingRAA, hence when we deliver the CS for the second B will
1288+
// have no pending response but will want to send a RAA/CS (with the updates for the second
1289+
// payment applied).
1290+
// Backported from chanmon_fail_consistency fuzz tests as it caught a bug here.
1291+
let mut nodes = create_network(2);
1292+
create_announced_chan_between_nodes(&nodes, 0, 1);
1293+
1294+
// Route the first payment outbound, holding the last RAA for B until we are set up so that we
1295+
// can deliver it and fail the monitor update.
1296+
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
1297+
let (payment_preimage_1, payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
1298+
nodes[0].node.send_payment(route, payment_hash_1).unwrap();
1299+
check_added_monitors!(nodes[0], 1);
1300+
1301+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
1302+
assert_eq!(events.len(), 1);
1303+
let payment_event = SendEvent::from_event(events.pop().unwrap());
1304+
assert_eq!(payment_event.node_id, nodes[1].node.get_our_node_id());
1305+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]).unwrap();
1306+
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &payment_event.commitment_msg).unwrap();
1307+
check_added_monitors!(nodes[1], 1);
1308+
let bs_responses = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id());
1309+
1310+
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_responses.0).unwrap();
1311+
check_added_monitors!(nodes[0], 1);
1312+
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_responses.1).unwrap();
1313+
check_added_monitors!(nodes[0], 1);
1314+
1315+
let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
1316+
1317+
// Route the second payment, generating an update_add_htlc/commitment_signed
1318+
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
1319+
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
1320+
nodes[0].node.send_payment(route, payment_hash_2).unwrap();
1321+
check_added_monitors!(nodes[0], 1);
1322+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
1323+
assert_eq!(events.len(), 1);
1324+
let payment_event = SendEvent::from_event(events.pop().unwrap());
1325+
assert_eq!(payment_event.node_id, nodes[1].node.get_our_node_id());
1326+
1327+
*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
1328+
1329+
// Deliver the final RAA for the first payment, which does not require a response. RAAs
1330+
// generally require a commitment_signed, so the fact that we're expecting an opposite response
1331+
// to the next message also tests resetting the delivery order.
1332+
if let msgs::HandleError { err, action: Some(msgs::ErrorAction::IgnoreError) } = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa).unwrap_err() {
1333+
assert_eq!(err, "Failed to update ChannelMonitor");
1334+
} else { panic!(); }
1335+
check_added_monitors!(nodes[1], 1);
1336+
1337+
// Now deliver the update_add_htlc/commitment_signed for the second payment, which does need an
1338+
// RAA/CS response, which should be generated when we call test_restore_channel_monitor (with
1339+
// the appropriate HTLC acceptance).
1340+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]).unwrap();
1341+
if let msgs::HandleError { err, action: Some(msgs::ErrorAction::IgnoreError) } = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &payment_event.commitment_msg).unwrap_err() {
1342+
assert_eq!(err, "Previous monitor update failure prevented generation of RAA");
1343+
} else { panic!(); }
1344+
1345+
*nodes[1].chan_monitor.update_ret.lock().unwrap() = Ok(());
1346+
nodes[1].node.test_restore_channel_monitor();
1347+
check_added_monitors!(nodes[1], 1);
1348+
1349+
expect_pending_htlcs_forwardable!(nodes[1]);
1350+
expect_payment_received!(nodes[1], payment_hash_1);
1351+
1352+
let bs_responses = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id());
1353+
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_responses.0).unwrap();
1354+
check_added_monitors!(nodes[0], 1);
1355+
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_responses.1).unwrap();
1356+
check_added_monitors!(nodes[0], 1);
1357+
1358+
let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
1359+
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa).unwrap();
1360+
check_added_monitors!(nodes[1], 1);
1361+
1362+
expect_pending_htlcs_forwardable!(nodes[1]);
1363+
expect_payment_received!(nodes[1], payment_hash_2);
1364+
1365+
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
1366+
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
1367+
}

src/ln/channel.rs

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1791,8 +1791,24 @@ impl Channel {
17911791
self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
17921792

17931793
if (self.channel_state & ChannelState::MonitorUpdateFailed as u32) != 0 {
1794+
// In case we initially failed monitor updating without requiring a response, we need
1795+
// to make sure the RAA gets sent first.
1796+
if !self.monitor_pending_commitment_signed {
1797+
self.monitor_pending_order = Some(RAACommitmentOrder::RevokeAndACKFirst);
1798+
}
17941799
self.monitor_pending_revoke_and_ack = true;
1795-
self.monitor_pending_commitment_signed |= need_our_commitment;
1800+
if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
1801+
// If we were going to send a commitment_signed after the RAA, go ahead and do all
1802+
// the corresponding HTLC status updates so that get_last_commitment_update
1803+
// includes the right HTLCs.
1804+
// Note that this generates a monitor update that we ignore! This is OK since we
1805+
// won't actually send the commitment_signed that generated the update to the other
1806+
// side until the latest monitor has been pulled from us and stored.
1807+
self.monitor_pending_commitment_signed = true;
1808+
self.send_commitment_no_status_check()?;
1809+
}
1810+
// TODO: Call maybe_propose_first_closing_signed on restoration (or call it here and
1811+
// re-send the message on restoration)
17961812
return Err(ChannelError::Ignore("Previous monitor update failure prevented generation of RAA"));
17971813
}
17981814

0 commit comments

Comments
 (0)