Skip to content

Commit 2934c37

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 94b9318 commit 2934c37

File tree

2 files changed

+96
-89
lines changed

2 files changed

+96
-89
lines changed

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 91 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -815,19 +815,73 @@ macro_rules! maybe_break_monitor_err {
815815
}
816816

817817
macro_rules! handle_chan_restoration_locked {
818-
($self: expr, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
819-
$raa: expr, $commitment_update: expr, $order: expr,
818+
($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
819+
$raa: expr, $commitment_update: expr, $order: expr, $pending_forwards: expr,
820+
$funding_broadcastable: expr, $funding_locked: expr) => { {
821+
let res = handle_chan_restoration_locked!($self, $channel_lock, $channel_state, $channel_entry,
822+
$raa, $commitment_update, $order, None, $pending_forwards, $funding_broadcastable, $funding_locked);
823+
// If there was no ChannelMonitorUpdate, we should never generate an Err in the res loop
824+
// below. Doing so would imply calling handle_err!() from channel_monitor_updated() which
825+
// should *never* end up calling back to `chain_monitor.update_channel()`.
826+
assert!(res.1.is_ok());
827+
res
828+
} };
829+
($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
830+
$raa: expr, $commitment_update: expr, $order: expr, $chanmon_update: expr,
820831
$pending_forwards: expr, $funding_broadcastable: expr, $funding_locked: expr) => { {
821832
let mut htlc_forwards = None;
822833
let counterparty_node_id = $channel_entry.get().get_counterparty_node_id();
823834

824-
{
825-
if !$pending_forwards.is_empty() {
835+
let res = loop {
836+
let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve
837+
if !forwards.is_empty() {
826838
htlc_forwards = Some(($channel_entry.get().get_short_channel_id().expect("We can't have pending forwards before funding confirmation"),
827-
$channel_entry.get().get_funding_txo().unwrap(), $pending_forwards));
839+
$channel_entry.get().get_funding_txo().unwrap(), forwards));
828840
}
829841

842+
let chanmon_update: Option<ChannelMonitorUpdate> = $chanmon_update; // Force type-checking to resolve
843+
if chanmon_update.is_some() {
844+
// On reconnect (or monitor restoration), we, by definition, only resend a
845+
// funding_locked if there have been no commitment updates, so the only channel
846+
// monitor update which could also be associated with a funding_locked would be
847+
// the funding_created/funding_signed monitor update. That monitor update failing
848+
// implies that we won't send funding_locked until it's been updated, so we can't
849+
// have a funding_locked and a monitor update here (so we don't bother to handle it
850+
// correctly below).
851+
assert!($funding_locked.is_none());
852+
// A channel monitor update makes no sense without either a funding_locked or a
853+
// commitment update to process after it. Since we can't have a funding_locked, we
854+
// only bother to handle the monitor-update + commitment_update case below.
855+
assert!($commitment_update.is_some());
856+
}
857+
858+
if let Some(msg) = $funding_locked {
859+
// Similar to the above, this implies that we're letting the funding_locked fly
860+
// before it should be allowed to.
861+
assert!(chanmon_update.is_none());
862+
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
863+
node_id: counterparty_node_id,
864+
msg,
865+
});
866+
if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
867+
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
868+
node_id: counterparty_node_id,
869+
msg: announcement_sigs,
870+
});
871+
}
872+
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
873+
}
874+
875+
let funding_broadcastable: Option<Transaction> = $funding_broadcastable; // Force type-checking to resolve
830876
macro_rules! handle_cs { () => {
877+
if let Some(monitor_update) = chanmon_update {
878+
assert!($order == RAACommitmentOrder::RevokeAndACKFirst);
879+
assert!(funding_broadcastable.is_none());
880+
assert!($commitment_update.is_some());
881+
if let Err(e) = $self.chain_monitor.update_channel($channel_entry.get().get_funding_txo().unwrap(), monitor_update) {
882+
break handle_monitor_err!($self, e, $channel_state, $channel_entry, RAACommitmentOrder::CommitmentFirst, false, true);
883+
}
884+
}
831885
if let Some(update) = $commitment_update {
832886
$channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
833887
node_id: counterparty_node_id,
@@ -853,30 +907,21 @@ macro_rules! handle_chan_restoration_locked {
853907
handle_cs!();
854908
},
855909
}
856-
if let Some(tx) = $funding_broadcastable {
910+
if let Some(tx) = funding_broadcastable {
857911
$self.tx_broadcaster.broadcast_transaction(&tx);
858912
}
859-
if let Some(msg) = $funding_locked {
860-
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
861-
node_id: counterparty_node_id,
862-
msg,
863-
});
864-
if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
865-
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
866-
node_id: counterparty_node_id,
867-
msg: announcement_sigs,
868-
});
869-
}
870-
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
871-
}
872-
}
873-
htlc_forwards
913+
break Ok(());
914+
};
915+
916+
(htlc_forwards, res, counterparty_node_id)
874917
} }
875918
}
876919

877920
macro_rules! post_handle_chan_restoration {
878-
($self: expr, $locked_res: expr) => { {
879-
let htlc_forwards = $locked_res;
921+
($self: ident, $locked_res: expr) => { {
922+
let (htlc_forwards, res, counterparty_node_id) = $locked_res;
923+
924+
let _ = handle_error!($self, res, counterparty_node_id);
880925

881926
if let Some(forwards) = htlc_forwards {
882927
$self.forward_htlcs(&mut [forwards][..]);
@@ -3133,77 +3178,34 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31333178
}
31343179

31353180
fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
3136-
let mut channel_state_lock = self.channel_state.lock().unwrap();
3137-
let channel_state = &mut *channel_state_lock;
3181+
let chan_restoration_res = {
3182+
let mut channel_state_lock = self.channel_state.lock().unwrap();
3183+
let channel_state = &mut *channel_state_lock;
31383184

3139-
match channel_state.by_id.entry(msg.channel_id) {
3140-
hash_map::Entry::Occupied(mut chan) => {
3141-
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
3142-
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
3143-
}
3144-
// Currently, we expect all holding cell update_adds to be dropped on peer
3145-
// disconnect, so Channel's reestablish will never hand us any holding cell
3146-
// freed HTLCs to fail backwards. If in the future we no longer drop pending
3147-
// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
3148-
let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, mut order, shutdown) =
3149-
try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
3150-
if let Some(monitor_update) = monitor_update_opt {
3151-
if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
3152-
// channel_reestablish doesn't guarantee the order it returns is sensical
3153-
// for the messages it returns, but if we're setting what messages to
3154-
// re-transmit on monitor update success, we need to make sure it is sane.
3155-
if revoke_and_ack.is_none() {
3156-
order = RAACommitmentOrder::CommitmentFirst;
3157-
}
3158-
if commitment_update.is_none() {
3159-
order = RAACommitmentOrder::RevokeAndACKFirst;
3160-
}
3161-
return_monitor_err!(self, e, channel_state, chan, order, revoke_and_ack.is_some(), commitment_update.is_some());
3162-
//TODO: Resend the funding_locked if needed once we get the monitor running again
3163-
}
3164-
}
3165-
if let Some(msg) = funding_locked {
3166-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
3167-
node_id: counterparty_node_id.clone(),
3168-
msg
3169-
});
3170-
}
3171-
macro_rules! send_raa { () => {
3172-
if let Some(msg) = revoke_and_ack {
3173-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK {
3174-
node_id: counterparty_node_id.clone(),
3175-
msg
3176-
});
3185+
match channel_state.by_id.entry(msg.channel_id) {
3186+
hash_map::Entry::Occupied(mut chan) => {
3187+
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
3188+
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
31773189
}
3178-
} }
3179-
macro_rules! send_cu { () => {
3180-
if let Some(updates) = commitment_update {
3181-
channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
3190+
// Currently, we expect all holding cell update_adds to be dropped on peer
3191+
// disconnect, so Channel's reestablish will never hand us any holding cell
3192+
// freed HTLCs to fail backwards. If in the future we no longer drop pending
3193+
// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
3194+
let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, shutdown) =
3195+
try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
3196+
if let Some(msg) = shutdown {
3197+
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
31823198
node_id: counterparty_node_id.clone(),
3183-
updates
3199+
msg,
31843200
});
31853201
}
3186-
} }
3187-
match order {
3188-
RAACommitmentOrder::RevokeAndACKFirst => {
3189-
send_raa!();
3190-
send_cu!();
3191-
},
3192-
RAACommitmentOrder::CommitmentFirst => {
3193-
send_cu!();
3194-
send_raa!();
3195-
},
3196-
}
3197-
if let Some(msg) = shutdown {
3198-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
3199-
node_id: counterparty_node_id.clone(),
3200-
msg,
3201-
});
3202-
}
3203-
Ok(())
3204-
},
3205-
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
3206-
}
3202+
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)
3203+
},
3204+
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
3205+
}
3206+
};
3207+
post_handle_chan_restoration!(self, chan_restoration_res);
3208+
Ok(())
32073209
}
32083210

32093211
/// 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
@@ -1506,6 +1506,11 @@ macro_rules! handle_chan_reestablish_msgs {
15061506
None
15071507
};
15081508

1509+
if let Some(&MessageSendEvent::SendAnnouncementSignatures { ref node_id, msg: _ }) = msg_events.get(idx) {
1510+
idx += 1;
1511+
assert_eq!(*node_id, $dst_node.node.get_our_node_id());
1512+
}
1513+
15091514
let mut revoke_and_ack = None;
15101515
let mut commitment_update = None;
15111516
let order = if let Some(ev) = msg_events.get(idx) {

0 commit comments

Comments
 (0)