Skip to content

Lean on the holding cell when batch-forwarding/failing HTLCs #1863

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 4 commits into from
Dec 7, 2022
Merged

Lean on the holding cell when batch-forwarding/failing HTLCs #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cae7c81
Lean on the holding cell when batch-forwarding/failing HTLCs
TheBlueMatt Nov 19, 2022
1531378
Free the holding cells during background timer ticks
TheBlueMatt Nov 21, 2022
aa9630b
Lean on the holding cell for commitments when updating fees
TheBlueMatt Nov 21, 2022
1833070
Correct confusing docs on `Channel` methods
TheBlueMatt Nov 28, 2022
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
153 changes: 85 additions & 68 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1946,9 +1946,26 @@ impl<Signer: Sign> Channel<Signer> {
/// an HTLC more than once or fulfill once and then attempt to fail after reconnect. We cannot,
/// however, fail more than once as we wait for an upstream failure to be irrevocably committed
/// before we fail backwards.
/// If we do fail twice, we debug_assert!(false) and return Ok(None). Thus, will always return
/// Ok(_) if debug assertions are turned on or preconditions are met.
pub fn get_update_fail_htlc<L: Deref>(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket, logger: &L) -> Result<Option<msgs::UpdateFailHTLC>, ChannelError> where L::Target: Logger {
///
/// If we do fail twice, we `debug_assert!(false)` and return `Ok(None)`. Thus, this will always
/// return `Ok(_)` if preconditions are met. In any case, `Err`s will only be
/// [`ChannelError::Ignore`].
pub fn queue_fail_htlc<L: Deref>(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket, logger: &L)
-> Result<(), ChannelError> where L::Target: Logger {
self.fail_htlc(htlc_id_arg, err_packet, true, logger)
.map(|msg_opt| assert!(msg_opt.is_none(), "We forced holding cell?"))
}

/// We can only have one resolution per HTLC. In some cases around reconnect, we may fulfill
/// an HTLC more than once or fulfill once and then attempt to fail after reconnect. We cannot,
/// however, fail more than once as we wait for an upstream failure to be irrevocably committed
/// before we fail backwards.
///
/// If we do fail twice, we `debug_assert!(false)` and return `Ok(None)`. Thus, this will always
/// return `Ok(_)` if preconditions are met. In any case, `Err`s will only be
/// [`ChannelError::Ignore`].
fn fail_htlc<L: Deref>(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket, mut force_holding_cell: bool, logger: &L)
-> Result<Option<msgs::UpdateFailHTLC>, ChannelError> where L::Target: Logger {
if (self.channel_state & (ChannelState::ChannelReady as u32)) != (ChannelState::ChannelReady as u32) {
panic!("Was asked to fail an HTLC when channel was not in an operational state");
}
Expand Down Expand Up @@ -1986,8 +2003,13 @@ impl<Signer: Sign> Channel<Signer> {
return Ok(None);
}

// Now update local state:
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 {
debug_assert!(force_holding_cell, "!force_holding_cell is only called when emptying the holding cell, so we shouldn't end up back in it!");
force_holding_cell = true;
}

// Now update local state:
if force_holding_cell {
for pending_update in self.holding_cell_htlc_updates.iter() {
match pending_update {
&HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => {
Expand Down Expand Up @@ -3146,8 +3168,8 @@ impl<Signer: Sign> Channel<Signer> {
} else { Ok((None, Vec::new())) }
}

/// Used to fulfill holding_cell_htlcs when we get a remote ack (or implicitly get it by them
/// fulfilling or failing the last pending HTLC)
/// Frees any pending commitment updates in the holding cell, generating the relevant messages
/// for our counterparty.
fn free_holding_cell_htlcs<L: Deref>(&mut self, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger {
assert_eq!(self.channel_state & ChannelState::MonitorUpdateInProgress as u32, 0);
if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() {
Expand All @@ -3173,7 +3195,7 @@ impl<Signer: Sign> Channel<Signer> {
// to rebalance channels.
match &htlc_update {
&HTLCUpdateAwaitingACK::AddHTLC {amount_msat, cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet, ..} => {
match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone(), logger) {
match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone(), false, logger) {
Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()),
Err(e) => {
match e {
Expand Down Expand Up @@ -3209,13 +3231,13 @@ impl<Signer: Sign> Channel<Signer> {
monitor_update.updates.append(&mut additional_monitor_update.updates);
},
&HTLCUpdateAwaitingACK::FailHTLC { htlc_id, ref err_packet } => {
match self.get_update_fail_htlc(htlc_id, err_packet.clone(), logger) {
match self.fail_htlc(htlc_id, err_packet.clone(), false, logger) {
Ok(update_fail_msg_option) => {
// If an HTLC failure was previously added to the holding cell (via
// `get_update_fail_htlc`) then generating the fail message itself
// must not fail - we should never end up in a state where we
// double-fail an HTLC or fail-then-claim an HTLC as it indicates
// we didn't wait for a full revocation before failing.
// `queue_fail_htlc`) then generating the fail message itself must
// not fail - we should never end up in a state where we double-fail
// an HTLC or fail-then-claim an HTLC as it indicates we didn't wait
// for a full revocation before failing.
update_fail_htlcs.push(update_fail_msg_option.unwrap())
},
Err(e) => {
Expand All @@ -3232,7 +3254,7 @@ impl<Signer: Sign> Channel<Signer> {
return Ok((None, htlcs_to_fail));
}
let update_fee = if let Some(feerate) = self.holding_cell_update_fee.take() {
self.send_update_fee(feerate, logger)
self.send_update_fee(feerate, false, logger)
} else {
None
};
Expand Down Expand Up @@ -3532,12 +3554,22 @@ impl<Signer: Sign> Channel<Signer> {
}
}

/// Queues up an outbound update fee by placing it in the holding cell. You should call
/// [`Self::maybe_free_holding_cell_htlcs`] in order to actually generate and send the
/// commitment update.
pub fn queue_update_fee<L: Deref>(&mut self, feerate_per_kw: u32, logger: &L) where L::Target: Logger {
let msg_opt = self.send_update_fee(feerate_per_kw, true, logger);
assert!(msg_opt.is_none(), "We forced holding cell?");
}

/// Adds a pending update to this channel. See the doc for send_htlc for
/// further details on the optionness of the return value.
/// If our balance is too low to cover the cost of the next commitment transaction at the
/// new feerate, the update is cancelled.
/// You MUST call send_commitment prior to any other calls on this Channel
fn send_update_fee<L: Deref>(&mut self, feerate_per_kw: u32, logger: &L) -> Option<msgs::UpdateFee> where L::Target: Logger {
///
/// You MUST call [`Self::send_commitment_no_state_update`] prior to any other calls on this
/// [`Channel`] if `force_holding_cell` is false.
fn send_update_fee<L: Deref>(&mut self, feerate_per_kw: u32, mut force_holding_cell: bool, logger: &L) -> Option<msgs::UpdateFee> where L::Target: Logger {
if !self.is_outbound() {
panic!("Cannot send fee from inbound channel");
}
Expand Down Expand Up @@ -3574,6 +3606,10 @@ impl<Signer: Sign> Channel<Signer> {
}

if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 {
force_holding_cell = true;
}

if force_holding_cell {
self.holding_cell_update_fee = Some(feerate_per_kw);
return None;
}
Expand All @@ -3587,16 +3623,6 @@ impl<Signer: Sign> Channel<Signer> {
})
}

pub fn send_update_fee_and_commit<L: Deref>(&mut self, feerate_per_kw: u32, logger: &L) -> Result<Option<(msgs::UpdateFee, msgs::CommitmentSigned, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
match self.send_update_fee(feerate_per_kw, logger) {
Some(update_fee) => {
let (commitment_signed, monitor_update) = self.send_commitment_no_status_check(logger)?;
Ok(Some((update_fee, commitment_signed, monitor_update)))
},
None => Ok(None)
}
}

/// Removes any uncommitted inbound HTLCs and resets the state of uncommitted outbound HTLC
/// updates, to be used on peer disconnection. After this, update_*_htlc messages need to be
/// resent.
Expand Down Expand Up @@ -5470,8 +5496,26 @@ impl<Signer: Sign> Channel<Signer> {

// Send stuff to our remote peers:

/// Queues up an outbound HTLC to send by placing it in the holding cell. You should call
/// [`Self::maybe_free_holding_cell_htlcs`] in order to actually generate and send the
/// commitment update.
///
/// `Err`s will only be [`ChannelError::Ignore`].
pub fn queue_add_htlc<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
onion_routing_packet: msgs::OnionPacket, logger: &L)
-> Result<(), ChannelError> where L::Target: Logger {
self
.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, true, logger)
.map(|msg_opt| assert!(msg_opt.is_none(), "We forced holding cell?"))
.map_err(|err| {
if let ChannelError::Ignore(_) = err { /* fine */ }
else { debug_assert!(false, "Queueing cannot trigger channel failure"); }
err
})
}

/// Adds a pending outbound HTLC to this channel, note that you probably want
/// send_htlc_and_commit instead cause you'll want both messages at once.
/// [`Self::send_htlc_and_commit`] instead cause you'll want both messages at once.
///
/// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
/// the wire:
Expand All @@ -5482,10 +5526,13 @@ impl<Signer: Sign> Channel<Signer> {
/// we may not yet have sent the previous commitment update messages and will need to
/// regenerate them.
///
/// You MUST call send_commitment prior to calling any other methods on this Channel!
/// You MUST call [`Self::send_commitment_no_state_update`] prior to calling any other methods
/// on this [`Channel`] if `force_holding_cell` is false.
///
/// If an Err is returned, it's a ChannelError::Ignore!
pub fn send_htlc<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, logger: &L) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> where L::Target: Logger {
/// `Err`s will only be [`ChannelError::Ignore`].
fn send_htlc<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
onion_routing_packet: msgs::OnionPacket, mut force_holding_cell: bool, logger: &L)
-> Result<Option<msgs::UpdateAddHTLC>, ChannelError> where L::Target: Logger {
if (self.channel_state & (ChannelState::ChannelReady as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelReady as u32) {
return Err(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
}
Expand Down Expand Up @@ -5580,8 +5627,12 @@ impl<Signer: Sign> Channel<Signer> {
return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat)));
}

// Now update local state:
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 {
force_holding_cell = true;
}

// Now update local state:
if force_holding_cell {
self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
amount_msat,
payment_hash,
Expand Down Expand Up @@ -5614,41 +5665,6 @@ impl<Signer: Sign> Channel<Signer> {
Ok(Some(res))
}

/// Creates a signed commitment transaction to send to the remote peer.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think ideally this would've been removed in the previous-previous commit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.

/// Always returns a ChannelError::Close if an immediately-preceding (read: the
/// last call to this Channel) send_htlc returned Ok(Some(_)) and there is an Err.
/// May panic if called except immediately after a successful, Ok(Some(_))-returning send_htlc.
pub fn send_commitment<L: Deref>(&mut self, logger: &L) -> Result<(msgs::CommitmentSigned, ChannelMonitorUpdate), ChannelError> where L::Target: Logger {
if (self.channel_state & (ChannelState::ChannelReady as u32)) != (ChannelState::ChannelReady as u32) {
panic!("Cannot create commitment tx until channel is fully established");
}
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
panic!("Cannot create commitment tx until remote revokes their previous commitment");
}
if (self.channel_state & (ChannelState::PeerDisconnected as u32)) == (ChannelState::PeerDisconnected as u32) {
panic!("Cannot create commitment tx while disconnected, as send_htlc will have returned an Err so a send_commitment precondition has been violated");
}
if (self.channel_state & (ChannelState::MonitorUpdateInProgress as u32)) == (ChannelState::MonitorUpdateInProgress as u32) {
panic!("Cannot create commitment tx while awaiting monitor update unfreeze, as send_htlc will have returned an Err so a send_commitment precondition has been violated");
}
let mut have_updates = self.is_outbound() && self.pending_update_fee.is_some();
for htlc in self.pending_outbound_htlcs.iter() {
if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
have_updates = true;
}
if have_updates { break; }
}
for htlc in self.pending_inbound_htlcs.iter() {
if let InboundHTLCState::LocalRemoved(_) = htlc.state {
have_updates = true;
}
if have_updates { break; }
}
if !have_updates {
panic!("Cannot create commitment tx until we have some updates to send");
}
self.send_commitment_no_status_check(logger)
}
/// Only fails in case of bad keys
fn send_commitment_no_status_check<L: Deref>(&mut self, logger: &L) -> Result<(msgs::CommitmentSigned, ChannelMonitorUpdate), ChannelError> where L::Target: Logger {
log_trace!(logger, "Updating HTLC state for a newly-sent commitment_signed...");
Expand Down Expand Up @@ -5771,10 +5787,11 @@ impl<Signer: Sign> Channel<Signer> {

/// Adds a pending outbound HTLC to this channel, and creates a signed commitment transaction
/// to send to the remote peer in one go.
/// Shorthand for calling send_htlc() followed by send_commitment(), see docs on those for
/// more info.
///
/// Shorthand for calling [`Self::send_htlc`] followed by a commitment update, see docs on
/// [`Self::send_htlc`] and [`Self::send_commitment_no_state_update`] for more info.
pub fn send_htlc_and_commit<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, logger: &L) -> Result<Option<(msgs::UpdateAddHTLC, msgs::CommitmentSigned, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
match self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, logger)? {
match self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, false, logger)? {
Some(update_add_htlc) => {
let (commitment_signed, monitor_update) = self.send_commitment_no_status_check(logger)?;
Ok(Some((update_add_htlc, commitment_signed, monitor_update)))
Expand Down
Loading