From: Matt Corallo Date: Mon, 21 Nov 2022 01:13:52 +0000 (+0000) Subject: Lean on the holding cell for commitments when updating fees X-Git-Tag: v0.0.113~11^2~1 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=aa9630b0cdb4ff1dd4e4fcebc3ce0be09a41a109;p=rust-lightning Lean on the holding cell for commitments when updating fees Like the previous commit, here we update the update_fee+commit logic to simply push the fee update into the holding cell and then use the standard holding-cell-freeing codepaths to actually send the commitment update. This removes a substantial amount of code, reducing redundant codepaths and keeping channel state machine logic in channel.rs. --- diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e8bae2e91..b90165382 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3251,7 +3251,7 @@ impl Channel { 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 }; @@ -3551,12 +3551,22 @@ impl Channel { } } + /// 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(&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(&mut self, feerate_per_kw: u32, logger: &L) -> Option 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(&mut self, feerate_per_kw: u32, mut force_holding_cell: bool, logger: &L) -> Option where L::Target: Logger { if !self.is_outbound() { panic!("Cannot send fee from inbound channel"); } @@ -3593,6 +3603,10 @@ impl Channel { } 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; } @@ -3606,16 +3620,6 @@ impl Channel { }) } - pub fn send_update_fee_and_commit(&mut self, feerate_per_kw: u32, logger: &L) -> Result, 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. @@ -5502,7 +5506,7 @@ impl Channel { } /// 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: @@ -5513,10 +5517,10 @@ impl Channel { /// 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 if - /// `force_holding_cell` is false. + /// 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! + /// `Err`s will only be [`ChannelError::Ignore`]. fn send_htlc(&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, ChannelError> where L::Target: Logger { @@ -5652,41 +5656,6 @@ impl Channel { Ok(Some(res)) } - /// Creates a signed commitment transaction to send to the remote peer. - /// 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(&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(&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..."); @@ -5809,8 +5778,9 @@ impl Channel { /// 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(&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 { match self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, false, logger)? { Some(update_add_htlc) => { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d01ff8b19..63e70f83a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3586,59 +3586,24 @@ impl ChannelManager, chan_id: &[u8; 32], chan: &mut Channel<::Signer>, new_feerate: u32) -> (bool, NotifyOption, Result<(), MsgHandleErrInternal>) { - if !chan.is_outbound() { return (true, NotifyOption::SkipPersist, Ok(())); } + fn update_channel_fee(&self, chan_id: &[u8; 32], chan: &mut Channel<::Signer>, new_feerate: u32) -> NotifyOption { + if !chan.is_outbound() { return NotifyOption::SkipPersist; } // If the feerate has decreased by less than half, don't bother if new_feerate <= chan.get_feerate() && new_feerate * 2 > chan.get_feerate() { log_trace!(self.logger, "Channel {} does not qualify for a feerate change from {} to {}.", log_bytes!(chan_id[..]), chan.get_feerate(), new_feerate); - return (true, NotifyOption::SkipPersist, Ok(())); + return NotifyOption::SkipPersist; } if !chan.is_live() { log_trace!(self.logger, "Channel {} does not qualify for a feerate change from {} to {} as it cannot currently be updated (probably the peer is disconnected).", log_bytes!(chan_id[..]), chan.get_feerate(), new_feerate); - return (true, NotifyOption::SkipPersist, Ok(())); + return NotifyOption::SkipPersist; } log_trace!(self.logger, "Channel {} qualifies for a feerate change from {} to {}.", log_bytes!(chan_id[..]), chan.get_feerate(), new_feerate); - let mut retain_channel = true; - let res = match chan.send_update_fee_and_commit(new_feerate, &self.logger) { - Ok(res) => Ok(res), - Err(e) => { - let (drop, res) = convert_chan_err!(self, e, chan, chan_id); - if drop { retain_channel = false; } - Err(res) - } - }; - let ret_err = match res { - Ok(Some((update_fee, commitment_signed, monitor_update))) => { - match self.chain_monitor.update_channel(chan.get_funding_txo().unwrap(), monitor_update) { - ChannelMonitorUpdateStatus::Completed => { - pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { - node_id: chan.get_counterparty_node_id(), - updates: msgs::CommitmentUpdate { - update_add_htlcs: Vec::new(), - update_fulfill_htlcs: Vec::new(), - update_fail_htlcs: Vec::new(), - update_fail_malformed_htlcs: Vec::new(), - update_fee: Some(update_fee), - commitment_signed, - }, - }); - Ok(()) - }, - e => { - let (res, drop) = handle_monitor_update_res!(self, e, chan, RAACommitmentOrder::CommitmentFirst, chan_id, COMMITMENT_UPDATE_ONLY); - if drop { retain_channel = false; } - res - } - } - }, - Ok(None) => Ok(()), - Err(e) => Err(e), - }; - (retain_channel, NotifyOption::DoPersist, ret_err) + chan.queue_update_fee(new_feerate, &self.logger); + NotifyOption::DoPersist } #[cfg(fuzzing)] @@ -3652,19 +3617,10 @@ impl ChannelManager ChannelManager, _)> = Vec::new(); let mut timed_out_mpp_htlcs = Vec::new(); { let mut channel_state_lock = self.channel_state.lock().unwrap(); let channel_state = &mut *channel_state_lock; let pending_msg_events = &mut channel_state.pending_msg_events; channel_state.by_id.retain(|chan_id, chan| { - let counterparty_node_id = chan.get_counterparty_node_id(); - let (retain_channel, chan_needs_persist, err) = self.update_channel_fee(pending_msg_events, chan_id, chan, new_feerate); + let chan_needs_persist = self.update_channel_fee(chan_id, chan, new_feerate); if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; } - if err.is_err() { - handle_errors.push((err, counterparty_node_id)); - } - if !retain_channel { return false; } if let Err(e) = chan.timer_check_closing_negotiation_progress() { let (needs_close, err) = convert_chan_err!(self, e, chan, chan_id);