Split KeysInterface::get_channel_signer into two

`get_channel_signer` previously had two different responsibilites:
generating unique `channel_keys_id` and using said ID to derive channel
keys. We decide to split it into two methods `generate_channel_keys_id`
and `derive_channel_signer`, such that we can use the latter to fulfill
our goal of re-deriving signers instead of persisting them. There's no
point in storing data that can be easily re-derived.

Author: wpaulino

fuzz/src/chanmon_consistency.rs

@@ -193,9 +193,14 @@ impl KeysInterface for KeyProvider {
 		ShutdownScript::new_p2wpkh(&pubkey_hash)
 	}
 
-	fn get_channel_signer(&self, _inbound: bool, channel_value_satoshis: u64) -> EnforcingSigner {
+	fn generate_channel_keys_id(&self, inbound: bool, channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] {
+		let id = self.rand_bytes_id.fetch_add(1, atomic::Ordering::Relaxed) as u8;
+		[id; 32]
+	}
+
+	fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> Self::Signer {
 		let secp_ctx = Secp256k1::signing_only();
-		let id = self.rand_bytes_id.fetch_add(1, atomic::Ordering::Relaxed);
+		let id = channel_keys_id[0];
 		let keys = InMemorySigner::new(
 			&secp_ctx,
 			self.get_node_secret(Recipient::Node).unwrap(),
@@ -204,9 +209,9 @@ impl KeysInterface for KeyProvider {
 			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, 6, self.node_id]).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, 7, self.node_id]).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, 8, self.node_id]).unwrap(),
-			[id as u8, 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, 9, self.node_id],
+			[id, 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, 9, self.node_id],
 			channel_value_satoshis,
-			[0; 32],
+			channel_keys_id,
 		);
 		let revoked_commitment = self.make_enforcement_state_cell(keys.commitment_seed);
 		EnforcingSigner::new_with_revoked(keys, revoked_commitment, false)

fuzz/src/full_stack.rs

@@ -297,9 +297,15 @@ impl KeysInterface for KeyProvider {
 		ShutdownScript::new_p2wpkh(&pubkey_hash)
 	}
 
-	fn get_channel_signer(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingSigner {
+	fn generate_channel_keys_id(&self, inbound: bool, channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] {
 		let ctr = self.counter.fetch_add(1, Ordering::Relaxed) as u8;
+		[ctr; 32]
+	}
+
+	fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> Self::Signer {
 		let secp_ctx = Secp256k1::signing_only();
+		let ctr = channel_keys_id[0];
+		let inbound = ctr % 2 == 0;
 		EnforcingSigner::new(if inbound {
 			InMemorySigner::new(
 				&secp_ctx,
@@ -311,7 +317,7 @@ impl KeysInterface for KeyProvider {
 				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, ctr]).unwrap(),
 				[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, 6, ctr],
 				channel_value_satoshis,
-				[0; 32],
+				channel_keys_id,
 			)
 		} else {
 			InMemorySigner::new(
@@ -324,7 +330,7 @@ impl KeysInterface for KeyProvider {
 				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, 11, ctr]).unwrap(),
 				[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, 12, ctr],
 				channel_value_satoshis,
-				[0; 32],
+				channel_keys_id,
 			)
 		})
 	}

fuzz/src/onion_message.rs

@@ -111,7 +111,9 @@ impl KeysInterface for KeyProvider {
 
 	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { unreachable!() }
 
-	fn get_channel_signer(&self, _inbound: bool, _channel_value_satoshis: u64) -> EnforcingSigner {
+	fn generate_channel_keys_id(&self, inbound: bool, channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] { unreachable!() }
+
+	fn derive_channel_signer(&self, _channel_value_satoshis: u64, _channel_keys_id: [u8; 32]) -> Self::Signer {
 		unreachable!()
 	}
 

lightning/src/chain/keysinterface.rs

@@ -402,7 +402,7 @@ pub enum Recipient {
 
 /// A trait to describe an object which can get user secrets and key material.
 pub trait KeysInterface {
-	/// A type which implements Sign which will be returned by get_channel_signer.
+	/// A type which implements Sign which will be returned by derive_channel_signer.
 	type Signer : Sign;
 
 	/// Get node secret key based on the provided [`Recipient`].
@@ -445,11 +445,20 @@ pub trait KeysInterface {
 	/// This method should return a different value each time it is called, to avoid linking
 	/// on-chain funds across channels as controlled to the same user.
 	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript;
-	/// Get a new set of Sign for per-channel secrets. These MUST be unique even if you
-	/// restarted with some stale data!
+	/// Generates a unique `channel_keys_id` that can be used to obtain a `Signer` through
+	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
+	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
+	/// `channel_keys_id`.
 	///
 	/// This method must return a different value each time it is called.
-	fn get_channel_signer(&self, inbound: bool, channel_value_satoshis: u64) -> Self::Signer;
+	fn generate_channel_keys_id(&self, inbound: bool, channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32];
+	/// Derives the private key material backing a `Signer`.
+	///
+	/// To derive a new `Signer`, a fresh `channel_keys_id` should be obtained through
+	/// [`KeysInterface::generate_channel_keys_id`]. Otherwise, an existing `Signer` can be
+	/// re-derived from its `channel_keys_id`, which can be obtained through its trait method
+	/// [`BaseSign::channel_keys_id`].
+	fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> Self::Signer;
 	/// Gets a unique, cryptographically-secure, random 32 byte value. This is used for encrypting
 	/// onion packets and for temporary channel IDs. There is no requirement that these be
 	/// persisted anywhere, though they must be unique across restarts.
@@ -987,13 +996,8 @@ impl KeysManager {
 		}
 	}
 	/// Derive an old Sign containing per-channel secrets based on a key derivation parameters.
-	///
-	/// Key derivation parameters are accessible through a per-channel secrets
-	/// Sign::channel_keys_id and is provided inside DynamicOuputP2WSH in case of
-	/// onchain output detection for which a corresponding delayed_payment_key must be derived.
 	pub fn derive_channel_keys(&self, channel_value_satoshis: u64, params: &[u8; 32]) -> InMemorySigner {
 		let chan_id = byte_utils::slice_to_be64(&params[0..8]);
-		assert!(chan_id <= core::u32::MAX as u64); // Otherwise the params field wasn't created by us
 		let mut unique_start = Sha256::engine();
 		unique_start.input(params);
 		unique_start.input(&self.seed);
@@ -1209,14 +1213,19 @@ impl KeysInterface for KeysManager {
 		ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.clone())
 	}
 
-	fn get_channel_signer(&self, _inbound: bool, channel_value_satoshis: u64) -> Self::Signer {
-		let child_ix = self.channel_child_index.fetch_add(1, Ordering::AcqRel);
-		assert!(child_ix <= core::u32::MAX as usize);
+	fn generate_channel_keys_id(&self, _inbound: bool, _channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] {
+		let child_idx = self.channel_child_index.fetch_add(1, Ordering::AcqRel);
+		assert!(child_idx <= core::u32::MAX as usize);
 		let mut id = [0; 32];
-		id[0..8].copy_from_slice(&byte_utils::be64_to_array(child_ix as u64));
-		id[8..16].copy_from_slice(&byte_utils::be64_to_array(self.starting_time_nanos as u64));
-		id[16..24].copy_from_slice(&byte_utils::be64_to_array(self.starting_time_secs));
-		self.derive_channel_keys(channel_value_satoshis, &id)
+		id[0..4].copy_from_slice(&byte_utils::be32_to_array(child_idx as u32));
+		id[4..8].copy_from_slice(&byte_utils::be32_to_array(self.starting_time_nanos));
+		id[8..16].copy_from_slice(&byte_utils::be64_to_array(self.starting_time_secs));
+		id[16..32].copy_from_slice(&byte_utils::be128_to_array(user_channel_id));
+		id
+	}
+
+	fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> Self::Signer {
+		self.derive_channel_keys(channel_value_satoshis, &channel_keys_id)
 	}
 
 	fn get_secure_random_bytes(&self) -> [u8; 32] {
@@ -1309,10 +1318,14 @@ impl KeysInterface for PhantomKeysManager {
 		self.inner.get_shutdown_scriptpubkey()
 	}
 
-	fn get_channel_signer(&self, inbound: bool, channel_value_satoshis: u64) -> Self::Signer {
-		self.inner.get_channel_signer(inbound, channel_value_satoshis)
+	fn generate_channel_keys_id(&self, inbound: bool, channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] {
+		self.inner.generate_channel_keys_id(inbound, channel_value_satoshis, user_channel_id)
 	}
 
+	fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> Self::Signer {
+		self.inner.derive_channel_signer(channel_value_satoshis, channel_keys_id)
+    }
+
 	fn get_secure_random_bytes(&self) -> [u8; 32] {
 		self.inner.get_secure_random_bytes()
 	}

lightning/src/ln/chan_utils.rs

@@ -1617,8 +1617,8 @@ mod tests {
 		let seed = [42; 32];
 		let network = Network::Testnet;
 		let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
-		let signer = keys_provider.get_channel_signer(false, 3000);
-		let counterparty_signer = keys_provider.get_channel_signer(false, 3000);
+		let signer = keys_provider.derive_channel_signer(3000, keys_provider.generate_channel_keys_id(false, 1_000_000, 0));
+		let counterparty_signer = keys_provider.derive_channel_signer(3000, keys_provider.generate_channel_keys_id(true, 1_000_000, 1));
 		let delayed_payment_base = &signer.pubkeys().delayed_payment_basepoint;
 		let per_commitment_secret = SecretKey::from_slice(&hex::decode("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap();
 		let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
@@ -1714,9 +1714,9 @@ mod tests {
 		assert_eq!(tx.built.transaction.output[0].script_pubkey, get_htlc_redeemscript(&received_htlc, false, &keys).to_v0_p2wsh());
 		assert_eq!(tx.built.transaction.output[1].script_pubkey, get_htlc_redeemscript(&offered_htlc, false, &keys).to_v0_p2wsh());
 		assert_eq!(get_htlc_redeemscript(&received_htlc, false, &keys).to_v0_p2wsh().to_hex(),
-				   "002085cf52e41ba7c099a39df504e7b61f6de122971ceb53b06731876eaeb85e8dc5");
+				   "0020e43a7c068553003fe68fcae424fb7b28ec5ce48cd8b6744b3945631389bad2fb");
 		assert_eq!(get_htlc_redeemscript(&offered_htlc, false, &keys).to_v0_p2wsh().to_hex(),
-				   "002049f0736bb335c61a04d2623a24df878a7592a3c51fa7258d41b2c85318265e73");
+				   "0020215d61bba56b19e9eadb6107f5a85d7f99c40f65992443f69229c290165bc00d");
 
 		// Generate broadcaster output and received and offered HTLC outputs,  with anchors
 		channel_parameters.opt_anchors = Some(());
@@ -1733,9 +1733,9 @@ mod tests {
 		assert_eq!(tx.built.transaction.output[2].script_pubkey, get_htlc_redeemscript(&received_htlc, true, &keys).to_v0_p2wsh());
 		assert_eq!(tx.built.transaction.output[3].script_pubkey, get_htlc_redeemscript(&offered_htlc, true, &keys).to_v0_p2wsh());
 		assert_eq!(get_htlc_redeemscript(&received_htlc, true, &keys).to_v0_p2wsh().to_hex(),
-				   "002067114123af3f95405bae4fd930fc95de03e3c86baaee8b2dd29b43dd26cf613c");
+				   "0020b70d0649c72b38756885c7a30908d912a7898dd5d79457a7280b8e9a20f3f2bc");
 		assert_eq!(get_htlc_redeemscript(&offered_htlc, true, &keys).to_v0_p2wsh().to_hex(),
-				   "0020a06e3b0d4fcf704f2b9c41e16a70099e39989466c3142b8573a1154542f28f57");
+				   "002087a3faeb1950a469c0e2db4a79b093a41b9526e5a6fc6ef5cb949bde3be379c7");
 	}
 
 	#[test]

lightning/src/ln/channel.rs

@@ -909,7 +909,8 @@ impl<Signer: Sign> Channel<Signer> {
 		let opt_anchors = false; // TODO - should be based on features
 
 		let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
-		let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+		let channel_keys_id = keys_provider.generate_channel_keys_id(false, channel_value_satoshis, user_id);
+		let holder_signer = keys_provider.derive_channel_signer(channel_value_satoshis, channel_keys_id);
 		let pubkeys = holder_signer.pubkeys().clone();
 
 		if !their_features.supports_wumbo() && channel_value_satoshis > MAX_FUNDING_SATOSHIS_NO_WUMBO {
@@ -1153,7 +1154,8 @@ impl<Signer: Sign> Channel<Signer> {
 			return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned()));
 		}
 
-		let holder_signer = keys_provider.get_channel_signer(true, msg.funding_satoshis);
+		let channel_keys_id = keys_provider.generate_channel_keys_id(true, msg.funding_satoshis, user_id);
+		let holder_signer = keys_provider.derive_channel_signer(msg.funding_satoshis, channel_keys_id);
 		let pubkeys = holder_signer.pubkeys().clone();
 		let counterparty_pubkeys = ChannelPublicKeys {
 			funding_pubkey: msg.funding_pubkey,
@@ -6725,7 +6727,7 @@ mod tests {
 	use crate::ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
 	use crate::chain::BestBlock;
 	use crate::chain::chaininterface::{FeeEstimator, LowerBoundedFeeEstimator, ConfirmationTarget};
-	use crate::chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface};
+	use crate::chain::keysinterface::{BaseSign, InMemorySigner, Recipient, KeyMaterial, KeysInterface};
 	use crate::chain::transaction::OutPoint;
 	use crate::util::config::UserConfig;
 	use crate::util::enforcing_trait_impls::EnforcingSigner;
@@ -6793,7 +6795,10 @@ mod tests {
 			ShutdownScript::new_p2wpkh_from_pubkey(PublicKey::from_secret_key(&secp_ctx, &channel_close_key))
 		}
 
-		fn get_channel_signer(&self, _inbound: bool, _channel_value_satoshis: u64) -> InMemorySigner {
+		fn generate_channel_keys_id(&self, _inbound: bool, _channel_value_satoshis: u64, _user_channel_id: u128) -> [u8; 32] {
+			self.signer.channel_keys_id()
+		}
+		fn derive_channel_signer(&self, _channel_value_satoshis: u64, _channel_keys_id: [u8; 32]) -> Self::Signer {
 			self.signer.clone()
 		}
 		fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }

lightning/src/ln/functional_tests.rs

@@ -4785,7 +4785,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 	assert_eq!(htlc_success_txn[2], commitment_txn[0]);
 	assert_eq!(htlc_success_txn[3], htlc_success_txn[0]);
 	assert_eq!(htlc_success_txn[4], htlc_success_txn[1]);
-	assert_ne!(htlc_success_txn[0].input[0].previous_output, htlc_timeout_tx.input[0].previous_output);
+	assert_ne!(htlc_success_txn[1].input[0].previous_output, htlc_timeout_tx.input[0].previous_output);
 
 	mine_transaction(&nodes[1], &htlc_timeout_tx);
 	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
@@ -4808,7 +4808,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 	// Solve 2nd HTLC by broadcasting on B's chain HTLC-Success Tx from C
 	// Note that the fee paid is effectively double as the HTLC value (including the nodes[1] fee
 	// and nodes[2] fee) is rounded down and then claimed in full.
-	mine_transaction(&nodes[1], &htlc_success_txn[0]);
+	mine_transaction(&nodes[1], &htlc_success_txn[1]);
 	expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(196*2), true, true);
 	let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	assert!(updates.update_add_htlcs.is_empty());

lightning/src/util/byte_utils.rs

@@ -69,11 +69,32 @@ pub fn be64_to_array(u: u64) -> [u8; 8] {
 	v[7] = ((u >> 8*0) & 0xff) as u8;
 	v
 }
+#[inline]
+pub fn be128_to_array(u: u128) -> [u8; 16] {
+	let mut v = [0; 16];
+	v[0] = ((u >> 8*15) & 0xff) as u8;
+	v[1] = ((u >> 8*14) & 0xff) as u8;
+	v[2] = ((u >> 8*13) & 0xff) as u8;
+	v[3] = ((u >> 8*12) & 0xff) as u8;
+	v[4] = ((u >> 8*11) & 0xff) as u8;
+	v[5] = ((u >> 8*10) & 0xff) as u8;
+	v[6] = ((u >> 8*9) & 0xff) as u8;
+	v[7] = ((u >> 8*8) & 0xff) as u8;
+	v[8] = ((u >> 8*7) & 0xff) as u8;
+	v[9] = ((u >> 8*6) & 0xff) as u8;
+	v[10] = ((u >> 8*5) & 0xff) as u8;
+	v[11] = ((u >> 8*4) & 0xff) as u8;
+	v[12] = ((u >> 8*3) & 0xff) as u8;
+	v[13] = ((u >> 8*2) & 0xff) as u8;
+	v[14] = ((u >> 8*1) & 0xff) as u8;
+	v[15] = ((u >> 8*0) & 0xff) as u8;
+	v
+}
 
 #[cfg(test)]
 mod tests {
 	use super::*;
-	
+
 	#[test]
 	fn test_all() {
 		assert_eq!(slice_to_be48(&[0xde, 0xad, 0xbe, 0xef, 0x1b, 0xad]), 0xdeadbeef1bad);
@@ -82,5 +103,9 @@ mod tests {
 		assert_eq!(be32_to_array(0xdeadbeef), [0xde, 0xad, 0xbe, 0xef]);
 		assert_eq!(be48_to_array(0xdeadbeef1bad), [0xde, 0xad, 0xbe, 0xef, 0x1b, 0xad]);
 		assert_eq!(be64_to_array(0xdeadbeef1bad1dea), [0xde, 0xad, 0xbe, 0xef, 0x1b, 0xad, 0x1d, 0xea]);
+		assert_eq!(
+			be128_to_array(0xdeadbeef1bad1deafeedfacefeedfacf),
+			[0xde, 0xad, 0xbe, 0xef, 0x1b, 0xad, 0x1d, 0xea, 0xfe, 0xed, 0xfa, 0xce, 0xfe, 0xed, 0xfa, 0xcf]
+		);
 	}
 }

lightning/src/util/test_utils.rs

@@ -80,7 +80,8 @@ impl keysinterface::KeysInterface for OnlyReadsKeysInterface {
 	fn get_inbound_payment_key_material(&self) -> KeyMaterial { unreachable!(); }
 	fn get_destination_script(&self) -> Script { unreachable!(); }
 	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { unreachable!(); }
-	fn get_channel_signer(&self, _inbound: bool, _channel_value_satoshis: u64) -> EnforcingSigner { unreachable!(); }
+	fn generate_channel_keys_id(&self, _inbound: bool, _channel_value_satoshis: u64, _user_channel_id: u128) -> [u8; 32] { unreachable!(); }
+	fn derive_channel_signer(&self, _channel_value_satoshis: u64, _channel_keys_id: [u8; 32]) -> Self::Signer { unreachable!(); }
 	fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }
 
 	fn read_chan_signer(&self, mut reader: &[u8]) -> Result<Self::Signer, msgs::DecodeError> {
@@ -629,8 +630,12 @@ impl keysinterface::KeysInterface for TestKeysInterface {
 		}
 	}
 
-	fn get_channel_signer(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingSigner {
-		let keys = self.backing.get_channel_signer(inbound, channel_value_satoshis);
+	fn generate_channel_keys_id(&self, inbound: bool, channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] {
+		self.backing.generate_channel_keys_id(inbound, channel_value_satoshis, user_channel_id)
+	}
+
+	fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> EnforcingSigner {
+		let keys = self.backing.derive_channel_signer(channel_value_satoshis, channel_keys_id);
 		let state = self.make_enforcement_state_cell(keys.commitment_seed);
 		EnforcingSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check)
 	}