Move node_id signing of ChannelAnnouncement into Signer

This removes one more place where we directly access the node_id
secret key in `ChannelManager`, slowly marching towards allowing
the node_id secret key to be offline in the signer.

More importantly, it allows more ChannelAnnouncement logic to move
into the `Channel` without having to pass the node secret key
around, avoiding the announcement logic being split across two
files.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -184,6 +184,7 @@ impl KeysInterface for KeyProvider {
 		let id = self.rand_bytes_id.fetch_add(1, atomic::Ordering::Relaxed);
 		let keys = InMemorySigner::new(
 			&secp_ctx,
+			self.get_node_secret(),
 			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_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, 5, 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, 6, self.node_id]).unwrap(),
@@ -207,7 +208,7 @@ impl KeysInterface for KeyProvider {
 	fn read_chan_signer(&self, buffer: &[u8]) -> Result<Self::Signer, DecodeError> {
 		let mut reader = std::io::Cursor::new(buffer);
 
-		let inner: InMemorySigner = Readable::read(&mut reader)?;
+		let inner: InMemorySigner = ReadableArgs::read(&mut reader, self.get_node_secret())?;
 		let state = self.make_enforcement_state_cell(inner.commitment_seed);
 
 		Ok(EnforcingSigner {

fuzz/src/full_stack.rs

@@ -45,7 +45,7 @@ use lightning::util::errors::APIError;
 use lightning::util::events::Event;
 use lightning::util::enforcing_trait_impls::{EnforcingSigner, EnforcementState};
 use lightning::util::logger::Logger;
-use lightning::util::ser::Readable;
+use lightning::util::ser::ReadableArgs;
 
 use utils::test_logger;
 use utils::test_persister::TestPersister;
@@ -286,6 +286,7 @@ impl KeysInterface for KeyProvider {
 		EnforcingSigner::new(if inbound {
 			InMemorySigner::new(
 				&secp_ctx,
+				self.node_secret.clone(),
 				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, 1, ctr]).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, 2, ctr]).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, 3, ctr]).unwrap(),
@@ -298,6 +299,7 @@ impl KeysInterface for KeyProvider {
 		} else {
 			InMemorySigner::new(
 				&secp_ctx,
+				self.node_secret.clone(),
 				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, ctr]).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, ctr]).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, 9, ctr]).unwrap(),
@@ -317,7 +319,7 @@ impl KeysInterface for KeyProvider {
 	}
 
 	fn read_chan_signer(&self, mut data: &[u8]) -> Result<EnforcingSigner, DecodeError> {
-		let inner: InMemorySigner = Readable::read(&mut data)?;
+		let inner: InMemorySigner = ReadableArgs::read(&mut data, self.node_secret.clone())?;
 		let state = Arc::new(Mutex::new(EnforcementState::new()));
 
 		Ok(EnforcingSigner::new_with_revoked(

lightning/src/chain/channelmonitor.rs

@@ -3358,6 +3358,7 @@ mod tests {
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
 			SecretKey::from_slice(&[41; 32]).unwrap(),
+			SecretKey::from_slice(&[41; 32]).unwrap(),
 			[41; 32],
 			0,
 			[0; 32]

lightning/src/chain/keysinterface.rs

@@ -30,7 +30,7 @@ use bitcoin::secp256k1::recovery::RecoverableSignature;
 use bitcoin::secp256k1;
 
 use util::{byte_utils, transaction_utils};
-use util::ser::{Writeable, Writer, Readable};
+use util::ser::{Writeable, Writer, Readable, ReadableArgs};
 
 use chain::transaction::OutPoint;
 use ln::chan_utils;
@@ -324,13 +324,17 @@ pub trait BaseSign {
 	/// chosen to forgo their output as dust.
 	fn sign_closing_transaction(&self, closing_tx: &ClosingTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>;
 
-	/// Signs a channel announcement message with our funding key, proving it comes from one
-	/// of the channel participants.
+	/// Signs a channel announcement message with our funding key and our node secret key (aka
+	/// node_id or network_key), proving it comes from one of the channel participants.
+	///
+	/// The first returned signature should be from our node secret key, the second from our
+	/// funding key.
 	///
 	/// Note that if this fails or is rejected, the channel will not be publicly announced and
 	/// our counterparty may (though likely will not) close the channel on us for violating the
 	/// protocol.
-	fn sign_channel_announcement(&self, msg: &UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>;
+	fn sign_channel_announcement(&self, msg: &UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>)
+		-> Result<(Signature, Signature), ()>;
 
 	/// Set the counterparty static channel data, including basepoints,
 	/// counterparty_selected/holder_selected_contest_delay and funding outpoint.
@@ -419,6 +423,8 @@ pub struct InMemorySigner {
 	pub commitment_seed: [u8; 32],
 	/// Holder public keys and basepoints
 	pub(crate) holder_channel_pubkeys: ChannelPublicKeys,
+	/// Private key of our node secret, used for signing channel announcements
+	node_secret: SecretKey,
 	/// Counterparty public keys and counterparty/holder selected_contest_delay, populated on channel acceptance
 	channel_parameters: Option<ChannelTransactionParameters>,
 	/// The total value of this channel
@@ -431,6 +437,7 @@ impl InMemorySigner {
 	/// Create a new InMemorySigner
 	pub fn new<C: Signing>(
 		secp_ctx: &Secp256k1<C>,
+		node_secret: SecretKey,
 		funding_key: SecretKey,
 		revocation_base_key: SecretKey,
 		payment_key: SecretKey,
@@ -450,6 +457,7 @@ impl InMemorySigner {
 			delayed_payment_base_key,
 			htlc_base_key,
 			commitment_seed,
+			node_secret,
 			channel_value_satoshis,
 			holder_channel_pubkeys,
 			channel_parameters: None,
@@ -677,9 +685,10 @@ impl BaseSign for InMemorySigner {
 		Ok(closing_tx.trust().sign(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
 	}
 
-	fn sign_channel_announcement(&self, msg: &UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
+	fn sign_channel_announcement(&self, msg: &UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>)
+	-> Result<(Signature, Signature), ()> {
 		let msghash = hash_to_message!(&Sha256dHash::hash(&msg.encode()[..])[..]);
-		Ok(secp_ctx.sign(&msghash, &self.funding_key))
+		Ok((secp_ctx.sign(&msghash, &self.node_secret), secp_ctx.sign(&msghash, &self.funding_key)))
 	}
 
 	fn ready_channel(&mut self, channel_parameters: &ChannelTransactionParameters) {
@@ -714,8 +723,8 @@ impl Writeable for InMemorySigner {
 	}
 }
 
-impl Readable for InMemorySigner {
-	fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+impl ReadableArgs<SecretKey> for InMemorySigner {
+	fn read<R: io::Read>(reader: &mut R, node_secret: SecretKey) -> Result<Self, DecodeError> {
 		let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 
 		let funding_key = Readable::read(reader)?;
@@ -741,6 +750,7 @@ impl Readable for InMemorySigner {
 			payment_key,
 			delayed_payment_base_key,
 			htlc_base_key,
+			node_secret,
 			commitment_seed,
 			channel_value_satoshis,
 			holder_channel_pubkeys,
@@ -889,6 +899,7 @@ impl KeysManager {
 
 		InMemorySigner::new(
 			&self.secp_ctx,
+			self.node_secret,
 			funding_key,
 			revocation_base_key,
 			payment_key,
@@ -1062,7 +1073,7 @@ impl KeysInterface for KeysManager {
 	}
 
 	fn read_chan_signer(&self, reader: &[u8]) -> Result<Self::Signer, DecodeError> {
-		InMemorySigner::read(&mut io::Cursor::new(reader))
+		InMemorySigner::read(&mut io::Cursor::new(reader), self.get_node_secret())
 	}
 
 	fn sign_invoice(&self, invoice_preimage: Vec<u8>) -> Result<RecoverableSignature, ()> {

lightning/src/ln/channel.rs

@@ -4438,7 +4438,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// https://github.com/lightningnetwork/lightning-rfc/issues/468
 	///
 	/// This will only return ChannelError::Ignore upon failure.
-	pub fn get_channel_announcement(&self, node_id: PublicKey, chain_hash: BlockHash) -> Result<(msgs::UnsignedChannelAnnouncement, Signature), ChannelError> {
+	fn get_channel_announcement(&self, node_id: PublicKey, chain_hash: BlockHash) -> Result<msgs::UnsignedChannelAnnouncement, ChannelError> {
 		if !self.config.announced_channel {
 			return Err(ChannelError::Ignore("Channel is not available for public announcements".to_owned()));
 		}
@@ -4462,19 +4462,30 @@ impl<Signer: Sign> Channel<Signer> {
 			excess_data: Vec::new(),
 		};
 
-		let sig = self.holder_signer.sign_channel_announcement(&msg, &self.secp_ctx)
+		Ok(msg)
+	}
+
+	pub fn get_announcement_sigs(&self, node_pk: PublicKey, genesis_block_hash: BlockHash) -> Result<msgs::AnnouncementSignatures, ChannelError> {
+		let announcement = self.get_channel_announcement(node_pk, genesis_block_hash)?;
+		let (our_node_sig, our_bitcoin_sig) = self.holder_signer.sign_channel_announcement(&announcement, &self.secp_ctx)
 			.map_err(|_| ChannelError::Ignore("Signer rejected channel_announcement".to_owned()))?;
 
-		Ok((msg, sig))
+		Ok(msgs::AnnouncementSignatures {
+			channel_id: self.channel_id(),
+			short_channel_id: self.get_short_channel_id().unwrap(),
+			node_signature: our_node_sig,
+			bitcoin_signature: our_bitcoin_sig,
+		})
 	}
 
 	/// Signs the given channel announcement, returning a ChannelError::Ignore if no keys are
 	/// available.
-	fn sign_channel_announcement(&self, our_node_secret: &SecretKey, our_node_id: PublicKey, msghash: secp256k1::Message, announcement: msgs::UnsignedChannelAnnouncement, our_bitcoin_sig: Signature) -> Result<msgs::ChannelAnnouncement, ChannelError> {
+	fn sign_channel_announcement(&self, our_node_id: PublicKey, announcement: msgs::UnsignedChannelAnnouncement) -> Result<msgs::ChannelAnnouncement, ChannelError> {
 		if let Some((their_node_sig, their_bitcoin_sig)) = self.announcement_sigs {
 			let were_node_one = announcement.node_id_1 == our_node_id;
 
-			let our_node_sig = self.secp_ctx.sign(&msghash, our_node_secret);
+			let (our_node_sig, our_bitcoin_sig) = self.holder_signer.sign_channel_announcement(&announcement, &self.secp_ctx)
+				.map_err(|_| ChannelError::Ignore("Signer rejected channel_announcement".to_owned()))?;
 			Ok(msgs::ChannelAnnouncement {
 				node_signature_1: if were_node_one { our_node_sig } else { their_node_sig },
 				node_signature_2: if were_node_one { their_node_sig } else { our_node_sig },
@@ -4490,8 +4501,8 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Processes an incoming announcement_signatures message, providing a fully-signed
 	/// channel_announcement message which we can broadcast and storing our counterparty's
 	/// signatures for later reconstruction/rebroadcast of the channel_announcement.
-	pub fn announcement_signatures(&mut self, our_node_secret: &SecretKey, our_node_id: PublicKey, chain_hash: BlockHash, msg: &msgs::AnnouncementSignatures) -> Result<msgs::ChannelAnnouncement, ChannelError> {
-		let (announcement, our_bitcoin_sig) = self.get_channel_announcement(our_node_id.clone(), chain_hash)?;
+	pub fn announcement_signatures(&mut self, our_node_id: PublicKey, chain_hash: BlockHash, msg: &msgs::AnnouncementSignatures) -> Result<msgs::ChannelAnnouncement, ChannelError> {
+		let announcement = self.get_channel_announcement(our_node_id.clone(), chain_hash)?;
 
 		let msghash = hash_to_message!(&Sha256d::hash(&announcement.encode()[..])[..]);
 
@@ -4508,18 +4519,17 @@ impl<Signer: Sign> Channel<Signer> {
 
 		self.announcement_sigs = Some((msg.node_signature, msg.bitcoin_signature));
 
-		self.sign_channel_announcement(our_node_secret, our_node_id, msghash, announcement, our_bitcoin_sig)
+		self.sign_channel_announcement(our_node_id, announcement)
 	}
 
 	/// Gets a signed channel_announcement for this channel, if we previously received an
 	/// announcement_signatures from our counterparty.
-	pub fn get_signed_channel_announcement(&self, our_node_secret: &SecretKey, our_node_id: PublicKey, chain_hash: BlockHash) -> Option<msgs::ChannelAnnouncement> {
-		let (announcement, our_bitcoin_sig) = match self.get_channel_announcement(our_node_id.clone(), chain_hash) {
+	pub fn get_signed_channel_announcement(&self, our_node_id: PublicKey, chain_hash: BlockHash) -> Option<msgs::ChannelAnnouncement> {
+		let announcement = match self.get_channel_announcement(our_node_id.clone(), chain_hash) {
 			Ok(res) => res,
 			Err(_) => return None,
 		};
-		let msghash = hash_to_message!(&Sha256d::hash(&announcement.encode()[..])[..]);
-		match self.sign_channel_announcement(our_node_secret, our_node_id, msghash, announcement, our_bitcoin_sig) {
+		match self.sign_channel_announcement(our_node_id, announcement) {
 			Ok(res) => Some(res),
 			Err(_) => None,
 		}
@@ -6034,6 +6044,7 @@ mod tests {
 
 		let mut signer = InMemorySigner::new(
 			&secp_ctx,
+			SecretKey::from_slice(&hex::decode("4242424242424242424242424242424242424242424242424242424242424242").unwrap()[..]).unwrap(),
 			SecretKey::from_slice(&hex::decode("30ff4956bbdd3222d44cc5e8a1261dab1e07957bdac5ae88fe3261ef321f3749").unwrap()[..]).unwrap(),
 			SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
 			SecretKey::from_slice(&hex::decode("1111111111111111111111111111111111111111111111111111111111111111").unwrap()[..]).unwrap(),

lightning/src/ln/channelmanager.rs

@@ -2487,20 +2487,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			log_trace!(self.logger, "Can't send announcement_signatures for private channel {}", log_bytes!(chan.channel_id()));
 			return None
 		}
-
-		let (announcement, our_bitcoin_sig) = match chan.get_channel_announcement(self.get_our_node_id(), self.genesis_hash.clone()) {
-			Ok(res) => res,
-			Err(_) => return None, // Only in case of state precondition violations eg channel is closing
-		};
-		let msghash = hash_to_message!(&Sha256dHash::hash(&announcement.encode()[..])[..]);
-		let our_node_sig = self.secp_ctx.sign(&msghash, &self.our_network_key);
-
-		Some(msgs::AnnouncementSignatures {
-			channel_id: chan.channel_id(),
-			short_channel_id: chan.get_short_channel_id().unwrap(),
-			node_signature: our_node_sig,
-			bitcoin_signature: our_bitcoin_sig,
-		})
+		chan.get_announcement_sigs(self.get_our_node_id(), self.genesis_hash.clone()).ok()
 	}
 
 	#[allow(dead_code)]
@@ -2560,7 +2547,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 		let mut announced_chans = false;
 		for (_, chan) in channel_state.by_id.iter() {
-			if let Some(msg) = chan.get_signed_channel_announcement(&self.our_network_key, self.get_our_node_id(), self.genesis_hash.clone()) {
+			if let Some(msg) = chan.get_signed_channel_announcement(self.get_our_node_id(), self.genesis_hash.clone()) {
 				channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
 					msg,
 					update_msg: match self.get_channel_update_for_broadcast(chan) {
@@ -4214,7 +4201,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				}
 
 				channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
-					msg: try_chan_entry!(self, chan.get_mut().announcement_signatures(&self.our_network_key, self.get_our_node_id(), self.genesis_hash.clone(), msg), channel_state, chan),
+					msg: try_chan_entry!(self, chan.get_mut().announcement_signatures(self.get_our_node_id(), self.genesis_hash.clone(), msg), channel_state, chan),
 					// Note that announcement_signatures fails if the channel cannot be announced,
 					// so get_channel_update_for_broadcast will never fail by the time we get here.
 					update_msg: self.get_channel_update_for_broadcast(chan.get()).unwrap(),

lightning/src/util/enforcing_trait_impls.rs

@@ -188,7 +188,8 @@ impl BaseSign for EnforcingSigner {
 		Ok(self.inner.sign_closing_transaction(closing_tx, secp_ctx).unwrap())
 	}
 
-	fn sign_channel_announcement(&self, msg: &msgs::UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
+	fn sign_channel_announcement(&self, msg: &msgs::UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>)
+	-> Result<(Signature, Signature), ()> {
 		self.inner.sign_channel_announcement(msg, secp_ctx)
 	}
 

lightning/src/util/test_utils.rs

@@ -76,7 +76,8 @@ impl keysinterface::KeysInterface for OnlyReadsKeysInterface {
 	fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }
 
 	fn read_chan_signer(&self, mut reader: &[u8]) -> Result<Self::Signer, msgs::DecodeError> {
-		let inner: InMemorySigner = Readable::read(&mut reader)?;
+		let dummy_sk = SecretKey::from_slice(&[42; 32]).unwrap();
+		let inner: InMemorySigner = ReadableArgs::read(&mut reader, dummy_sk)?;
 		let state = Arc::new(Mutex::new(EnforcementState::new()));
 
 		Ok(EnforcingSigner::new_with_revoked(
@@ -497,7 +498,7 @@ impl keysinterface::KeysInterface for TestKeysInterface {
 	fn read_chan_signer(&self, buffer: &[u8]) -> Result<Self::Signer, msgs::DecodeError> {
 		let mut reader = io::Cursor::new(buffer);
 
-		let inner: InMemorySigner = Readable::read(&mut reader)?;
+		let inner: InMemorySigner = ReadableArgs::read(&mut reader, self.get_node_secret())?;
 		let state = self.make_enforcement_state_cell(inner.commitment_seed);
 
 		Ok(EnforcingSigner::new_with_revoked(