Lean on the holding cell for commitments when updating fees

TheBlueMatt · TheBlueMatt · commit aa9630b0cdb4 · 2022-12-06T18:18:26.000Z
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
@@ -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) => {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -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);
