Improve error handling, persistence; misc review changes

Author: optout21

lightning/src/ln/channel.rs

@@ -8377,74 +8377,66 @@ impl<SP: Deref> FundedChannel<SP> where
 		}
 	}
 
-	/// Initiate splicing
+	/// Initiate splicing.
+	/// - our_funding_inputs: the inputs we contribute to the new funding transaction.
+	///   Includes the witness weight for this input (e.g. P2WPKH_WITNESS_WEIGHT=109 for typical P2WPKH inputs).
 	#[cfg(splicing)]
 	pub fn splice_channel(&mut self, our_funding_contribution_satoshis: i64,
-		our_funding_inputs: Vec<(TxIn, Transaction, Weight)>, funding_feerate_perkw: u32, locktime: u32,
-	) -> Result<msgs::SpliceInit, ChannelError> {
+		_our_funding_inputs: &Vec<(TxIn, Transaction, Weight)>,
+		funding_feerate_per_kw: u32, locktime: u32,
+	) -> Result<msgs::SpliceInit, APIError> {
 		// Check if a splice has been initiated already.
-		// Note: this could be handled more nicely, and support multiple outstanding splice's, the incoming splice_ack matters anyways.
+		// Note: only a single outstanding splice is supported (per spec)
 		if let Some(splice_info) = &self.pending_splice_pre {
-			return Err(ChannelError::Warn(format!(
-				"Channel has already a splice pending, contribution {}", splice_info.our_funding_contribution
-			)));
+			return Err(APIError::APIMisuseError { err: format!(
+				"Channel {} cannot be spliced, as it has already a splice pending (contribution {})",
+				self.context.channel_id(), splice_info.our_funding_contribution
+			)});
 		}
 
-		if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) {
-			return Err(ChannelError::Warn(format!("Cannot initiate splicing, as channel is not Ready")));
+		if !self.context.is_live() {
+			return Err(APIError::APIMisuseError { err: format!(
+				"Channel {} cannot be spliced, as channel is not live",
+				self.context.channel_id()
+			)});
 		}
 
-		let pre_channel_value = self.funding.get_value_satoshis();
-		// Sanity check: capacity cannot decrease below 0
-		if (pre_channel_value as i64).saturating_add(our_funding_contribution_satoshis) < 0 {
-			return Err(ChannelError::Warn(format!(
-				"Post-splicing channel value cannot be negative. It was {} + {}",
-				pre_channel_value, our_funding_contribution_satoshis
-			)));
-		}
+		// TODO(splicing): check for quiescence
 
 		if our_funding_contribution_satoshis < 0 {
-			return Err(ChannelError::Warn(format!(
-				"TODO(splicing): Splice-out not supported, only splice in, contribution {}",
-				our_funding_contribution_satoshis,
-			)));
+			return Err(APIError::APIMisuseError { err: format!(
+				"TODO(splicing): Splice-out not supported, only splice in; channel ID {}, contribution {}",
+				self.context.channel_id(), our_funding_contribution_satoshis,
+			)});
 		}
 
-		// Note: post-splice channel value is not yet known at this point, counterpary contribution is not known
+		// TODO(splicing): Once splice-out is supported, check that channel balance does not go below 0
+		// (or below channel reserve)
+
+		// Note: post-splice channel value is not yet known at this point, counterparty contribution is not known
 		// (Cannot test for miminum required post-splice channel value)
 
-		// Check that inputs are sufficient to cover our contribution
-		let sum_input: i64 = our_funding_inputs.into_iter().map(
-			|(txin, tx, _)| tx.output.get(txin.previous_output.vout as usize).map(|tx| tx.value.to_sat() as i64).unwrap_or(0)
-		).sum();
-		if sum_input < our_funding_contribution_satoshis {
-			return Err(ChannelError::Warn(format!(
-				"Provided inputs are insufficient for our contribution, {} {}",
-				sum_input, our_funding_contribution_satoshis,
-			)));
-		}
 
 		self.pending_splice_pre = Some(PendingSplice {
 			our_funding_contribution: our_funding_contribution_satoshis,
 		});
 
-		let msg = self.get_splice_init(our_funding_contribution_satoshis, funding_feerate_perkw, locktime);
+		let msg = self.get_splice_init(our_funding_contribution_satoshis, funding_feerate_per_kw, locktime);
 		Ok(msg)
 	}
 
 	/// Get the splice message that can be sent during splice initiation.
 	#[cfg(splicing)]
 	pub fn get_splice_init(&self, our_funding_contribution_satoshis: i64,
-		funding_feerate_perkw: u32, locktime: u32,
+		funding_feerate_per_kw: u32, locktime: u32,
 	) -> msgs::SpliceInit {
-		// Reuse the existing funding pubkey, in spite of the channel value changing
-		// (though at this point we don't know the new value yet, due tue the optional counterparty contribution)
+		// TODO(splicing): The exisiting pubkey is reused, but a new one should be generated. See #3542.
 		// Note that channel_keys_id is supposed NOT to change
 		let funding_pubkey = self.funding.get_holder_pubkeys().funding_pubkey.clone();
 		msgs::SpliceInit {
 			channel_id: self.context.channel_id,
 			funding_contribution_satoshis: our_funding_contribution_satoshis,
-			funding_feerate_perkw,
+			funding_feerate_per_kw,
 			locktime,
 			funding_pubkey,
 			require_confirmed_inputs: None,
@@ -8466,20 +8458,11 @@ impl<SP: Deref> FundedChannel<SP> where
 			)));
 		}
 
-		if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) {
-			return Err(ChannelError::Warn(format!("Splicing requested on a channel that is not Ready")));
-		}
-
-		let pre_channel_value = self.funding.get_value_satoshis();
-		// Sanity check: capacity cannot decrease below 0
-		if (pre_channel_value as i64)
-			.saturating_add(their_funding_contribution_satoshis)
-			.saturating_add(our_funding_contribution_satoshis) < 0
-		{
-			return Err(ChannelError::Warn(format!(
-				"Post-splicing channel value cannot be negative. It was {} + {} + {}",
-				pre_channel_value, their_funding_contribution_satoshis, our_funding_contribution_satoshis,
-			)));
+		// - If it has received shutdown:
+		//   MUST send a warning and close the connection or send an error
+		//   and fail the channel.
+		if !self.context.is_live() {
+			return Err(ChannelError::Warn(format!("Splicing requested on a channel that is not live")));
 		}
 
 		if their_funding_contribution_satoshis.saturating_add(our_funding_contribution_satoshis) < 0 {
@@ -8489,11 +8472,12 @@ impl<SP: Deref> FundedChannel<SP> where
 			)));
 		}
 
-		let post_channel_value = PendingSplice::compute_post_value(pre_channel_value, their_funding_contribution_satoshis, our_funding_contribution_satoshis);
-		let post_balance = PendingSplice::add_checked(self.funding.value_to_self_msat, our_funding_contribution_satoshis);
-		// Early check for reserve requirement, assuming maximum balance of full channel value
-		// This will also be checked later at tx_complete
-		let _res = self.context.check_balance_meets_reserve_requirements(post_balance, post_channel_value)?;
+		// TODO(splicing): Once splice acceptor can contribute, check that inputs are sufficient,
+		// similarly to the check in `splice_channel`.
+
+		// Note on channel reserve requirement pre-check: as the splice acceptor does not contribute,
+		// it can't go below reserve, therefore no pre-check is done here.
+		// TODO(splicing): Once splice acceptor can contribute, add reserve pre-check, similar to the one in `splice_ack`.
 
 		// TODO(splicing): Store msg.funding_pubkey
 		// TODO(splicing): Apply start of splice (splice_start)
@@ -8506,7 +8490,8 @@ impl<SP: Deref> FundedChannel<SP> where
 	/// Get the splice_ack message that can be sent in response to splice initiation.
 	#[cfg(splicing)]
 	pub fn get_splice_ack(&self, our_funding_contribution_satoshis: i64) -> msgs::SpliceAck {
-		// Reuse the existing funding pubkey, in spite of the channel value changing
+		// TODO(splicing): The exisiting pubkey is reused, but a new one should be generated. See #3542.
+		// Note that channel_keys_id is supposed NOT to change
 		let funding_pubkey = self.funding.get_holder_pubkeys().funding_pubkey;
 		msgs::SpliceAck {
 			channel_id: self.context.channel_id,
@@ -12906,15 +12891,15 @@ mod tests {
 		);
 	}
 
-	#[cfg(all(test, splicing))]
+	#[cfg(splicing)]
 	fn get_pre_and_post(pre_channel_value: u64, our_funding_contribution: i64, their_funding_contribution: i64) -> (u64, u64) {
 		use crate::ln::channel::PendingSplice;
 
 		let post_channel_value = PendingSplice::compute_post_value(pre_channel_value, our_funding_contribution, their_funding_contribution);
 		(pre_channel_value, post_channel_value)
 	}
 
-	#[cfg(all(test, splicing))]
+	#[cfg(splicing)]
 	#[test]
 	fn test_splice_compute_post_value() {
 		{

lightning/src/ln/channelmanager.rs

@@ -30,9 +30,9 @@ use bitcoin::hash_types::{BlockHash, Txid};
 
 use bitcoin::secp256k1::{SecretKey,PublicKey};
 use bitcoin::secp256k1::Secp256k1;
-use bitcoin::{secp256k1, Sequence, TxIn};
+use bitcoin::{secp256k1, Sequence};
 #[cfg(splicing)]
-use bitcoin::Weight;
+use bitcoin::{TxIn, Weight};
 
 use crate::events::FundingInfo;
 use crate::blinded_path::message::{AsyncPaymentsContext, MessageContext, OffersContext};
@@ -4280,36 +4280,33 @@ where
 		}
 	}
 
-	/// Initiate a splice, to change the channel capacity of an existing funded channel.
-	/// After completion of splicing, the funding transaction will be replaced by a new one, spending the old funding transaction,
-	/// with optional extra inputs (splice-in) and/or extra outputs (splice-out or change).
-	/// TODO(splicing): Implementation is currently incomplete.
-	/// Note: Currently only splice-in is supported (increase in channel capacity), splice-out is not.
-	/// - our_funding_contribution_satoshis: the amount contributed by us to the channel. This will increase our channel balance.
-	/// - our_funding_inputs: the funding inputs provided by us. If our contribution is positive, our funding inputs must cover at least that amount.
+	/// See [`splice_channel`]
 	#[cfg(splicing)]
-	pub fn splice_channel(
+	fn internal_splice_channel(
 		&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, our_funding_contribution_satoshis: i64,
-		our_funding_inputs: Vec<(TxIn, Transaction, Weight)>, funding_feerate_perkw: u32, locktime: u32,
+		our_funding_inputs: &Vec<(TxIn, Transaction, Weight)>,
+		funding_feerate_per_kw: u32, locktime: Option<u32>,
 	) -> Result<(), APIError> {
 		let per_peer_state = self.per_peer_state.read().unwrap();
 
-		let peer_state_mutex = per_peer_state.get(counterparty_node_id)
-			.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) })?;
+		let peer_state_mutex = match per_peer_state.get(counterparty_node_id)
+			.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) }) {
+			Ok(p) => p,
+			Err(e) => return Err(e),
+		};
 
 		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 		let peer_state = &mut *peer_state_lock;
 
 		// Look for the channel
 		match peer_state.channel_by_id.entry(*channel_id) {
 			hash_map::Entry::Occupied(mut chan_phase_entry) => {
+				let locktime = locktime.unwrap_or(self.current_best_block().height);
 				if let Some(chan) = chan_phase_entry.get_mut().as_funded_mut() {
-					let msg = chan.splice_channel(our_funding_contribution_satoshis, our_funding_inputs, funding_feerate_perkw, locktime)
-						.map_err(|err| APIError::APIMisuseError {
-							err: format!(
-								"Cannot initiate Splicing, {}, channel ID {}", err, channel_id
-							)
-						})?;
+					let msg = match chan.splice_channel(our_funding_contribution_satoshis, our_funding_inputs, funding_feerate_per_kw, locktime) {
+						Ok(m) => m,
+						Err(e) => return Err(e),
+					};
 
 					peer_state.pending_msg_events.push(events::MessageSendEvent::SendSpliceInit {
 						node_id: *counterparty_node_id,
@@ -4327,16 +4324,47 @@ where
 				}
 			},
 			hash_map::Entry::Vacant(_) => {
-				return Err(APIError::ChannelUnavailable {
+				Err(APIError::ChannelUnavailable {
 					err: format!(
 						"Channel with id {} not found for the passed counterparty node_id {}",
 						channel_id, counterparty_node_id,
 					)
-				});
+				})
 			},
 		}
 	}
 
+	/// Initiate a splice, to change the channel capacity of an existing funded channel.
+	/// After completion of splicing, the funding transaction will be replaced by a new one, spending the old funding transaction,
+	/// with optional extra inputs (splice-in) and/or extra outputs (splice-out or change).
+	/// TODO(splicing): Implementation is currently incomplete.
+	///
+	/// Note: Currently only splice-in is supported (increase in channel capacity), splice-out is not.
+	///
+	/// - our_funding_contribution_satoshis: the amount contributed by us to the channel. This will increase our channel balance.
+	/// - our_funding_inputs: the funding inputs provided by us. If our contribution is positive, our funding inputs must cover at least that amount.
+	///   Includes the witness weight for this input (e.g. P2WPKH_WITNESS_WEIGHT=109 for typical P2WPKH inputs).
+	/// - locktime: Optional locktime for the new funding transaction. If None, set to the current block height.
+	#[cfg(splicing)]
+	pub fn splice_channel(
+		&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, our_funding_contribution_satoshis: i64,
+		our_funding_inputs: Vec<(TxIn, Transaction, Weight)>,
+		funding_feerate_per_kw: u32, locktime: Option<u32>,
+	) -> Result<(), APIError> {
+		let mut res = Ok(());
+		PersistenceNotifierGuard::optionally_notify(self, || {
+			let result = self.internal_splice_channel(
+				channel_id, counterparty_node_id, our_funding_contribution_satoshis, &our_funding_inputs, funding_feerate_per_kw, locktime
+			);
+			res = result;
+			match res {
+				Ok(_) => NotifyOption::SkipPersistHandleEvents,
+				Err(_) => NotifyOption::SkipPersistNoEvents,
+			}
+		});
+		res
+	}
+
 	fn can_forward_htlc_to_outgoing_channel(
 		&self, chan: &mut FundedChannel<SP>, msg: &msgs::UpdateAddHTLC, next_packet: &NextPacketDetails
 	) -> Result<(), (&'static str, u16)> {
@@ -9536,17 +9564,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				), msg.channel_id)),
 			hash_map::Entry::Occupied(mut chan_entry) => {
 				if let Some(chan) = chan_entry.get_mut().as_funded_mut() {
-					match chan.splice_init(msg) {
-						Ok(splice_ack_msg) => {
-							peer_state.pending_msg_events.push(events::MessageSendEvent::SendSpliceAck {
-								node_id: *counterparty_node_id,
-								msg: splice_ack_msg,
-							});
-						},
-						Err(err) => {
-							return Err(MsgHandleErrInternal::from_chan_no_close(err, msg.channel_id));
-						}
-					}
+					let splice_ack_msg = try_channel_entry!(self, peer_state, chan.splice_init(msg), chan_entry);
+					peer_state.pending_msg_events.push(events::MessageSendEvent::SendSpliceAck {
+						node_id: *counterparty_node_id,
+						msg: splice_ack_msg,
+					});
 				} else {
 					return Err(MsgHandleErrInternal::send_err_msg_no_close("Channel is not funded, cannot be spliced".to_owned(), msg.channel_id));
 				}
@@ -9579,14 +9601,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 					"Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}",
 					counterparty_node_id
 				), msg.channel_id)),
-			hash_map::Entry::Occupied(mut chan) => {
-				if let Some(chan) = chan.get_mut().as_funded_mut() {
-					match chan.splice_ack(msg) {
-						Ok(_) => {}
-						Err(err) => {
-							return Err(MsgHandleErrInternal::from_chan_no_close(err, msg.channel_id));
-						}
-					}
+			hash_map::Entry::Occupied(mut chan_entry) => {
+				if let Some(chan) = chan_entry.get_mut().as_funded_mut() {
+					try_channel_entry!(self, peer_state, chan.splice_ack(msg), chan_entry);
 				} else {
 					return Err(MsgHandleErrInternal::send_err_msg_no_close("Channel is not funded, cannot splice".to_owned(), msg.channel_id));
 				}
@@ -11883,7 +11900,7 @@ where
 			let persist = match &res {
 				Err(e) if e.closes_channel() => NotifyOption::DoPersist,
 				Err(_) => NotifyOption::SkipPersistHandleEvents,
-				Ok(()) => NotifyOption::SkipPersistNoEvents,
+				Ok(()) => NotifyOption::SkipPersistHandleEvents,
 			};
 			let _ = handle_error!(self, res, counterparty_node_id);
 			persist
@@ -11897,7 +11914,7 @@ where
 			let persist = match &res {
 				Err(e) if e.closes_channel() => NotifyOption::DoPersist,
 				Err(_) => NotifyOption::SkipPersistHandleEvents,
-				Ok(()) => NotifyOption::SkipPersistNoEvents,
+				Ok(()) => NotifyOption::SkipPersistHandleEvents,
 			};
 			let _ = handle_error!(self, res, counterparty_node_id);
 			persist

lightning/src/ln/functional_tests_splice.rs

@@ -72,7 +72,7 @@ fn test_v1_splice_in() {
 			splice_in_sats as i64,
 			funding_inputs,
 			funding_feerate_per_kw,
-			0, // locktime
+			None, // locktime
 		)
 		.unwrap();
 	// Extract the splice message from node0 to node1
@@ -82,7 +82,7 @@ fn test_v1_splice_in() {
 		acceptor_node.node.get_our_node_id()
 	);
 	assert_eq!(splice_init_msg.funding_contribution_satoshis, splice_in_sats as i64);
-	assert_eq!(splice_init_msg.funding_feerate_perkw, funding_feerate_per_kw);
+	assert_eq!(splice_init_msg.funding_feerate_per_kw, funding_feerate_per_kw);
 	assert_eq!(splice_init_msg.funding_pubkey.to_string(), expected_initiator_funding_key);
 	assert!(splice_init_msg.require_confirmed_inputs.is_none());
 

lightning/src/ln/msgs.rs

@@ -470,7 +470,7 @@ pub struct SpliceInit {
 	/// or remove from its channel balance (splice-out).
 	pub funding_contribution_satoshis: i64,
 	/// The feerate for the new funding transaction, set by the splice initiator
-	pub funding_feerate_perkw: u32,
+	pub funding_feerate_per_kw: u32,
 	/// The locktime for the new funding transaction
 	pub locktime: u32,
 	/// The key of the sender (splice initiator) controlling the new funding transaction
@@ -2239,7 +2239,7 @@ impl_writeable_msg!(Stfu, {
 impl_writeable_msg!(SpliceInit, {
 	channel_id,
 	funding_contribution_satoshis,
-	funding_feerate_perkw,
+	funding_feerate_per_kw,
 	locktime,
 	funding_pubkey,
 }, {
@@ -4099,7 +4099,7 @@ mod tests {
 		let splice_init = msgs::SpliceInit {
 			channel_id: ChannelId::from_bytes([2; 32]),
 			funding_contribution_satoshis: -123456,
-			funding_feerate_perkw: 2000,
+			funding_feerate_per_kw: 2000,
 			locktime: 0,
 			funding_pubkey: pubkey_1,
 			require_confirmed_inputs: Some(()),