Merge pull request #1179 from TheBlueMatt/2021-11-fix-announce-sigs-broadcast-time

Disconnect announcement_signatures sending from funding_locked

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -188,6 +188,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(),
@@ -211,7 +212,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;
@@ -293,6 +293,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(),
@@ -305,6 +306,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(),
@@ -324,7 +326,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

@@ -3479,6 +3479,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

@@ -31,7 +31,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, PaymentPreimage};
@@ -346,13 +346,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.
@@ -447,6 +451,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
@@ -459,6 +465,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,
@@ -478,6 +485,7 @@ impl InMemorySigner {
 			delayed_payment_base_key,
 			htlc_base_key,
 			commitment_seed,
+			node_secret,
 			channel_value_satoshis,
 			holder_channel_pubkeys,
 			channel_parameters: None,
@@ -720,9 +728,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) {
@@ -757,8 +766,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)?;
@@ -784,6 +793,7 @@ impl Readable for InMemorySigner {
 			payment_key,
 			delayed_payment_base_key,
 			htlc_base_key,
+			node_secret,
 			commitment_seed,
 			channel_value_satoshis,
 			holder_channel_pubkeys,
@@ -937,6 +947,7 @@ impl KeysManager {
 
 		InMemorySigner::new(
 			&self.secp_ctx,
+			self.node_secret,
 			funding_key,
 			revocation_base_key,
 			payment_key,
@@ -1119,7 +1130,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, hrp_bytes: &[u8], invoice_data: &[u5]) -> Result<RecoverableSignature, ()> {

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -20,6 +20,7 @@ use chain::channelmonitor::ChannelMonitor;
 use chain::transaction::OutPoint;
 use chain::{ChannelMonitorUpdateErr, Listen, Watch};
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure};
+use ln::channel::AnnouncementSigsState;
 use ln::features::InitFeatures;
 use ln::msgs;
 use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler};
@@ -1402,6 +1403,11 @@ fn monitor_failed_no_reestablish_response() {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
+	{
+		let mut lock;
+		get_channel_ref!(nodes[0], lock, channel_id).announcement_sigs_state = AnnouncementSigsState::PeerReceived;
+		get_channel_ref!(nodes[1], lock, channel_id).announcement_sigs_state = AnnouncementSigsState::PeerReceived;
+	}
 
 	// Route the payment and deliver the initial commitment_signed (with a monitor update failure
 	// on receipt).
@@ -1789,9 +1795,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
 }
 
-// confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
+// restore_b_before_lock has no meaning if confirm_a_first
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool, restore_b_before_lock: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1833,6 +1839,8 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 	if confirm_a_first {
 		confirm_transaction(&nodes[0], &funding_tx);
 		nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id()));
+		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 	} else {
 		assert!(!restore_b_before_conf);
 		confirm_transaction(&nodes[1], &funding_tx);
@@ -1851,20 +1859,32 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 	}
+	if !confirm_a_first && !restore_b_before_lock {
+		confirm_transaction(&nodes[0], &funding_tx);
+		nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id()));
+		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+	}
 
 	chanmon_cfgs[1].persister.set_update_ret(Ok(()));
 	let (outpoint, latest_update, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
 	nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
 	check_added_monitors!(nodes[1], 0);
 
 	let (channel_id, (announcement, as_update, bs_update)) = if !confirm_a_first {
-		nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id()));
-
-		confirm_transaction(&nodes[0], &funding_tx);
-		let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
-		(channel_id, create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked))
+		if !restore_b_before_lock {
+			let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[0], &nodes[1]);
+			(channel_id, create_chan_between_nodes_with_value_b(&nodes[1], &nodes[0], &funding_locked))
+		} else {
+			nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id()));
+			confirm_transaction(&nodes[0], &funding_tx);
+			let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
+			(channel_id, create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked))
+		}
 	} else {
 		if restore_b_before_conf {
+			assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+			assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 			confirm_transaction(&nodes[1], &funding_tx);
 		}
 		let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[0], &nodes[1]);
@@ -1884,9 +1904,10 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(true, true);
-	do_during_funding_monitor_fail(true, false);
-	do_during_funding_monitor_fail(false, false);
+	do_during_funding_monitor_fail(true, true, false);
+	do_during_funding_monitor_fail(true, false, false);
+	do_during_funding_monitor_fail(false, false, false);
+	do_during_funding_monitor_fail(false, false, true);
 }
 
 #[test]