Lean on the holding cell for commitments when updating fees
authorMatt Corallo <git@bluematt.me>
Mon, 21 Nov 2022 01:13:52 +0000 (01:13 +0000)
committerMatt Corallo <git@bluematt.me>
Tue, 6 Dec 2022 18:18:26 +0000 (18:18 +0000)
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.

lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs

index e8bae2e917de3eb506088aafcb52b1a386490878..b9016538212fede1de44439c57dfeeee95655eec 100644 (file)
@@ -3251,7 +3251,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
                        };
@@ -3551,12 +3551,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");
                }
@@ -3593,6 +3603,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;
                }
@@ -3606,16 +3620,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.
@@ -5502,7 +5506,7 @@ impl<Signer: Sign> Channel<Signer> {
        }
 
        /// 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<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 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<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 {
@@ -5652,41 +5656,6 @@ impl<Signer: Sign> Channel<Signer> {
                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<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...");
@@ -5809,8 +5778,9 @@ 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, false, logger)? {
                        Some(update_add_htlc) => {
index d01ff8b1921cc53ce0e0f3f64864441cc5f13790..63e70f83a398f146dd16f538b3fe85fcf3cff105 100644 (file)
@@ -3586,59 +3586,24 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                self.process_background_events();
        }
 
-       fn update_channel_fee(&self, pending_msg_events: &mut Vec<events::MessageSendEvent>, chan_id: &[u8; 32], chan: &mut Channel<<K::Target as KeysInterface>::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<<K::Target as KeysInterface>::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<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
                        let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
-                       let mut handle_errors = 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 (retain_channel, chan_needs_persist, err) = self.update_channel_fee(pending_msg_events, chan_id, chan, new_feerate);
-                                       if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
-                                       if err.is_err() {
-                                               handle_errors.push(err);
-                                       }
-                                       retain_channel
-                               });
+                       let mut channel_state = self.channel_state.lock().unwrap();
+                       for (chan_id, chan) in channel_state.by_id.iter_mut() {
+                               let chan_needs_persist = self.update_channel_fee(chan_id, chan, new_feerate);
+                               if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
                        }
 
                        should_persist
@@ -3729,20 +3685,15 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
                        let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
-                       let mut handle_errors = Vec::new();
+                       let mut handle_errors: Vec<(Result<(), _>, _)> = 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);