Skip to content

Clean up and more liberally free holding cell HTLCs #756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

Clean up and more liberally free holding cell HTLCs #756

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6d99c89
Drop unused variable and loop in channel_monitor_updated
TheBlueMatt Nov 20, 2020
f5451ec
Move channel restoration after monitor update to a two-part macro
TheBlueMatt Nov 20, 2020
6170828
Clean up channel updating macro somewhat
TheBlueMatt Dec 15, 2020
eb0b664
Free holding cell on monitor-updating-restored when there's no upd
TheBlueMatt Nov 19, 2020
49c7b01
Use new chan_restoration macros in channel_reestablish handling.
TheBlueMatt Nov 20, 2020
6e865ea
Stop failing back HTLCs on temporary monitor udpate failures
TheBlueMatt Nov 20, 2020
e7a4908
Use return struct in monitor_updating_restored+channel_reestablish
TheBlueMatt Feb 21, 2021
daedbbe
[fuzz] fix deadlock in chanmon_consistency due to new reentrancy
TheBlueMatt Nov 19, 2020
81cd1ad
Print the output of all failed fuzz cases in test, not one test.
TheBlueMatt Nov 23, 2020
d0a8a90
[fuzz] Fail in chanmon_consistency on IgnoreError error events
TheBlueMatt Nov 24, 2020
627df71
[fuzz] Allow SendAnnouncementSigs events in chanmon_consistency
TheBlueMatt Nov 24, 2020
6b22abd
Read monitors from our KeysInterface in chanmon_consistency_fuzz
TheBlueMatt Feb 9, 2021
76db5db
Add a test for HTLC freeing on monitor update restoration
TheBlueMatt Mar 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2756,7 +2756,10 @@ impl<Signer: Sign> Channel<Signer> {
/// Indicates that the latest ChannelMonitor update has been committed by the client
/// successfully and we should restore normal operation. Returns messages which should be sent
/// to the remote side.
pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool, Option<msgs::FundingLocked>) where L::Target: Logger {
pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> (
Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Option<ChannelMonitorUpdate>,
Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Vec<(HTLCSource, PaymentHash)>,
bool, Option<msgs::FundingLocked>) where L::Target: Logger {
assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);

Expand Down Expand Up @@ -2786,25 +2789,39 @@ impl<Signer: Sign> Channel<Signer> {
if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 {
self.monitor_pending_revoke_and_ack = false;
self.monitor_pending_commitment_signed = false;
return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, needs_broadcast_safe, funding_locked);
return (None, None, RAACommitmentOrder::RevokeAndACKFirst, None, forwards, failures, Vec::new(), needs_broadcast_safe, funding_locked);
}

let raa = if self.monitor_pending_revoke_and_ack {
Some(self.get_last_revoke_and_ack())
} else { None };
let commitment_update = if self.monitor_pending_commitment_signed {
let mut commitment_update = if self.monitor_pending_commitment_signed {
Some(self.get_last_commitment_update(logger))
} else { None };

let mut order = self.resend_order.clone();
self.monitor_pending_revoke_and_ack = false;
self.monitor_pending_commitment_signed = false;
let order = self.resend_order.clone();

let mut htlcs_failed_to_forward = Vec::new();
let mut chanmon_update = None;
if commitment_update.is_none() && self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32) == 0 {
order = RAACommitmentOrder::RevokeAndACKFirst;

let (update_opt, mut failed_htlcs) = self.free_holding_cell_htlcs(logger).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now have the whole process_background_events thing, I wonder if it'd ease the reentrancy concerns a bit to shift this call over there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of using that for this because its hooked on the one minute timer which may take a while...however, I think it makes sense in get_and_clear_pending_msg_events. Going to open a new PR with that.

htlcs_failed_to_forward.append(&mut failed_htlcs);
if let Some((com_update, mon_update)) = update_opt {
commitment_update = Some(com_update);
chanmon_update = Some(mon_update);
}
}

log_trace!(logger, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first",
if needs_broadcast_safe { "a funding broadcast safe, " } else { "" },
if commitment_update.is_some() { "a" } else { "no" },
if raa.is_some() { "an" } else { "no" },
match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
(raa, commitment_update, order, forwards, failures, needs_broadcast_safe, funding_locked)
(raa, commitment_update, order, chanmon_update, forwards, failures, htlcs_failed_to_forward, needs_broadcast_safe, funding_locked)
}

pub fn update_fee<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::UpdateFee) -> Result<(), ChannelError>
Expand Down
45 changes: 33 additions & 12 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,20 +745,30 @@ macro_rules! maybe_break_monitor_err {
}

macro_rules! handle_chan_restoration_locked {
($self: expr, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
$raa: expr, $commitment_update: expr, $order: expr,
($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr,
$raa: expr, $commitment_update: expr, $order: expr, $chanmon_update: expr,
$pending_forwards: expr, $broadcast_safe: expr, $funding_locked: expr) => { {
let mut htlc_forwards = None;
let mut funding_broadcast_safe = None;
let counterparty_node_id = $channel_entry.get().get_counterparty_node_id();
let channel_id = $channel_entry.get().channel_id();

{
let res = loop {
if !$pending_forwards.is_empty() {
htlc_forwards = Some(($channel_entry.get().get_short_channel_id().expect("We can't have pending forwards before funding confirmation"),
$channel_entry.get().get_funding_txo().unwrap(), $pending_forwards));
}

macro_rules! handle_cs { () => {
if let Some(monitor_update) = $chanmon_update {
assert!($order == RAACommitmentOrder::RevokeAndACKFirst);
assert!(!$broadcast_safe);
assert!($funding_locked.is_none());
assert!($commitment_update.is_some());
if let Err(e) = $self.chain_monitor.update_channel($channel_entry.get().get_funding_txo().unwrap(), monitor_update) {
break handle_monitor_err!($self, e, $channel_state, $channel_entry, RAACommitmentOrder::CommitmentFirst, false, true);
}
}
if let Some(update) = $commitment_update {
$channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
node_id: counterparty_node_id,
Expand Down Expand Up @@ -801,21 +811,26 @@ macro_rules! handle_chan_restoration_locked {
msg: announcement_sigs,
});
}
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id());
$channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), channel_id);
}
}
(htlc_forwards, funding_broadcast_safe)
break Ok(());
};

(htlc_forwards, funding_broadcast_safe, res, channel_id, counterparty_node_id)
} }
}

macro_rules! post_handle_chan_restoration {
($self: expr, $locked_res: expr, $pending_failures: expr) => { {
let (htlc_forwards, funding_broadcast_safe) = $locked_res;
($self: ident, $locked_res: expr, $pending_failures: expr, $forwarding_failures: expr) => { {
let (htlc_forwards, funding_broadcast_safe, res, channel_id, counterparty_node_id) = $locked_res;

let _ = handle_error!($self, res, counterparty_node_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line also adds reentrancy risk, right? (in addition to the reentrancy risk of freeing the holding cell HTLCs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so, at least not from the PoV of calling user code - handle_error shouldn't ever call user code directly, only pushing events onto the message/regular event queues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's hard to untangle the macros, but handle_error does call finish_force_close, which looks like it can call update_channel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I see your point. Tracing it back a bit, I think the only way we have an Err in res here is if we break the handle_chan_restoration_locked loop with a handle_monitor_err result, which is precisely only if the monitor update that we generated failed.


if let Some(ev) = funding_broadcast_safe {
$self.pending_events.lock().unwrap().push(ev);
}

$self.fail_holding_cell_htlcs($forwarding_failures, channel_id);
for failure in $pending_failures.drain(..) {
$self.fail_htlc_backwards_internal($self.channel_state.lock().unwrap(), failure.0, &failure.1, failure.2);
}
Expand Down Expand Up @@ -2332,6 +2347,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
/// ChannelMonitorUpdateErr::TemporaryFailures is fine. The highest_applied_update_id field
/// exists largely only to prevent races between this and concurrent update_monitor calls.
///
/// In some cases, this may generate a monitor update, resulting in a call to the
/// `chain::Watch`'s `update_channel` method for the same channel monitor which is being
/// notified of a successful update here. Because of this, please be very careful with
/// reentrancy bugs! It is incredibly easy to write an implementation of `update_channel` which
/// will take a lock that is also held when calling this method.
///
/// Thus, the anticipated use is, at a high level:
/// 1) You register a chain::Watch with this ChannelManager,
/// 2) it stores each update to disk, and begins updating any remote (eg watchtower) copies of
Expand All @@ -2343,7 +2364,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
pub fn channel_monitor_updated(&self, funding_txo: &OutPoint, highest_applied_update_id: u64) {
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);

let (mut pending_failures, chan_restoration_res) = {
let (mut pending_failures, forwarding_failures, chan_restoration_res) = {
let mut channel_lock = self.channel_state.lock().unwrap();
let channel_state = &mut *channel_lock;
let mut channel = match channel_state.by_id.entry(funding_txo.to_channel_id()) {
Expand All @@ -2354,10 +2375,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
return;
}

let (raa, commitment_update, order, pending_forwards, pending_failures, needs_broadcast_safe, funding_locked) = channel.get_mut().monitor_updating_restored(&self.logger);
(pending_failures, handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, pending_forwards, needs_broadcast_safe, funding_locked))
let (raa, commitment_update, order, chanmon_update, pending_forwards, pending_failures, forwarding_failures, needs_broadcast_safe, funding_locked) = channel.get_mut().monitor_updating_restored(&self.logger);
(pending_failures, forwarding_failures, handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, chanmon_update, pending_forwards, needs_broadcast_safe, funding_locked))
};
post_handle_chan_restoration!(self, chan_restoration_res, pending_failures);
post_handle_chan_restoration!(self, chan_restoration_res, pending_failures, forwarding_failures);
}

fn internal_open_channel(&self, counterparty_node_id: &PublicKey, their_features: InitFeatures, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
Expand Down