Properly handle funding key rotation during splices

When splicing, we're required by protocol to retain all the
existing keys material except the funding key which we're allowed
to rotate. In the original implementation we acknowledged that but
figured we'd stick with a single `pubkey` method in the
`ChannelSigner` anyway cause adding a specific method for it is
annoying.

Sadly, this was ultimately broken - in `FundingScope::for_splice`,
we called the signer's `new_pubkeys` method (renamed from `pubkeys`
after splicing initially landed), replacing all of the public keys
the `Channel` would use rather than just the funding key. This can
result in commitment signature mismatches if the signer changes any
keys aside from the funding one.

`InMemorySigner` did not do so, however, so we didn't notice the
bug. Luckily-ish, in 189b8ac we
started generating a fresh `remote_key` when splicing (at least
when upgrading from 0.1 to 0.2 or when setting `KeysManager` to use
v1 `remote_key` derivation). This breaks splicing cause we can't
communicate the new `remote_key` to the counterparty during the
splicing handshake.

Ultimately this bug is because the API we had didn't communicate to
the signer that we weren't allowed to change anything except the
funding key, and allowed returning a `ChannelPublicKeys` which
would break the channel.

Here we fix this by renaming `new_pubkeys` `pubkeys` again
(partially reverting 9d291e0 but
keeping the changed requirements that `pubkeys` only be called
once) and adding a new `ChannelSigner:new_funding_pubkey` method
specifically for splicing.

We also update `channel.rs` to correctly fetch the new funding
pubkey before sending `splice_init`, storing it in the
`PendingFunding` untl we build a `FundingScope`.

Author: TheBlueMatt

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
 	}