Convert ChannelSigner::pubkeys to only fetch *new* pubkeys

The `remote_key` derived by default in `KeysManager` depends on the
chanel's `channel_keys_id`, which generally has sufficient entropy
that without it the `remote_key` cannot be re-derived. In disaster
case where there is no remaining state except the `KeysManager`'s
`seed`, this results in lost funds, even if the counterparty
force-closes the channel.

Luckily, because of the `static_remote_key` feature, there's no
need for this. If the `remote_key` we derive is one of a countable
set, we can simply scan the chain for outputs to our `remote_key`s.

In the next commit, we'll start using different `remote_key`s based
on a config knob the user sets, but with the current
`ChannelSigner::pubkeys` API this would be invalid - we can't
return a different set of keys for a re-derived `ChannelSigner`.
Luckily, this isn't actually how LDK uses `ChannelSigner::pubkeys`,
it actually only calls it when it wants a new set of pubkeys,
either for a new channel or a splice.

Thus, here, we rename `ChannelSigner::pubkeys` to
`ChannelSigner::new_pubkeys` and update documentation to match.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -382,11 +382,9 @@ impl SignerProvider for KeyProvider {
 	}
 
 	fn derive_channel_signer(&self, channel_keys_id: [u8; 32]) -> Self::EcdsaSigner {
-		let secp_ctx = Secp256k1::signing_only();
 		let id = channel_keys_id[0];
 		#[rustfmt::skip]
 		let keys = InMemorySigner::new(
-			&secp_ctx,
 			SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, self.node_secret[31]]).unwrap(),
 			SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, self.node_secret[31]]).unwrap(),
 			// We leave both the v1 and v2 derivation to_remote keys the same as there's not any

fuzz/src/full_stack.rs

@@ -458,7 +458,6 @@ impl SignerProvider for KeyProvider {
 	}
 
 	fn derive_channel_signer(&self, keys_id: [u8; 32]) -> Self::EcdsaSigner {
-		let secp_ctx = Secp256k1::signing_only();
 		let ctr = keys_id[0];
 		let (inbound, state) = self.signer_state.borrow().get(&ctr).unwrap().clone();
 
@@ -479,7 +478,7 @@ impl SignerProvider for KeyProvider {
 		f = key;
 		// We leave both the v1 and v2 derivation to_remote keys the same as there's not any real
 		// reason to fuzz differences here, and it keeps us consistent with past behavior.
-		let signer = InMemorySigner::new(&secp_ctx, a, b, c, c, d, e, f, keys_id, keys_id);
+		let signer = InMemorySigner::new(a, b, c, c, d, e, f, keys_id, keys_id);
 
 		TestChannelSigner::new_with_revoked(DynSigner::new(signer), state, false, false)
 	}

lightning/src/chain/channelmonitor.rs

@@ -6699,7 +6699,7 @@ mod tests {
 	use crate::ln::functional_test_utils::*;
 	use crate::ln::script::ShutdownScript;
 	use crate::ln::types::ChannelId;
-	use crate::sign::InMemorySigner;
+	use crate::sign::{ChannelSigner, InMemorySigner};
 	use crate::sync::Arc;
 	use crate::types::features::ChannelTypeFeatures;
 	use crate::types::payment::{PaymentHash, PaymentPreimage};
@@ -6872,7 +6872,6 @@ mod tests {
 		}
 
 		let keys = InMemorySigner::new(
-			&secp_ctx,
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
@@ -6894,7 +6893,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.holder_channel_pubkeys.clone(),
+			holder_pubkeys: keys.new_pubkeys(None, &secp_ctx),
 			holder_selected_contest_delay: 66,
 			is_outbound_from_holder: true,
 			counterparty_parameters: Some(CounterpartyChannelTransactionParameters {
@@ -7135,7 +7134,6 @@ mod tests {
 		let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 
 		let keys = InMemorySigner::new(
-			&secp_ctx,
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
@@ -7157,7 +7155,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.holder_channel_pubkeys.clone(),
+			holder_pubkeys: keys.new_pubkeys(None, &secp_ctx),
 			holder_selected_contest_delay: 66,
 			is_outbound_from_holder: true,
 			counterparty_parameters: Some(CounterpartyChannelTransactionParameters {

lightning/src/chain/onchaintx.rs

@@ -1287,7 +1287,7 @@ mod tests {
 	};
 	use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint};
 	use crate::ln::functional_test_utils::create_dummy_block;
-	use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, InMemorySigner};
+	use crate::sign::{ChannelDerivationParameters, ChannelSigner, HTLCDescriptor, InMemorySigner};
 	use crate::types::payment::{PaymentHash, PaymentPreimage};
 	use crate::util::test_utils::{TestBroadcaster, TestFeeEstimator, TestLogger};
 
@@ -1301,7 +1301,6 @@ mod tests {
 	fn test_broadcast_height() {
 		let secp_ctx = Secp256k1::new();
 		let signer = InMemorySigner::new(
-			&secp_ctx,
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
@@ -1339,7 +1338,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.holder_channel_pubkeys.clone(),
+			holder_pubkeys: signer.new_pubkeys(None, &secp_ctx),
 			holder_selected_contest_delay: 66,
 			is_outbound_from_holder: true,
 			counterparty_parameters: Some(CounterpartyChannelTransactionParameters {

lightning/src/ln/chan_utils.rs

@@ -1014,11 +1014,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::pubkeys`] for more context on how
+	/// See [`compute_funding_key_tweak`] and [`ChannelSigner::new_pubkeys`] for more context on how
 	/// this may be used.
 	///
 	/// [`compute_funding_key_tweak`]: crate::sign::compute_funding_key_tweak
-	/// [`ChannelSigner::pubkeys`]: crate::sign::ChannelSigner::pubkeys
+	/// [`ChannelSigner::new_pubkeys`]: crate::sign::ChannelSigner::new_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.
@@ -2240,8 +2240,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.pubkeys(None, &secp_ctx);
-			let counterparty_pubkeys = counterparty_signer.pubkeys(None, &secp_ctx).clone();
+			let holder_pubkeys = signer.new_pubkeys(None, &secp_ctx);
+			let counterparty_pubkeys = counterparty_signer.new_pubkeys(None, &secp_ctx).clone();
 			let channel_parameters = ChannelTransactionParameters {
 				holder_pubkeys: holder_pubkeys.clone(),
 				holder_selected_contest_delay: 0,

lightning/src/ln/channel.rs

@@ -2380,7 +2380,7 @@ impl FundingScope {
 
 		// Rotate the pubkeys using the prev_funding_txid as a tweak
 		let prev_funding_txid = prev_funding.get_funding_txid();
-		let holder_pubkeys = context.holder_pubkeys(prev_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 {
@@ -3365,7 +3365,7 @@ where
 
 		// TODO(dual_funding): Checks for `funding_feerate_sat_per_1000_weight`?
 
-		let pubkeys = holder_signer.pubkeys(None, &secp_ctx);
+		let pubkeys = holder_signer.new_pubkeys(None, &secp_ctx);
 
 		let funding = FundingScope {
 			value_to_self_msat,
@@ -3603,7 +3603,7 @@ where
 			Err(_) => return Err(APIError::ChannelUnavailable { err: "Failed to get destination script".to_owned()}),
 		};
 
-		let pubkeys = holder_signer.pubkeys(None, &secp_ctx);
+		let pubkeys = holder_signer.new_pubkeys(None, &secp_ctx);
 		let temporary_channel_id = temporary_channel_id_fn.map(|f| f(&pubkeys))
 			.unwrap_or_else(|| ChannelId::temporary_from_entropy_source(entropy_source));
 
@@ -3967,9 +3967,9 @@ where
 	}
 
 	/// Returns holder pubkeys to use for the channel.
-	fn holder_pubkeys(&self, prev_funding_txid: Option<Txid>) -> ChannelPublicKeys {
+	fn new_holder_pubkeys(&self, prev_funding_txid: Option<Txid>) -> ChannelPublicKeys {
 		match &self.holder_signer {
-			ChannelSignerType::Ecdsa(ecdsa) => ecdsa.pubkeys(prev_funding_txid, &self.secp_ctx),
+			ChannelSignerType::Ecdsa(ecdsa) => ecdsa.new_pubkeys(prev_funding_txid, &self.secp_ctx),
 			// TODO (taproot|arik)
 			#[cfg(taproot)]
 			_ => todo!(),
@@ -11553,7 +11553,7 @@ where
 
 		// 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.holder_pubkeys(prev_funding_txid).funding_pubkey;
+		let funding_pubkey = self.context.new_holder_pubkeys(prev_funding_txid).funding_pubkey;
 
 		Ok(msgs::SpliceInit {
 			channel_id: self.context.channel_id,
@@ -15983,7 +15983,6 @@ mod tests {
 		let secp_ctx = Secp256k1::new();
 
 		let signer = InMemorySigner::new(
-			&secp_ctx,
 			SecretKey::from_slice(&<Vec<u8>>::from_hex("30ff4956bbdd3222d44cc5e8a1261dab1e07957bdac5ae88fe3261ef321f3749").unwrap()[..]).unwrap(),
 			SecretKey::from_slice(&<Vec<u8>>::from_hex("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
 			SecretKey::from_slice(&<Vec<u8>>::from_hex("1111111111111111111111111111111111111111111111111111111111111111").unwrap()[..]).unwrap(),
@@ -15997,7 +15996,7 @@ mod tests {
 			[0; 32],
 		);
 
-		let holder_pubkeys = signer.pubkeys(None, &secp_ctx);
+		let holder_pubkeys = signer.new_pubkeys(None, &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

@@ -270,11 +270,14 @@ pub enum SpendableOutputDescriptor {
 	/// it is an output from an old state which we broadcast (which should never happen).
 	///
 	/// 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::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::pubkeys`].
+	/// 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
+	/// [`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`].
 	///
 	/// To derive the [`DelayedPaymentOutputDescriptor::revocation_pubkey`] provided here (which is
 	/// used in the witness script generation), you must pass the counterparty
@@ -289,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::pubkeys`]). The output type depends on the
+	/// to the `payment_point` in [`ChannelSigner::new_pubkeys`]). The output type depends on the
 	/// channel type negotiated.
 	///
 	/// On an anchor outputs channel, the witness in the spending input is:
@@ -789,14 +792,17 @@ pub trait ChannelSigner {
 	/// and pause future signing operations until this validation completes.
 	fn validate_counterparty_revocation(&self, idx: u64, secret: &SecretKey) -> Result<(), ()>;
 
-	/// Returns the holder's channel public keys and basepoints.
+	/// 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.
 	///
 	/// `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 cached locally.
-	fn pubkeys(
+	fn new_pubkeys(
 		&self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> ChannelPublicKeys;
 
@@ -1095,7 +1101,7 @@ mod sealed {
 	use bitcoin::secp256k1::{Scalar, SecretKey};
 
 	#[derive(Clone, PartialEq)]
-	pub struct MaybeTweakedSecretKey(SecretKey);
+	pub struct MaybeTweakedSecretKey(pub(super) SecretKey);
 
 	impl From<SecretKey> for MaybeTweakedSecretKey {
 		fn from(value: SecretKey) -> Self {
@@ -1163,8 +1169,6 @@ pub struct InMemorySigner {
 	pub htlc_base_key: SecretKey,
 	/// Commitment seed.
 	pub commitment_seed: [u8; 32],
-	/// Holder public keys and basepoints.
-	pub(crate) holder_channel_pubkeys: ChannelPublicKeys,
 	/// Key derivation parameters.
 	channel_keys_id: [u8; 32],
 	/// A source of random bytes.
@@ -1180,7 +1184,6 @@ impl PartialEq for InMemorySigner {
 			&& self.delayed_payment_base_key == other.delayed_payment_base_key
 			&& self.htlc_base_key == other.htlc_base_key
 			&& self.commitment_seed == other.commitment_seed
-			&& self.holder_channel_pubkeys == other.holder_channel_pubkeys
 			&& self.channel_keys_id == other.channel_keys_id
 	}
 }
@@ -1195,7 +1198,6 @@ impl Clone for InMemorySigner {
 			delayed_payment_base_key: self.delayed_payment_base_key.clone(),
 			htlc_base_key: self.htlc_base_key.clone(),
 			commitment_seed: self.commitment_seed.clone(),
-			holder_channel_pubkeys: self.holder_channel_pubkeys.clone(),
 			channel_keys_id: self.channel_keys_id,
 			entropy_source: RandomBytes::new(self.get_secure_random_bytes()),
 		}
@@ -1204,21 +1206,11 @@ impl Clone for InMemorySigner {
 
 impl InMemorySigner {
 	#[cfg(any(feature = "_test_utils", test))]
-	pub fn new<C: Signing>(
-		secp_ctx: &Secp256k1<C>, funding_key: SecretKey, revocation_base_key: SecretKey,
-		payment_key_v1: SecretKey, payment_key_v2: SecretKey, delayed_payment_base_key: SecretKey,
-		htlc_base_key: SecretKey, commitment_seed: [u8; 32], channel_keys_id: [u8; 32],
-		rand_bytes_unique_start: [u8; 32],
+	pub fn new(
+		funding_key: SecretKey, revocation_base_key: SecretKey, payment_key_v1: SecretKey,
+		payment_key_v2: SecretKey, delayed_payment_base_key: SecretKey, htlc_base_key: SecretKey,
+		commitment_seed: [u8; 32], channel_keys_id: [u8; 32], rand_bytes_unique_start: [u8; 32],
 	) -> InMemorySigner {
-		// TODO: Make the key used dynamic
-		let holder_channel_pubkeys = InMemorySigner::make_holder_keys(
-			secp_ctx,
-			&funding_key,
-			&revocation_base_key,
-			&payment_key_v1,
-			&delayed_payment_base_key,
-			&htlc_base_key,
-		);
 		InMemorySigner {
 			funding_key: sealed::MaybeTweakedSecretKey::from(funding_key),
 			revocation_base_key,
@@ -1227,28 +1219,17 @@ impl InMemorySigner {
 			delayed_payment_base_key,
 			htlc_base_key,
 			commitment_seed,
-			holder_channel_pubkeys,
 			channel_keys_id,
 			entropy_source: RandomBytes::new(rand_bytes_unique_start),
 		}
 	}
 
 	#[cfg(not(any(feature = "_test_utils", test)))]
-	fn new<C: Signing>(
-		secp_ctx: &Secp256k1<C>, funding_key: SecretKey, revocation_base_key: SecretKey,
-		payment_key_v1: SecretKey, payment_key_v2: SecretKey, delayed_payment_base_key: SecretKey,
-		htlc_base_key: SecretKey, commitment_seed: [u8; 32], channel_keys_id: [u8; 32],
-		rand_bytes_unique_start: [u8; 32],
+	fn new(
+		funding_key: SecretKey, revocation_base_key: SecretKey, payment_key_v1: SecretKey,
+		payment_key_v2: SecretKey, delayed_payment_base_key: SecretKey, htlc_base_key: SecretKey,
+		commitment_seed: [u8; 32], channel_keys_id: [u8; 32], rand_bytes_unique_start: [u8; 32],
 	) -> InMemorySigner {
-		// TODO: Make the key used dynamic
-		let holder_channel_pubkeys = InMemorySigner::make_holder_keys(
-			secp_ctx,
-			&funding_key,
-			&revocation_base_key,
-			&payment_key_v1,
-			&delayed_payment_base_key,
-			&htlc_base_key,
-		);
 		InMemorySigner {
 			funding_key: sealed::MaybeTweakedSecretKey::from(funding_key),
 			revocation_base_key,
@@ -1257,7 +1238,6 @@ impl InMemorySigner {
 			delayed_payment_base_key,
 			htlc_base_key,
 			commitment_seed,
-			holder_channel_pubkeys,
 			channel_keys_id,
 			entropy_source: RandomBytes::new(rand_bytes_unique_start),
 		}
@@ -1271,22 +1251,6 @@ impl InMemorySigner {
 		self.funding_key.with_tweak(tweak)
 	}
 
-	fn make_holder_keys<C: Signing>(
-		secp_ctx: &Secp256k1<C>, funding_key: &SecretKey, revocation_base_key: &SecretKey,
-		payment_key: &SecretKey, delayed_payment_base_key: &SecretKey, htlc_base_key: &SecretKey,
-	) -> ChannelPublicKeys {
-		let from_secret = |s: &SecretKey| PublicKey::from_secret_key(secp_ctx, s);
-		ChannelPublicKeys {
-			funding_pubkey: from_secret(&funding_key),
-			revocation_basepoint: RevocationBasepoint::from(from_secret(&revocation_base_key)),
-			payment_point: from_secret(&payment_key),
-			delayed_payment_basepoint: DelayedPaymentBasepoint::from(from_secret(
-				&delayed_payment_base_key,
-			)),
-			htlc_basepoint: HtlcBasepoint::from(from_secret(&htlc_base_key)),
-		}
-	}
-
 	/// Sign the single input of `spend_tx` at index `input_idx`, which spends the output described
 	/// by `descriptor`, returning the witness stack for the input.
 	///
@@ -1476,10 +1440,21 @@ impl ChannelSigner for InMemorySigner {
 		Ok(())
 	}
 
-	fn pubkeys(
+	fn new_pubkeys(
 		&self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
 	) -> ChannelPublicKeys {
-		let mut pubkeys = self.holder_channel_pubkeys.clone();
+		let from_secret = |s: &SecretKey| PublicKey::from_secret_key(secp_ctx, s);
+		let mut pubkeys = ChannelPublicKeys {
+			funding_pubkey: from_secret(&self.funding_key.0),
+			revocation_basepoint: RevocationBasepoint::from(from_secret(&self.revocation_base_key)),
+			// TODO: Make the payment_key used dynamic
+			payment_point: from_secret(&self.payment_key_v1),
+			delayed_payment_basepoint: DelayedPaymentBasepoint::from(from_secret(
+				&self.delayed_payment_base_key,
+			)),
+			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);
@@ -2135,7 +2110,6 @@ impl KeysManager {
 			u64::from_le_bytes(commitment_seed[..8].try_into().expect("8 bytes"));
 
 		InMemorySigner::new(
-			&self.secp_ctx,
 			funding_key,
 			revocation_base_key,
 			payment_key_v1,