Skip to content

Commit 74ab4d5

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 8157246 commit 74ab4d5

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
@@ -856,19 +856,73 @@ macro_rules! maybe_break_monitor_err {
856856
}
857857

858858
macro_rules! handle_chan_restoration_locked {
859-
($self: expr, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
860-
$raa: expr, $commitment_update: expr, $order: expr,
859+
($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
860+
$raa: expr, $commitment_update: expr, $order: expr, $pending_forwards: expr,
861+
$funding_broadcastable: expr, $funding_locked: expr) => { {
862+
let res = handle_chan_restoration_locked!($self, $channel_lock, $channel_state, $channel_entry,
863+
$raa, $commitment_update, $order, None, $pending_forwards, $funding_broadcastable, $funding_locked);
864+
// If there was no ChannelMonitorUpdate, we should never generate an Err in the res loop
865+
// below. Doing so would imply calling handle_err!() from channel_monitor_updated() which
866+
// should *never* end up calling back to `chain_monitor.update_channel()`.
867+
assert!(res.1.is_ok());
868+
res
869+
} };
870+
($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
871+
$raa: expr, $commitment_update: expr, $order: expr, $chanmon_update: expr,
861872
$pending_forwards: expr, $funding_broadcastable: expr, $funding_locked: expr) => { {
862873
let mut htlc_forwards = None;
863874
let counterparty_node_id = $channel_entry.get().get_counterparty_node_id();
864875

865-
{
866-
if !$pending_forwards.is_empty() {
876+
let res = loop {
877+
let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve
878+
if !forwards.is_empty() {
867879
htlc_forwards = Some(($channel_entry.get().get_short_channel_id().expect("We can't have pending forwards before funding confirmation"),
868-
$channel_entry.get().get_funding_txo().unwrap(), $pending_forwards));
880+
$channel_entry.get().get_funding_txo().unwrap(), forwards));
869881
}
870882

883+
let chanmon_update: Option<ChannelMonitorUpdate> = $chanmon_update; // Force type-checking to resolve
884+
if chanmon_update.is_some() {
885+
// On reconnect (or monitor restoration), we, by definition, only resend a
886+
// funding_locked if there have been no commitment updates, so the only channel
887+
// monitor update which could also be associated with a funding_locked would be
888+
// the funding_created/funding_signed monitor update. That monitor update failing
889+
// implies that we won't send funding_locked until it's been updated, so we can't
890+
// have a funding_locked and a monitor update here (so we don't bother to handle it
891+
// correctly below).
892+
assert!($funding_locked.is_none());
893+
// A channel monitor update makes no sense without either a funding_locked or a
894+
// commitment update to process after it. Since we can't have a funding_locked, we
895+
// only bother to handle the monitor-update + commitment_update case below.
896+
assert!($commitment_update.is_some());
897+
}
898+
899+
if let Some(msg) = $funding_locked {
900+
// Similar to the above, this implies that we're letting the funding_locked fly
901+
// before it should be allowed to.
902+
assert!(chanmon_update.is_none());
903+
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
904+
node_id: counterparty_node_id,
905+
msg,
906+
});
907+
if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
908+
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
909+
node_id: counterparty_node_id,
910+
msg: announcement_sigs,
911+
});
912+
}
913+
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
914+
}
915+
916+
let funding_broadcastable: Option<Transaction> = $funding_broadcastable; // Force type-checking to resolve
871917
macro_rules! handle_cs { () => {
918+
if let Some(monitor_update) = chanmon_update {
919+
assert!($order == RAACommitmentOrder::RevokeAndACKFirst);
920+
assert!(funding_broadcastable.is_none());
921+
assert!($commitment_update.is_some());
922+
if let Err(e) = $self.chain_monitor.update_channel($channel_entry.get().get_funding_txo().unwrap(), monitor_update) {
923+
break handle_monitor_err!($self, e, $channel_state, $channel_entry, RAACommitmentOrder::CommitmentFirst, false, true);
924+
}
925+
}
872926
if let Some(update) = $commitment_update {
873927
$channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
874928
node_id: counterparty_node_id,
@@ -894,31 +948,22 @@ macro_rules! handle_chan_restoration_locked {
894948
handle_cs!();
895949
},
896950
}
897-
if let Some(tx) = $funding_broadcastable {
951+
if let Some(tx) = funding_broadcastable {
898952
log_info!($self.logger, "Broadcasting funding transaction with txid {}", tx.txid());
899953
$self.tx_broadcaster.broadcast_transaction(&tx);
900954
}
901-
if let Some(msg) = $funding_locked {
902-
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
903-
node_id: counterparty_node_id,
904-
msg,
905-
});
906-
if let Some(announcement_sigs) = $self.get_announcement_sigs($channel_entry.get()) {
907-
$channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
908-
node_id: counterparty_node_id,
909-
msg: announcement_sigs,
910-
});
911-
}
912-
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
913-
}
914-
}
915-
htlc_forwards
955+
break Ok(());
956+
};
957+
958+
(htlc_forwards, res, counterparty_node_id)
916959
} }
917960
}
918961

919962
macro_rules! post_handle_chan_restoration {
920-
($self: expr, $locked_res: expr) => { {
921-
let htlc_forwards = $locked_res;
963+
($self: ident, $locked_res: expr) => { {
964+
let (htlc_forwards, res, counterparty_node_id) = $locked_res;
965+
966+
let _ = handle_error!($self, res, counterparty_node_id);
922967

923968
if let Some(forwards) = htlc_forwards {
924969
$self.forward_htlcs(&mut [forwards][..]);
@@ -3213,77 +3258,34 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
32133258
}
32143259

32153260
fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
3216-
let mut channel_state_lock = self.channel_state.lock().unwrap();
3217-
let channel_state = &mut *channel_state_lock;
3261+
let chan_restoration_res = {
3262+
let mut channel_state_lock = self.channel_state.lock().unwrap();
3263+
let channel_state = &mut *channel_state_lock;
32183264

3219-
match channel_state.by_id.entry(msg.channel_id) {
3220-
hash_map::Entry::Occupied(mut chan) => {
3221-
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
3222-
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
3223-
}
3224-
// Currently, we expect all holding cell update_adds to be dropped on peer
3225-
// disconnect, so Channel's reestablish will never hand us any holding cell
3226-
// freed HTLCs to fail backwards. If in the future we no longer drop pending
3227-
// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
3228-
let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, mut order, shutdown) =
3229-
try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
3230-
if let Some(monitor_update) = monitor_update_opt {
3231-
if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
3232-
// channel_reestablish doesn't guarantee the order it returns is sensical
3233-
// for the messages it returns, but if we're setting what messages to
3234-
// re-transmit on monitor update success, we need to make sure it is sane.
3235-
if revoke_and_ack.is_none() {
3236-
order = RAACommitmentOrder::CommitmentFirst;
3237-
}
3238-
if commitment_update.is_none() {
3239-
order = RAACommitmentOrder::RevokeAndACKFirst;
3240-
}
3241-
return_monitor_err!(self, e, channel_state, chan, order, revoke_and_ack.is_some(), commitment_update.is_some());
3242-
//TODO: Resend the funding_locked if needed once we get the monitor running again
3243-
}
3244-
}
3245-
if let Some(msg) = funding_locked {
3246-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
3247-
node_id: counterparty_node_id.clone(),
3248-
msg
3249-
});
3250-
}
3251-
macro_rules! send_raa { () => {
3252-
if let Some(msg) = revoke_and_ack {
3253-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK {
3254-
node_id: counterparty_node_id.clone(),
3255-
msg
3256-
});
3265+
match channel_state.by_id.entry(msg.channel_id) {
3266+
hash_map::Entry::Occupied(mut chan) => {
3267+
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
3268+
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
32573269
}
3258-
} }
3259-
macro_rules! send_cu { () => {
3260-
if let Some(updates) = commitment_update {
3261-
channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
3270+
// Currently, we expect all holding cell update_adds to be dropped on peer
3271+
// disconnect, so Channel's reestablish will never hand us any holding cell
3272+
// freed HTLCs to fail backwards. If in the future we no longer drop pending
3273+
// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
3274+
let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, shutdown) =
3275+
try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
3276+
if let Some(msg) = shutdown {
3277+
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
32623278
node_id: counterparty_node_id.clone(),
3263-
updates
3279+
msg,
32643280
});
32653281
}
3266-
} }
3267-
match order {
3268-
RAACommitmentOrder::RevokeAndACKFirst => {
3269-
send_raa!();
3270-
send_cu!();
3271-
},
3272-
RAACommitmentOrder::CommitmentFirst => {
3273-
send_cu!();
3274-
send_raa!();
3275-
},
3276-
}
3277-
if let Some(msg) = shutdown {
3278-
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
3279-
node_id: counterparty_node_id.clone(),
3280-
msg,
3281-
});
3282-
}
3283-
Ok(())
3284-
},
3285-
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
3286-
}
3282+
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)
3283+
},
3284+
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
3285+
}
3286+
};
3287+
post_handle_chan_restoration!(self, chan_restoration_res);
3288+
Ok(())
32873289
}
32883290

32893291
/// 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)