Skip to content

Don't send init closing_signed too early after final HTLC removal #2529

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

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Don't send init closing_signed too early after final HTLC removal #2529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 31 additions & 0 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,19 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
#[cfg(not(test))]
closing_fee_limits: Option<(u64, u64)>,

/// If we remove an HTLC (or fee update), commit, and receive our counterparty's
/// `revoke_and_ack`, we remove all knowledge of said HTLC (or fee update). However, the latest
/// local commitment transaction that we can broadcast still contains the HTLC (or old fee)
/// until we receive a further `commitment_signed`. Thus we are not eligible for initiating the
/// `closing_signed` negotiation if we're expecting a counterparty `commitment_signed`.
///
/// To ensure we don't send a `closing_signed` too early, we track this state here, waiting
/// until we see a `commitment_signed` before doing so.
///
/// We don't bother to persist this - we anticipate this state won't last longer than a few
/// milliseconds, so any accidental force-closes here should be exceedingly rare.
expecting_peer_commitment_signed: bool,

/// The hash of the block in which the funding transaction was included.
funding_tx_confirmed_in: Option<BlockHash>,
funding_tx_confirmation_height: u32,
Expand Down Expand Up @@ -3248,6 +3261,7 @@ impl<SP: Deref> Channel<SP> where
};

self.context.cur_holder_commitment_transaction_number -= 1;
self.context.expecting_peer_commitment_signed = false;
// Note that if we need_commitment & !AwaitingRemoteRevoke we'll call
// build_commitment_no_status_check() next which will reset this to RAAFirst.
self.context.resend_order = RAACommitmentOrder::CommitmentFirst;
Expand Down Expand Up @@ -3513,6 +3527,7 @@ impl<SP: Deref> Channel<SP> where
// Take references explicitly so that we can hold multiple references to self.context.
let pending_inbound_htlcs: &mut Vec<_> = &mut self.context.pending_inbound_htlcs;
let pending_outbound_htlcs: &mut Vec<_> = &mut self.context.pending_outbound_htlcs;
let expecting_peer_commitment_signed = &mut self.context.expecting_peer_commitment_signed;

// We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug)
pending_inbound_htlcs.retain(|htlc| {
Expand All @@ -3521,6 +3536,7 @@ impl<SP: Deref> Channel<SP> where
if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
value_to_self_msat_diff += htlc.amount_msat as i64;
}
*expecting_peer_commitment_signed = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, can we use the existing mark_awaiting_response here? This will also make sure we disconnect them if they take too long or don't send it at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we do this more generally? Its an indication of an HTLC hang on the remote end which could still cause issues. I'd kinda like to do that as a followup, though, since its a separate concept and a lot more stuff to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

false
} else { true }
});
Expand Down Expand Up @@ -3580,6 +3596,7 @@ impl<SP: Deref> Channel<SP> where
if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", &htlc.payment_hash);
htlc.state = OutboundHTLCState::Committed;
*expecting_peer_commitment_signed = true;
}
if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
Expand All @@ -3600,6 +3617,7 @@ impl<SP: Deref> Channel<SP> where
log_trace!(logger, " ...promoting outbound fee update {} to Committed", feerate);
self.context.feerate_per_kw = feerate;
self.context.pending_update_fee = None;
self.context.expecting_peer_commitment_signed = true;
},
FeeUpdateState::RemoteAnnounced => { debug_assert!(!self.context.is_outbound()); },
FeeUpdateState::AwaitingRemoteRevokeToAnnounce => {
Expand Down Expand Up @@ -4380,6 +4398,10 @@ impl<SP: Deref> Channel<SP> where
-> Result<(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>), ChannelError>
where F::Target: FeeEstimator, L::Target: Logger
{
// If we're waiting on a monitor persistence, that implies we're also waiting to send some
// message to our counterparty (probably a `revoke_and_ack`). In such a case, we shouldn't
// initiate `closing_signed` negotiation until we're clear of all pending messages. Note
// that closing_negotiation_ready checks this case (as well as a few others).
if self.context.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
return Ok((None, None, None));
}
Expand All @@ -4391,6 +4413,12 @@ impl<SP: Deref> Channel<SP> where
return Ok((None, None, None));
}

// If we're waiting on a counterparty `commitment_signed` to clear some updates from our
// local commitment transaction, we can't yet initiate `closing_signed` negotiation.
if self.context.expecting_peer_commitment_signed {
return Ok((None, None, None));
}

let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);

assert!(self.context.shutdown_scriptpubkey.is_some());
Expand Down Expand Up @@ -6004,6 +6032,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {

last_sent_closing_fee: None,
pending_counterparty_closing_signed: None,
expecting_peer_commitment_signed: false,
closing_fee_limits: None,
target_closing_feerate_sats_per_kw: None,

Expand Down Expand Up @@ -6636,6 +6665,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {

last_sent_closing_fee: None,
pending_counterparty_closing_signed: None,
expecting_peer_commitment_signed: false,
closing_fee_limits: None,
target_closing_feerate_sats_per_kw: None,

Expand Down Expand Up @@ -7715,6 +7745,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch

last_sent_closing_fee: None,
pending_counterparty_closing_signed: None,
expecting_peer_commitment_signed: false,
closing_fee_limits: None,
target_closing_feerate_sats_per_kw,

Expand Down
102 changes: 101 additions & 1 deletion lightning/src/ln/shutdown_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
//! Tests of our shutdown and closing_signed negotiation logic.

use crate::sign::{EntropySource, SignerProvider};
use crate::chain::ChannelMonitorUpdateStatus;
use crate::chain::transaction::OutPoint;
use crate::events::{MessageSendEvent, MessageSendEventsProvider, ClosureReason};
use crate::events::{MessageSendEvent, HTLCDestination, MessageSendEventsProvider, ClosureReason};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: imports out of order

use crate::ln::channelmanager::{self, PaymentSendFailure, PaymentId, RecipientOnionFields, ChannelShutdownState, ChannelDetails};
use crate::routing::router::{PaymentParameters, get_route, RouteParameters};
use crate::ln::msgs;
Expand Down Expand Up @@ -1237,3 +1238,102 @@ fn simple_target_feerate_shutdown() {
check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
}

fn do_outbound_update_no_early_closing_signed(use_htlc: bool) {
// Previously, if we have a pending inbound HTLC (or fee update) on a channel which has
// initiated shutdown, we'd send our initial closing_signed immediately after receiving the
// peer's last RAA to remove the HTLC/fee update, but before receiving their final
// commitment_signed for a commitment without the HTLC/with the new fee. This caused at least
// LDK peers to force-close as we initiated closing_signed prior to the channel actually being
// fully empty of pending updates/HTLCs.
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;

send_payment(&nodes[0], &[&nodes[1]], 1_000_000);
let payment_hash_opt = if use_htlc {
Some(route_payment(&nodes[1], &[&nodes[0]], 10_000).1)
} else {
None
};

if use_htlc {
nodes[0].node.fail_htlc_backwards(&payment_hash_opt.unwrap());
expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[0],
[HTLCDestination::FailedPayment { payment_hash: payment_hash_opt.unwrap() }]);
} else {
*chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap() *= 10;
nodes[0].node.timer_tick_occurred();
}
let updates = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id());
check_added_monitors(&nodes[0], 1);

nodes[1].node.close_channel(&chan_id, &nodes[0].node.get_our_node_id()).unwrap();
let node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
nodes[0].node.close_channel(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();
let node_1_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());

nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_0_shutdown);
nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &node_1_shutdown);

if use_htlc {
nodes[1].node.handle_update_fail_htlc(&nodes[0].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
} else {
nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &updates.update_fee.unwrap());
}
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &updates.commitment_signed);
check_added_monitors(&nodes[1], 1);
let (bs_raa, bs_cs) = get_revoke_commit_msgs(&nodes[1], &nodes[0].node.get_our_node_id());

nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
check_added_monitors(&nodes[0], 1);

// At this point the Channel on nodes[0] has no record of any HTLCs but the latest
// broadcastable commitment does contain the HTLC (but only the ChannelMonitor knows this).
// Thus, the channel should not yet initiate closing_signed negotiation (but previously did).
assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());

chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_cs);
check_added_monitors(&nodes[0], 1);
assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());

expect_channel_shutdown_state!(nodes[0], chan_id, ChannelShutdownState::ResolvingHTLCs);
assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);

let as_raa_closing_signed = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(as_raa_closing_signed.len(), 2);

if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &as_raa_closing_signed[0] {
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &msg);
check_added_monitors(&nodes[1], 1);
if use_htlc {
expect_payment_failed!(nodes[1], payment_hash_opt.unwrap(), true);
}
} else { panic!("Unexpected message {:?}", as_raa_closing_signed[0]); }

if let MessageSendEvent::SendClosingSigned { msg, .. } = &as_raa_closing_signed[1] {
nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &msg);
} else { panic!("Unexpected message {:?}", as_raa_closing_signed[1]); }

let bs_closing_signed = get_event_msg!(nodes[1], MessageSendEvent::SendClosingSigned, nodes[0].node.get_our_node_id());
nodes[0].node.handle_closing_signed(&nodes[1].node.get_our_node_id(), &bs_closing_signed);
let (_, as_2nd_closing_signed) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id());
nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &as_2nd_closing_signed.unwrap());
let (_, node_1_none) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id());
assert!(node_1_none.is_none());

check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
}

#[test]
fn outbound_update_no_early_closing_signed() {
do_outbound_update_no_early_closing_signed(true);
do_outbound_update_no_early_closing_signed(false);
}