Skip to content

Commit f3650df

Browse files
TheBlueMattTheBlueMatt
committed
Use new chan_restoration macros in channel_reestablish handling.
This merges the code for restoring channel functionality between channel monitor updating restored and peer reconnection, reducing redundant code.
1 parent 1fa2da3 commit f3650df

File tree

2 files changed

+107
-89
lines changed

2 files changed

+107
-89
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 102 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -886,16 +886,74 @@ macro_rules! maybe_break_monitor_err {
886886
}
887887

888888
macro_rules! handle_chan_restoration_locked {
889-
($self: expr, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
890-
$raa: expr, $commitment_update: expr, $order: expr,
889+
($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
890+
$raa: expr, $commitment_update: expr, $order: expr, $chanmon_update: expr,
891891
$pending_forwards: expr, $funding_broadcastable: expr, $funding_locked: expr) => { {
892892
let mut htlc_forwards = None;
893893
let counterparty_node_id = $channel_entry.get().get_counterparty_node_id();
894894

895-
{
896-
if !$pending_forwards.is_empty() {
895+
let chanmon_update: Option<ChannelMonitorUpdate> = $chanmon_update; // Force type-checking to resolve
896+
let chanmon_update_is_some = chanmon_update.is_some();
897+
let res = loop {
898+
let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve
899+
if !forwards.is_empty() {
897900
htlc_forwards = Some(($channel_entry.get().get_short_channel_id().expect("We can't have pending forwards before funding confirmation"),
898-
$channel_entry.get().get_funding_txo().unwrap(), $pending_forwards));
901+
$channel_entry.get().get_funding_txo().unwrap(), forwards));
902+
}
903+
904+
if chanmon_update.is_some() {
905+
// On reconnect, we, by definition, only resend a funding_locked if there have been
906+
// no commitment updates, so the only channel monitor update which could also be
907+
// associated with a funding_locked would be the funding_created/funding_signed
908+
// monitor update. That monitor update failing implies that we won't send
909+
// funding_locked until it's been updated, so we can't have a funding_locked and a
910+
// monitor update here (so we don't bother to handle it correctly below).
911+
assert!($funding_locked.is_none());
912+
// A channel monitor update makes no sense without either a funding_locked or a
913+
// commitment update to process after it. Since we can't have a funding_locked, we
914+
// only bother to handle the monitor-update + commitment_update case below.
915+
assert!($commitment_update.is_some());
916+
}
917+
918+
if let Some(msg) = $funding_locked {
919+
// Similar to the above, this implies that we're letting the funding_locked fly
920+
// before it should be allowed to.
921+
assert!(chanmon_update.is_none());
922+
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
923+
node_id: counterparty_node_id,
924+
msg,
925+
});
926+
if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
927+
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
928+
node_id: counterparty_node_id,
929+
msg: announcement_sigs,
930+
});
931+
}
932+
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
933+
}
934+
935+
let funding_broadcastable: Option<Transaction> = $funding_broadcastable; // Force type-checking to resolve
936+
if let Some(monitor_update) = chanmon_update {
937+
// We only ever broadcast a funding transaction in response to a funding_signed
938+
// message and the resulting monitor update. Thus, on channel_reestablish
939+
// message handling we can't have a funding transaction to broadcast. When
940+
// processing a monitor update finishing resulting in a funding broadcast, we
941+
// cannot have a second monitor update, thus this case would indicate a bug.
942+
assert!(funding_broadcastable.is_none());
943+
// Given we were just reconnected or finished updating a channel monitor, the
944+
// only case where we can get a new ChannelMonitorUpdate would be if we also
945+
// have some commitment updates to send as well.
946+
assert!($commitment_update.is_some());
947+
if let Err(e) = $self.chain_monitor.update_channel($channel_entry.get().get_funding_txo().unwrap(), monitor_update) {
948+
// channel_reestablish doesn't guarantee the order it returns is sensical
949+
// for the messages it returns, but if we're setting what messages to
950+
// re-transmit on monitor update success, we need to make sure it is sane.
951+
let mut order = $order;
952+
if $raa.is_none() {
953+
order = RAACommitmentOrder::CommitmentFirst;
954+
}
955+
break handle_monitor_err!($self, e, $channel_state, $channel_entry, order, $raa.is_some(), true);
956+
}
899957
}
900958

901959
macro_rules! handle_cs { () => {
@@ -924,31 +982,29 @@ macro_rules! handle_chan_restoration_locked {
924982
handle_cs!();
925983
},
926984
}
927-
if let Some(tx) = $funding_broadcastable {
985+
if let Some(tx) = funding_broadcastable {
928986
log_info!($self.logger, "Broadcasting funding transaction with txid {}", tx.txid());
929987
$self.tx_broadcaster.broadcast_transaction(&tx);
930988
}
931-
if let Some(msg) = $funding_locked {
932-
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
933-
node_id: counterparty_node_id,
934-
msg,
935-
});
936-
if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
937-
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
938-
node_id: counterparty_node_id,
939-
msg: announcement_sigs,
940-
});
941-
}
942-
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
943-
}
989+
break Ok(());
990+
};
991+
992+
if !chanmon_update_is_some {
993+
// If there was no ChannelMonitorUpdate, we should never generate an Err in the res loop
994+
// above. Doing so would imply calling handle_err!() from channel_monitor_updated() which
995+
// should *never* end up calling back to `chain_monitor.update_channel()`.
996+
assert!(res.is_ok());
944997
}
945-
htlc_forwards
998+
999+
(htlc_forwards, res, counterparty_node_id)
9461000
} }
9471001
}
9481002

9491003
macro_rules! post_handle_chan_restoration {
950-
($self: expr, $locked_res: expr) => { {
951-
let htlc_forwards = $locked_res;
1004+
($self: ident, $locked_res: expr) => { {
1005+
let (htlc_forwards, res, counterparty_node_id) = $locked_res;
1006+
1007+
let _ = handle_error!($self, res, counterparty_node_id);
9521008

9531009
if let Some(forwards) = htlc_forwards {
9541010
$self.forward_htlcs(&mut [forwards][..]);
@@ -2676,7 +2732,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
26762732
}
26772733

26782734
let (raa, commitment_update, order, pending_forwards, pending_failures, funding_broadcastable, funding_locked) = channel.get_mut().monitor_updating_restored(&self.logger);
2679-
(pending_failures, handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, pending_forwards, funding_broadcastable, funding_locked))
2735+
(pending_failures, handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, None, pending_forwards, funding_broadcastable, funding_locked))
26802736
};
26812737
post_handle_chan_restoration!(self, chan_restoration_res);
26822738
for failure in pending_failures.drain(..) {
@@ -3292,77 +3348,34 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32923348
}
32933349

32943350
fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
3295-
let mut channel_state_lock = self.channel_state.lock().unwrap();
3296-
let channel_state = &mut *channel_state_lock;
3351+
let chan_restoration_res = {
3352+
let mut channel_state_lock = self.channel_state.lock().unwrap();
3353+
let channel_state = &mut *channel_state_lock;
32973354

3298-
match channel_state.by_id.entry(msg.channel_id) {
3299-
hash_map::Entry::Occupied(mut chan) => {
3300-
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
3301-
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
3302-
}
3303-
// Currently, we expect all holding cell update_adds to be dropped on peer
3304-
// disconnect, so Channel's reestablish will never hand us any holding cell
3305-
// freed HTLCs to fail backwards. If in the future we no longer drop pending
3306-
// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
3307-
let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, mut order, shutdown) =
3308-
try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
3309-
if let Some(monitor_update) = monitor_update_opt {
3310-
if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
3311-
// channel_reestablish doesn't guarantee the order it returns is sensical
3312-
// for the messages it returns, but if we're setting what messages to
3313-
// re-transmit on monitor update success, we need to make sure it is sane.
3314-
if revoke_and_ack.is_none() {
3315-
order = RAACommitmentOrder::CommitmentFirst;
3316-
}
3317-
if commitment_update.is_none() {
3318-
order = RAACommitmentOrder::RevokeAndACKFirst;
3319-
}
3320-
return_monitor_err!(self, e, channel_state, chan, order, revoke_and_ack.is_some(), commitment_update.is_some());
3321-
//TODO: Resend the funding_locked if needed once we get the monitor running again
3322-
}
3323-
}
3324-
if let Some(msg) = funding_locked {
3325-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
3326-
node_id: counterparty_node_id.clone(),
3327-
msg
3328-
});
3329-
}
3330-
macro_rules! send_raa { () => {
3331-
if let Some(msg) = revoke_and_ack {
3332-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK {
3333-
node_id: counterparty_node_id.clone(),
3334-
msg
3335-
});
3355+
match channel_state.by_id.entry(msg.channel_id) {
3356+
hash_map::Entry::Occupied(mut chan) => {
3357+
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
3358+
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
33363359
}
3337-
} }
3338-
macro_rules! send_cu { () => {
3339-
if let Some(updates) = commitment_update {
3340-
channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
3360+
// Currently, we expect all holding cell update_adds to be dropped on peer
3361+
// disconnect, so Channel's reestablish will never hand us any holding cell
3362+
// freed HTLCs to fail backwards. If in the future we no longer drop pending
3363+
// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
3364+
let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, shutdown) =
3365+
try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
3366+
if let Some(msg) = shutdown {
3367+
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
33413368
node_id: counterparty_node_id.clone(),
3342-
updates
3369+
msg,
33433370
});
33443371
}
3345-
} }
3346-
match order {
3347-
RAACommitmentOrder::RevokeAndACKFirst => {
3348-
send_raa!();
3349-
send_cu!();
3350-
},
3351-
RAACommitmentOrder::CommitmentFirst => {
3352-
send_cu!();
3353-
send_raa!();
3354-
},
3355-
}
3356-
if let Some(msg) = shutdown {
3357-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
3358-
node_id: counterparty_node_id.clone(),
3359-
msg,
3360-
});
3361-
}
3362-
Ok(())
3363-
},
3364-
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
3365-
}
3372+
handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked)
3373+
},
3374+
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
3375+
}
3376+
};
3377+
post_handle_chan_restoration!(self, chan_restoration_res);
3378+
Ok(())
33663379
}
33673380

33683381
/// Begin Update fee process. Allowed only on an outbound channel.

‎lightning/src/ln/functional_test_utils.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1515,6 +1515,11 @@ macro_rules! handle_chan_reestablish_msgs {
15151515
None
15161516
};
15171517

1518+
if let Some(&MessageSendEvent::SendAnnouncementSignatures { ref node_id, msg: _ }) = msg_events.get(idx) {
1519+
idx += 1;
1520+
assert_eq!(*node_id, $dst_node.node.get_our_node_id());
1521+
}
1522+
15181523
let mut revoke_and_ack = None;
15191524
let mut commitment_update = None;
15201525
let order = if let Some(ev) = msg_events.get(idx) {

0 commit comments

Comments
 (0)