Fix SCID removal when an HTLC is pending from 0.1 before upgrading

lightning/src/chain/channelmonitor.rs

@@ -7009,7 +7009,7 @@ mod tests {
 		let funding_outpoint = OutPoint { txid: Txid::all_zeros(), index: u16::MAX };
 		let channel_id = ChannelId::v1_from_funding_outpoint(funding_outpoint);
 		let channel_parameters = ChannelTransactionParameters {
-			holder_pubkeys: keys.new_pubkeys(None, &secp_ctx),
+			holder_pubkeys: keys.pubkeys(&secp_ctx),
 			holder_selected_contest_delay: 66,
 			is_outbound_from_holder: true,
 			counterparty_parameters: Some(CounterpartyChannelTransactionParameters {
@@ -7272,7 +7272,7 @@ mod tests {
 		let funding_outpoint = OutPoint { txid: Txid::all_zeros(), index: u16::MAX };
 		let channel_id = ChannelId::v1_from_funding_outpoint(funding_outpoint);
 		let channel_parameters = ChannelTransactionParameters {
-			holder_pubkeys: keys.new_pubkeys(None, &secp_ctx),
+			holder_pubkeys: keys.pubkeys(&secp_ctx),
 			holder_selected_contest_delay: 66,
 			is_outbound_from_holder: true,
 			counterparty_parameters: Some(CounterpartyChannelTransactionParameters {

lightning/src/chain/onchaintx.rs

@@ -1339,7 +1339,7 @@ mod tests {
 		// Use non-anchor channels so that HTLC-Timeouts are broadcast immediately instead of sent
 		// to the user for external funding.
 		let chan_params = ChannelTransactionParameters {
-			holder_pubkeys: signer.new_pubkeys(None, &secp_ctx),
+			holder_pubkeys: signer.pubkeys(&secp_ctx),
 			holder_selected_contest_delay: 66,
 			is_outbound_from_holder: true,
 			counterparty_parameters: Some(CounterpartyChannelTransactionParameters {

lightning/src/ln/chan_utils.rs

@@ -1018,11 +1018,11 @@ pub struct ChannelTransactionParameters {
 	/// If a channel was funded with transaction A, and later spliced with transaction B, this field
 	/// tracks the txid of transaction A.
 	///
-	/// See [`compute_funding_key_tweak`] and [`ChannelSigner::new_pubkeys`] for more context on how
+	/// See [`compute_funding_key_tweak`] and [`ChannelSigner::pubkeys`] for more context on how
 	/// this may be used.
 	///
 	/// [`compute_funding_key_tweak`]: crate::sign::compute_funding_key_tweak
-	/// [`ChannelSigner::new_pubkeys`]: crate::sign::ChannelSigner::new_pubkeys
+	/// [`ChannelSigner::pubkeys`]: crate::sign::ChannelSigner::pubkeys
 	pub splice_parent_funding_txid: Option<Txid>,
 	/// This channel's type, as negotiated during channel open. For old objects where this field
 	/// wasn't serialized, it will default to static_remote_key at deserialization.
@@ -2245,8 +2245,8 @@ mod tests {
 			let counterparty_signer = keys_provider.derive_channel_signer(keys_provider.generate_channel_keys_id(true, 1));
 			let per_commitment_secret = SecretKey::from_slice(&<Vec<u8>>::from_hex("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap();
 			let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
-			let holder_pubkeys = signer.new_pubkeys(None, &secp_ctx);
-			let counterparty_pubkeys = counterparty_signer.new_pubkeys(None, &secp_ctx).clone();
+			let holder_pubkeys = signer.pubkeys(&secp_ctx);
+			let counterparty_pubkeys = counterparty_signer.pubkeys(&secp_ctx).clone();
 			let channel_parameters = ChannelTransactionParameters {
 				holder_pubkeys: holder_pubkeys.clone(),
 				holder_selected_contest_delay: 0,

lightning/src/ln/channel.rs

@@ -2470,6 +2470,7 @@ impl FundingScope {
 	fn for_splice<SP: Deref>(
 		prev_funding: &Self, context: &ChannelContext<SP>, our_funding_contribution: SignedAmount,
 		their_funding_contribution: SignedAmount, counterparty_funding_pubkey: PublicKey,
+		our_new_holder_keys: ChannelPublicKeys,
 	) -> Self
 	where
 		SP::Target: SignerProvider,
@@ -2489,19 +2490,15 @@ impl FundingScope {
 		debug_assert!(post_value_to_self_msat.is_some());
 		let post_value_to_self_msat = post_value_to_self_msat.unwrap();
 
-		// Rotate the pubkeys using the prev_funding_txid as a tweak
-		let prev_funding_txid = prev_funding.get_funding_txid();
-		let holder_pubkeys = context.new_holder_pubkeys(prev_funding_txid);
-
 		let channel_parameters = &prev_funding.channel_transaction_parameters;
 		let mut post_channel_transaction_parameters = ChannelTransactionParameters {
-			holder_pubkeys,
+			holder_pubkeys: our_new_holder_keys,
 			holder_selected_contest_delay: channel_parameters.holder_selected_contest_delay,
 			// The 'outbound' attribute doesn't change, even if the splice initiator is the other node
 			is_outbound_from_holder: channel_parameters.is_outbound_from_holder,
 			counterparty_parameters: channel_parameters.counterparty_parameters.clone(),
 			funding_outpoint: None, // filled later
-			splice_parent_funding_txid: prev_funding_txid,
+			splice_parent_funding_txid: prev_funding.get_funding_txid(),
 			channel_type_features: channel_parameters.channel_type_features.clone(),
 			channel_value_satoshis: post_channel_value,
 		};
@@ -2637,6 +2634,7 @@ impl_writeable_tlv_based!(PendingFunding, {
 enum FundingNegotiation {
 	AwaitingAck {
 		context: FundingNegotiationContext,
+		new_holder_funding_key: PublicKey,
 	},
 	ConstructingTransaction {
 		funding: FundingScope,
@@ -2667,7 +2665,7 @@ impl FundingNegotiation {
 
 	fn is_initiator(&self) -> bool {
 		match self {
-			FundingNegotiation::AwaitingAck { context } => context.is_initiator,
+			FundingNegotiation::AwaitingAck { context, .. } => context.is_initiator,
 			FundingNegotiation::ConstructingTransaction { interactive_tx_constructor, .. } => {
 				interactive_tx_constructor.is_initiator()
 			},
@@ -3510,7 +3508,7 @@ where
 
 		// TODO(dual_funding): Checks for `funding_feerate_sat_per_1000_weight`?
 
-		let pubkeys = holder_signer.new_pubkeys(None, &secp_ctx);
+		let pubkeys = holder_signer.pubkeys(&secp_ctx);
 
 		let funding = FundingScope {
 			value_to_self_msat,
@@ -3748,7 +3746,7 @@ where
 			Err(_) => return Err(APIError::ChannelUnavailable { err: "Failed to get destination script".to_owned()}),
 		};
 
-		let pubkeys = holder_signer.new_pubkeys(None, &secp_ctx);
+		let pubkeys = holder_signer.pubkeys(&secp_ctx);
 		let temporary_channel_id = temporary_channel_id_fn.map(|f| f(&pubkeys))
 			.unwrap_or_else(|| ChannelId::temporary_from_entropy_source(entropy_source));
 
@@ -4111,16 +4109,6 @@ where
 		return &mut self.holder_signer;
 	}
 
-	/// Returns holder pubkeys to use for the channel.
-	fn new_holder_pubkeys(&self, prev_funding_txid: Option<Txid>) -> ChannelPublicKeys {
-		match &self.holder_signer {
-			ChannelSignerType::Ecdsa(ecdsa) => ecdsa.new_pubkeys(prev_funding_txid, &self.secp_ctx),
-			// TODO (taproot|arik)
-			#[cfg(taproot)]
-			_ => todo!(),
-		}
-	}
-
 	/// Only allowed immediately after deserialization if get_outbound_scid_alias returns 0,
 	/// indicating we were written by LDK prior to 0.0.106 which did not set outbound SCID aliases
 	/// or prior to any channel actions during `Channel` initialization.
@@ -6891,7 +6879,7 @@ macro_rules! maybe_create_splice_funding_failed {
 					.map(|funding| funding.get_channel_type().clone());
 
 				let (contributed_inputs, contributed_outputs) = match funding_negotiation {
-					FundingNegotiation::AwaitingAck { context } => {
+					FundingNegotiation::AwaitingAck { context, .. } => {
 						context.$contributed_inputs_and_outputs()
 					},
 					FundingNegotiation::ConstructingTransaction {
@@ -11962,17 +11950,29 @@ where
 			change_script,
 		};
 
+		// Rotate the funding pubkey using the prev_funding_txid as a tweak
+		let prev_funding_txid = self.funding.get_funding_txid();
+		let funding_pubkey = match (prev_funding_txid, &self.context.holder_signer) {
+			(None, _) => {
+				debug_assert!(false);
+				self.funding.get_holder_pubkeys().funding_pubkey
+			},
+			(Some(prev_funding_txid), ChannelSignerType::Ecdsa(ecdsa)) => {
+				ecdsa.new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx)
+			},
+			#[cfg(taproot)]
+			_ => todo!(),
+		};
+
+		let funding_negotiation =
+			FundingNegotiation::AwaitingAck { context, new_holder_funding_key: funding_pubkey };
 		self.pending_splice = Some(PendingFunding {
-			funding_negotiation: Some(FundingNegotiation::AwaitingAck { context }),
+			funding_negotiation: Some(funding_negotiation),
 			negotiated_candidates: vec![],
 			sent_funding_txid: None,
 			received_funding_txid: None,
 		});
 
-		// Rotate the pubkeys using the prev_funding_txid as a tweak
-		let prev_funding_txid = self.funding.get_funding_txid();
-		let funding_pubkey = self.context.new_holder_pubkeys(prev_funding_txid).funding_pubkey;
-
 		msgs::SpliceInit {
 			channel_id: self.context.channel_id,
 			funding_contribution_satoshis: adjusted_funding_contribution.to_sat(),
@@ -12056,12 +12056,29 @@ where
 		self.validate_splice_contributions(our_funding_contribution, their_funding_contribution)
 			.map_err(|e| ChannelError::WarnAndDisconnect(e))?;
 
+		// Rotate the pubkeys using the prev_funding_txid as a tweak
+		let prev_funding_txid = self.funding.get_funding_txid();
+		let funding_pubkey = match (prev_funding_txid, &self.context.holder_signer) {
+			(None, _) => {
+				debug_assert!(false);
+				self.funding.get_holder_pubkeys().funding_pubkey
+			},
+			(Some(prev_funding_txid), ChannelSignerType::Ecdsa(ecdsa)) => {
+				ecdsa.new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx)
+			},
+			#[cfg(taproot)]
+			_ => todo!(),
+		};
+		let mut new_keys = self.funding.get_holder_pubkeys().clone();
+		new_keys.funding_pubkey = funding_pubkey;
+
 		Ok(FundingScope::for_splice(
 			&self.funding,
 			&self.context,
 			our_funding_contribution,
 			their_funding_contribution,
 			msg.funding_pubkey,
+			new_keys,
 		))
 	}
 
@@ -12206,8 +12223,7 @@ where
 		// optimization, but for often-offline nodes it may be, as we may connect and immediately
 		// go into splicing from both sides.
 
-		let funding_pubkey = splice_funding.get_holder_pubkeys().funding_pubkey;
-
+		let new_funding_pubkey = splice_funding.get_holder_pubkeys().funding_pubkey;
 		self.pending_splice = Some(PendingFunding {
 			funding_negotiation: Some(FundingNegotiation::ConstructingTransaction {
 				funding: splice_funding,
@@ -12221,7 +12237,7 @@ where
 		Ok(msgs::SpliceAck {
 			channel_id: self.context.channel_id,
 			funding_contribution_satoshis: our_funding_contribution.to_sat(),
-			funding_pubkey,
+			funding_pubkey: new_funding_pubkey,
 			require_confirmed_inputs: None,
 		})
 	}
@@ -12247,13 +12263,14 @@ where
 		let pending_splice =
 			self.pending_splice.as_mut().expect("We should have returned an error earlier!");
 		// TODO: Good candidate for a let else statement once MSRV >= 1.65
-		let funding_negotiation_context = if let Some(FundingNegotiation::AwaitingAck { context }) =
-			pending_splice.funding_negotiation.take()
-		{
-			context
-		} else {
-			panic!("We should have returned an error earlier!");
-		};
+		let funding_negotiation_context =
+			if let Some(FundingNegotiation::AwaitingAck { context, .. }) =
+				pending_splice.funding_negotiation.take()
+			{
+				context
+			} else {
+				panic!("We should have returned an error earlier!");
+			};
 
 		let mut interactive_tx_constructor = funding_negotiation_context
 			.into_interactive_tx_constructor(
@@ -12284,13 +12301,17 @@ where
 	fn validate_splice_ack(&self, msg: &msgs::SpliceAck) -> Result<FundingScope, ChannelError> {
 		// TODO(splicing): Add check that we are the splice (quiescence) initiator
 
-		let funding_negotiation_context = match &self
+		let pending_splice = self
 			.pending_splice
 			.as_ref()
-			.ok_or(ChannelError::Ignore("Channel is not in pending splice".to_owned()))?
+			.ok_or_else(|| ChannelError::Ignore("Channel is not in pending splice".to_owned()))?;
+
+		let (funding_negotiation_context, new_holder_funding_key) = match &pending_splice
 			.funding_negotiation
 		{
-			Some(FundingNegotiation::AwaitingAck { context }) => context,
+			Some(FundingNegotiation::AwaitingAck { context, new_holder_funding_key }) => {
+				(context, new_holder_funding_key)
+			},
 			Some(FundingNegotiation::ConstructingTransaction { .. })
 			| Some(FundingNegotiation::AwaitingSignatures { .. }) => {
 				return Err(ChannelError::WarnAndDisconnect(
@@ -12309,12 +12330,16 @@ where
 		self.validate_splice_contributions(our_funding_contribution, their_funding_contribution)
 			.map_err(|e| ChannelError::WarnAndDisconnect(e))?;
 
+		let mut new_keys = self.funding.get_holder_pubkeys().clone();
+		new_keys.funding_pubkey = *new_holder_funding_key;
+
 		Ok(FundingScope::for_splice(
 			&self.funding,
 			&self.context,
 			our_funding_contribution,
 			their_funding_contribution,
 			msg.funding_pubkey,
+			new_keys,
 		))
 	}
 
@@ -16475,7 +16500,7 @@ mod tests {
 			[0; 32],
 		);
 
-		let holder_pubkeys = signer.new_pubkeys(None, &secp_ctx);
+		let holder_pubkeys = signer.pubkeys(&secp_ctx);
 		assert_eq!(holder_pubkeys.funding_pubkey.serialize()[..],
 				<Vec<u8>>::from_hex("023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb").unwrap()[..]);
 		let keys_provider = Keys { signer: signer.clone() };

lightning/src/sign/mod.rs

@@ -272,12 +272,12 @@ pub enum SpendableOutputDescriptor {
 	/// To derive the delayed payment key which is used to sign this input, you must pass the
 	/// holder [`InMemorySigner::delayed_payment_base_key`] (i.e., the private key which
 	/// corresponds to the [`ChannelPublicKeys::delayed_payment_basepoint`] in
-	/// [`ChannelSigner::new_pubkeys`]) and the provided
+	/// [`ChannelSigner::pubkeys`]) and the provided
 	/// [`DelayedPaymentOutputDescriptor::per_commitment_point`] to
 	/// [`chan_utils::derive_private_key`]. The DelayedPaymentKey can be generated without the
 	/// secret key using [`DelayedPaymentKey::from_basepoint`] and only the
 	/// [`ChannelPublicKeys::delayed_payment_basepoint`] which appears in
-	/// [`ChannelSigner::new_pubkeys`].
+	/// [`ChannelSigner::pubkeys`].
 	///
 	/// To derive the [`DelayedPaymentOutputDescriptor::revocation_pubkey`] provided here (which is
 	/// used in the witness script generation), you must pass the counterparty
@@ -292,7 +292,7 @@ pub enum SpendableOutputDescriptor {
 	/// [`chan_utils::get_revokeable_redeemscript`].
 	DelayedPaymentOutput(DelayedPaymentOutputDescriptor),
 	/// An output spendable exclusively by our payment key (i.e., the private key that corresponds
-	/// to the `payment_point` in [`ChannelSigner::new_pubkeys`]). The output type depends on the
+	/// to the `payment_point` in [`ChannelSigner::pubkeys`]). The output type depends on the
 	/// channel type negotiated.
 	///
 	/// On an anchor outputs channel, the witness in the spending input is:
@@ -792,19 +792,25 @@ pub trait ChannelSigner {
 	/// and pause future signing operations until this validation completes.
 	fn validate_counterparty_revocation(&self, idx: u64, secret: &SecretKey) -> Result<(), ()>;
 
-	/// Returns a *new* set of holder channel public keys and basepoints. They may be the same as a
-	/// previous value, but are also allowed to change arbitrarily. Signing methods must still
-	/// support signing for any keys which have ever been returned. This should only be called
-	/// either for new channels or new splices.
+	/// Returns the holder channel public keys and basepoints. This should only be called once
+	/// during channel creation and as such implementations are allowed undefined behavior if
+	/// called more than once.
 	///
-	/// `splice_parent_funding_txid` can be used to compute a tweak to rotate the funding key in the
-	/// 2-of-2 multisig script during a splice. See [`compute_funding_key_tweak`] for an example
-	/// tweak and more details.
+	/// This method is *not* asynchronous. Instead, the value must be computed locally or in
+	/// advance and cached.
+	fn pubkeys(&self, secp_ctx: &Secp256k1<secp256k1::All>) -> ChannelPublicKeys;
+
+	/// Returns a new funding pubkey (i.e. our public which is used in a 2-of-2 with the
+	/// counterparty's key to to lock the funds on-chain) for a spliced channel.
+	///
+	/// `splice_parent_funding_txid` can be used to compute a tweak with which to rotate the base
+	/// key (which will then be available later in signing operations via
+	/// [`ChannelTransactionParameters::splice_parent_funding_txid`]).
 	///
 	/// This method is *not* asynchronous. Instead, the value must be cached locally.
-	fn new_pubkeys(
-		&self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
-	) -> ChannelPublicKeys;
+	fn new_funding_pubkey(
+		&self, splice_parent_funding_txid: Txid, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> PublicKey;
 
 	/// Returns an arbitrary identifier describing the set of keys which are provided back to you in
 	/// some [`SpendableOutputDescriptor`] types. This should be sufficient to identify this
@@ -1457,17 +1463,13 @@ impl ChannelSigner for InMemorySigner {
 		Ok(())
 	}
 
-	fn new_pubkeys(
-		&self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
-	) -> ChannelPublicKeys {
+	fn pubkeys(&self, secp_ctx: &Secp256k1<secp256k1::All>) -> ChannelPublicKeys {
 		// Because splices always break downgrades, we go ahead and always use the new derivation
 		// here as its just much better.
-		let use_v2_derivation =
-			self.v2_remote_key_derivation || splice_parent_funding_txid.is_some();
 		let payment_key =
-			if use_v2_derivation { &self.payment_key_v2 } else { &self.payment_key_v1 };
+			if self.v2_remote_key_derivation { &self.payment_key_v2 } else { &self.payment_key_v1 };
 		let from_secret = |s: &SecretKey| PublicKey::from_secret_key(secp_ctx, s);
-		let mut pubkeys = ChannelPublicKeys {
+		let pubkeys = ChannelPublicKeys {
 			funding_pubkey: from_secret(&self.funding_key.0),
 			revocation_basepoint: RevocationBasepoint::from(from_secret(&self.revocation_base_key)),
 			payment_point: from_secret(payment_key),
@@ -1477,13 +1479,15 @@ impl ChannelSigner for InMemorySigner {
 			htlc_basepoint: HtlcBasepoint::from(from_secret(&self.htlc_base_key)),
 		};
 
-		if splice_parent_funding_txid.is_some() {
-			pubkeys.funding_pubkey =
-				self.funding_key(splice_parent_funding_txid).public_key(secp_ctx);
-		}
 		pubkeys
 	}
 
+	fn new_funding_pubkey(
+		&self, splice_parent_funding_txid: Txid, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> PublicKey {
+		self.funding_key(Some(splice_parent_funding_txid)).public_key(secp_ctx)
+	}
+
 	fn channel_keys_id(&self) -> [u8; 32] {
 		self.channel_keys_id
 	}