Skip to content

Commit 2bc710a

Browse files
watersonwaterson
committed
Clear pending flags when peer disconnects (fixup)
1 parent 291366c commit 2bc710a

3 files changed

Lines changed: 106 additions & 18 deletions

File tree

‎lightning/src/ln/async_signer_tests.rs‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,6 +1243,73 @@ fn peer_restart_with_blocked_signer_before_pending_payment() {
12431243
expect_payment_claimable!(nodes[1], payment_hash, payment_secret, 1_000_000);
12441244
}
12451245

1246+
#[test]
1247+
fn no_stray_channel_reestablish() {
1248+
// Original fuzz trace.
1249+
// a0 Disable A’s signer.
1250+
// 2c Disconnect A and B, then restart A.
1251+
// 0e Reconnect A and B.
1252+
// 2d Disconnect A and B (and C), then restart B.
1253+
// a1 Unblock A’s signer get_per_commitment_point
1254+
// ff Reset.
1255+
1256+
let chanmon_cfgs = create_chanmon_cfgs(2);
1257+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
1258+
let alice_persister;
1259+
let bob_persister;
1260+
let alice_new_chain_monitor;
1261+
let bob_new_chain_monitor;
1262+
1263+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
1264+
let alice_deserialized;
1265+
let bob_deserialized;
1266+
1267+
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
1268+
let (_, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
1269+
1270+
// Turn off Alice's signer.
1271+
eprintln!("disabling alice's signer");
1272+
nodes[0].set_channel_signer_ops_available(
1273+
&nodes[1].node.get_our_node_id(), &channel_id,
1274+
ops::GET_PER_COMMITMENT_POINT | ops::RELEASE_COMMITMENT_SECRET | ops::SIGN_COUNTERPARTY_COMMITMENT,
1275+
false);
1276+
1277+
// Disconnect Bob and restart Alice
1278+
eprintln!("disconnecting bob");
1279+
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
1280+
1281+
eprintln!("restarting alice");
1282+
{
1283+
let alice_serialized = nodes[0].node.encode();
1284+
let alice_monitor_serialized = get_monitor!(nodes[0], channel_id).encode();
1285+
reload_node!(nodes[0], *nodes[0].node.get_current_default_configuration(), &alice_serialized, &[&alice_monitor_serialized], alice_persister, alice_new_chain_monitor, alice_deserialized);
1286+
}
1287+
1288+
// Reconnect Alice and Bob.
1289+
eprintln!("reconnecting alice and bob");
1290+
nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init {
1291+
features: nodes[1].node.init_features(), networks: None, remote_network_address: None
1292+
}, false).unwrap();
1293+
1294+
// Disconnect Alice and restart Bob
1295+
eprintln!("disconnecting alice");
1296+
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
1297+
1298+
eprintln!("restarting bob");
1299+
{
1300+
let bob_serialized = nodes[1].node.encode();
1301+
let bob_monitor_serialized = get_monitor!(nodes[1], channel_id).encode();
1302+
reload_node!(nodes[1], *nodes[1].node.get_current_default_configuration(), &bob_serialized, &[&bob_monitor_serialized], bob_persister, bob_new_chain_monitor, bob_deserialized);
1303+
}
1304+
1305+
eprintln!("unblocking alice's signer for get_per_commitment_point");
1306+
nodes[0].set_channel_signer_ops_available(&nodes[1].node.get_our_node_id(), &channel_id, ops::GET_PER_COMMITMENT_POINT, true);
1307+
nodes[0].node.signer_unblocked(None);
1308+
1309+
let events = nodes[0].node.get_and_clear_pending_msg_events();
1310+
assert!(events.is_empty(), "Expected no events from Alice, got {:?}", events);
1311+
}
1312+
12461313
#[test]
12471314
fn dont_elide_channely_ready_from_state_1() {
12481315
// 1. Disable Alice's signer.

‎lightning/src/ln/channel.rs‎

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4216,6 +4216,26 @@ impl<SP: Deref> Channel<SP> where
42164216
return Err(())
42174217
}
42184218

4219+
// If we have requests pending to the signer, then clear out any of them that will be recomputed
4220+
// on `channel_reestablish`. This ensures we don't send them twice if the signer unblocks before
4221+
// we receive the counterparty's reestablish message.
4222+
if self.context.signer_pending_channel_reestablish {
4223+
log_trace!(logger, "Clearing signer_pending_channel_reestablish");
4224+
self.context.signer_pending_channel_reestablish = false;
4225+
}
4226+
if self.context.signer_pending_channel_ready {
4227+
log_trace!(logger, "Clearing signer_pending_channel_ready");
4228+
self.context.signer_pending_channel_ready = false;
4229+
}
4230+
if self.context.signer_pending_revoke_and_ack {
4231+
log_trace!(logger, "Clearing signer_pending_revoke_and_ack");
4232+
self.context.signer_pending_revoke_and_ack = false;
4233+
}
4234+
if self.context.signer_pending_commitment_update {
4235+
log_trace!(logger, "Clearing signer_pending_commitment_update");
4236+
self.context.signer_pending_commitment_update = false;
4237+
}
4238+
42194239
if self.context.channel_state.is_peer_disconnected() {
42204240
// While the below code should be idempotent, it's simpler to just return early, as
42214241
// redundant disconnect events can fire, though they should be rare.
@@ -4232,22 +4252,6 @@ impl<SP: Deref> Channel<SP> where
42324252
self.context.pending_counterparty_closing_signed = None;
42334253
self.context.closing_fee_limits = None;
42344254

4235-
// If we have requests pending to the signer, then clear out any of them that will be recomputed
4236-
// on `channel_reestablish`. This ensures we don't send them twice if the signer unblocks before
4237-
// we receive the counterparty's reestablish message.
4238-
if self.context.signer_pending_channel_ready {
4239-
log_trace!(logger, "clearing signer_pending_channel_ready");
4240-
self.context.signer_pending_channel_ready = false;
4241-
}
4242-
if self.context.signer_pending_revoke_and_ack {
4243-
log_trace!(logger, "clearing signer_pending_revoke_and_ack");
4244-
self.context.signer_pending_revoke_and_ack = false;
4245-
}
4246-
if self.context.signer_pending_commitment_update {
4247-
log_trace!(logger, "clearing signer_pending_commitment_update");
4248-
self.context.signer_pending_commitment_update = false;
4249-
}
4250-
42514255
let mut inbound_drop_count = 0;
42524256
self.context.pending_inbound_htlcs.retain(|htlc| {
42534257
match htlc.state {

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7381,28 +7381,41 @@ where
73817381
let logger = WithChannelContext::from(&self.logger, &chan.context);
73827382
let msgs = chan.signer_maybe_unblocked(&&logger);
73837383
if let Some(msg) = msgs.channel_reestablish {
7384+
log_trace!(logger, "Queuing channel_reestablish to {}", node_id);
73847385
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish { node_id, msg });
73857386
}
73867387
match (msgs.commitment_update, msgs.raa) {
73877388
(Some(cu), Some(raa)) if msgs.order == RAACommitmentOrder::CommitmentFirst => {
7389+
log_trace!(logger, "Queuing update_htlcs to {}", node_id);
73887390
pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu });
7391+
log_trace!(logger, "Queuing revoke_and_ack to {}", node_id);
73897392
pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa });
73907393
},
73917394
(Some(cu), Some(raa)) if msgs.order == RAACommitmentOrder::RevokeAndACKFirst => {
7395+
log_trace!(logger, "Queuing revoke_and_ack to {}", node_id);
73927396
pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa });
7397+
log_trace!(logger, "Queuing update_htlcs to {}", node_id);
73937398
pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu });
73947399
},
7395-
(Some(cu), _) => pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu }),
7396-
(_, Some(raa)) => pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa }),
7400+
(Some(cu), _) => {
7401+
log_trace!(logger, "Queuing update_htlcs to {}", node_id);
7402+
pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu })
7403+
},
7404+
(_, Some(raa)) => {
7405+
log_trace!(logger, "Queuing revoke_and_ack to {}", node_id);
7406+
pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa })
7407+
},
73977408
(_, _) => (),
73987409
};
73997410
if let Some(msg) = msgs.funding_signed {
7411+
log_trace!(logger, "Queuing funding_signed to {}", node_id);
74007412
pending_msg_events.push(events::MessageSendEvent::SendFundingSigned {
74017413
node_id,
74027414
msg,
74037415
});
74047416
}
74057417
if let Some(msg) = msgs.channel_ready {
7418+
log_trace!(logger, "Queuing channel_ready to {}", node_id);
74067419
send_channel_ready!(self, pending_msg_events, chan, msg);
74077420
}
74087421
}
@@ -7411,6 +7424,7 @@ where
74117424
let msgs = chan.signer_maybe_unblocked(&&logger);
74127425
let node_id = phase.context().get_counterparty_node_id();
74137426
if let Some(msg) = msgs.accept_channel {
7427+
log_trace!(logger, "Queuing accept_channel to {}", node_id);
74147428
pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { node_id, msg });
74157429
}
74167430
}
@@ -7419,9 +7433,11 @@ where
74197433
let msgs = chan.signer_maybe_unblocked(&self.chain_hash, &&logger);
74207434
let node_id = phase.context().get_counterparty_node_id();
74217435
if let Some(msg) = msgs.open_channel {
7436+
log_trace!(logger, "Queuing open_channel to {}", node_id);
74227437
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { node_id, msg });
74237438
}
74247439
if let Some(msg) = msgs.funding_created {
7440+
log_trace!(logger, "Queuing funding_created to {}", node_id);
74257441
pending_msg_events.push(events::MessageSendEvent::SendFundingCreated { node_id, msg });
74267442
}
74277443
}
@@ -9050,6 +9066,7 @@ where
90509066
).for_each(|chan| {
90519067
let logger = WithChannelContext::from(&self.logger, &chan.context);
90529068
if let Some(msg) = chan.get_channel_reestablish(&&logger) {
9069+
log_trace!(logger, "Queuing channel_reestablish to {}", chan.context.get_counterparty_node_id());
90539070
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
90549071
node_id: chan.context.get_counterparty_node_id(),
90559072
msg,

0 commit comments

Comments
 (0)