Merge pull request #3638 from wpaulino/bye-bye-outpoint-to-peer

Require counterparty_node_id TLV for ChannelMonitor

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -282,7 +282,7 @@ impl chain::Watch<TestChannelSigner> for TestChainMonitor {
 
 	fn release_pending_monitor_events(
 		&self,
-	) -> Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, Option<PublicKey>)> {
+	) -> Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, PublicKey)> {
 		return self.chain_monitor.release_pending_monitor_events();
 	}
 }

lightning/src/chain/chainmonitor.rs

@@ -246,7 +246,7 @@ pub struct ChainMonitor<ChannelSigner: EcdsaChannelSigner, C: Deref, T: Deref, F
 	persister: P,
 	/// "User-provided" (ie persistence-completion/-failed) [`MonitorEvent`]s. These came directly
 	/// from the user and not from a [`ChannelMonitor`].
-	pending_monitor_events: Mutex<Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, Option<PublicKey>)>>,
+	pending_monitor_events: Mutex<Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, PublicKey)>>,
 	/// The best block height seen, used as a proxy for the passage of time.
 	highest_chain_height: AtomicUsize,
 
@@ -804,7 +804,7 @@ where C::Target: chain::Filter,
 		let monitors = self.monitors.read().unwrap();
 		match monitors.get(&channel_id) {
 			None => {
-				let logger = WithContext::from(&self.logger, update.counterparty_node_id, Some(channel_id), None);
+				let logger = WithContext::from(&self.logger, None, Some(channel_id), None);
 				log_error!(logger, "Failed to update channel monitor: no such monitor registered");
 
 				// We should never ever trigger this from within ChannelManager. Technically a
@@ -874,7 +874,7 @@ where C::Target: chain::Filter,
 		}
 	}
 
-	fn release_pending_monitor_events(&self) -> Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, Option<PublicKey>)> {
+	fn release_pending_monitor_events(&self) -> Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, PublicKey)> {
 		let mut pending_monitor_events = self.pending_monitor_events.lock().unwrap().split_off(0);
 		for monitor_state in self.monitors.read().unwrap().values() {
 			let monitor_events = monitor_state.monitor.get_and_clear_pending_monitor_events();

lightning/src/chain/channelmonitor.rs

@@ -74,15 +74,6 @@ use crate::sync::{Mutex, LockTestExt};
 #[must_use]
 pub struct ChannelMonitorUpdate {
 	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
-	/// Historically, [`ChannelMonitor`]s didn't know their counterparty node id. However,
-	/// `ChannelManager` really wants to know it so that it can easily look up the corresponding
-	/// channel. For now, this results in a temporary map in `ChannelManager` to look up channels
-	/// by only the funding outpoint.
-	///
-	/// To eventually remove that, we repeat the counterparty node id here so that we can upgrade
-	/// `ChannelMonitor`s to become aware of the counterparty node id if they were generated prior
-	/// to when it was stored directly in them.
-	pub(crate) counterparty_node_id: Option<PublicKey>,
 	/// The sequence number of this update. Updates *must* be replayed in-order according to this
 	/// sequence number (and updates may panic if they are not). The update_id values are strictly
 	/// increasing and increase by one for each new update, with two exceptions specified below.
@@ -117,7 +108,7 @@ impl Writeable for ChannelMonitorUpdate {
 			update_step.write(w)?;
 		}
 		write_tlv_fields!(w, {
-			(1, self.counterparty_node_id, option),
+			// 1 was previously used to store `counterparty_node_id`
 			(3, self.channel_id, option),
 		});
 		Ok(())
@@ -134,13 +125,12 @@ impl Readable for ChannelMonitorUpdate {
 				updates.push(upd);
 			}
 		}
-		let mut counterparty_node_id = None;
 		let mut channel_id = None;
 		read_tlv_fields!(r, {
-			(1, counterparty_node_id, option),
+			// 1 was previously used to store `counterparty_node_id`
 			(3, channel_id, option),
 		});
-		Ok(Self { update_id, counterparty_node_id, updates, channel_id })
+		Ok(Self { update_id, updates, channel_id })
 	}
 }
 
@@ -1020,7 +1010,7 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	best_block: BestBlock,
 
 	/// The node_id of our counterparty
-	counterparty_node_id: Option<PublicKey>,
+	counterparty_node_id: PublicKey,
 
 	/// Initial counterparty commmitment data needed to recreate the commitment tx
 	/// in the persistence pipeline for third-party watchtowers. This will only be present on
@@ -1242,7 +1232,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
 			(3, self.htlcs_resolved_on_chain, required_vec),
 			(5, pending_monitor_events, required_vec),
 			(7, self.funding_spend_seen, required),
-			(9, self.counterparty_node_id, option),
+			(9, self.counterparty_node_id, required),
 			(11, self.confirmed_commitment_tx_counterparty_output, option),
 			(13, self.spendable_txids_confirmed, required_vec),
 			(15, self.counterparty_fulfilled_htlcs, required),
@@ -1338,7 +1328,7 @@ impl<'a, L: Deref> WithChannelMonitor<'a, L> where L::Target: Logger {
 	}
 
 	pub(crate) fn from_impl<S: EcdsaChannelSigner>(logger: &'a L, monitor_impl: &ChannelMonitorImpl<S>, payment_hash: Option<PaymentHash>) -> Self {
-		let peer_id = monitor_impl.counterparty_node_id;
+		let peer_id = Some(monitor_impl.counterparty_node_id);
 		let channel_id = Some(monitor_impl.channel_id());
 		WithChannelMonitor {
 			logger, peer_id, channel_id, payment_hash,
@@ -1462,7 +1452,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			spendable_txids_confirmed: Vec::new(),
 
 			best_block,
-			counterparty_node_id: Some(counterparty_node_id),
+			counterparty_node_id: counterparty_node_id,
 			initial_counterparty_commitment_info: None,
 			balances_empty_height: None,
 
@@ -1788,10 +1778,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	}
 
 	/// Gets the `node_id` of the counterparty for this channel.
-	///
-	/// Will be `None` for channels constructed on LDK versions prior to 0.0.110 and always `Some`
-	/// otherwise.
-	pub fn get_counterparty_node_id(&self) -> Option<PublicKey> {
+	pub fn get_counterparty_node_id(&self) -> PublicKey {
 		self.inner.lock().unwrap().counterparty_node_id
 	}
 
@@ -3200,14 +3187,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len());
 		}
 
-		if updates.counterparty_node_id.is_some() {
-			if self.counterparty_node_id.is_none() {
-				self.counterparty_node_id = updates.counterparty_node_id;
-			} else {
-				debug_assert_eq!(self.counterparty_node_id, updates.counterparty_node_id);
-			}
-		}
-
 		// ChannelMonitor updates may be applied after force close if we receive a preimage for a
 		// broadcasted commitment transaction HTLC output that we'd like to claim on-chain. If this
 		// is the case, we no longer have guaranteed access to the monitor's update ID, so we use a
@@ -3376,10 +3355,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					package_target_feerate_sat_per_1000_weight, commitment_tx, anchor_output_idx,
 				} => {
 					let channel_id = self.channel_id;
-					// unwrap safety: `ClaimEvent`s are only available for Anchor channels,
-					// introduced with v0.0.116. counterparty_node_id is guaranteed to be `Some`
-					// since v0.0.110.
-					let counterparty_node_id = self.counterparty_node_id.unwrap();
+					let counterparty_node_id = self.counterparty_node_id;
 					let commitment_txid = commitment_tx.compute_txid();
 					debug_assert_eq!(self.current_holder_commitment_tx.txid, commitment_txid);
 					let pending_htlcs = self.current_holder_commitment_tx.non_dust_htlcs();
@@ -3410,10 +3386,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					target_feerate_sat_per_1000_weight, htlcs, tx_lock_time,
 				} => {
 					let channel_id = self.channel_id;
-					// unwrap safety: `ClaimEvent`s are only available for Anchor channels,
-					// introduced with v0.0.116. counterparty_node_id is guaranteed to be `Some`
-					// since v0.0.110.
-					let counterparty_node_id = self.counterparty_node_id.unwrap();
+					let counterparty_node_id = self.counterparty_node_id;
 					let mut htlc_descriptors = Vec::with_capacity(htlcs.len());
 					for htlc in htlcs {
 						htlc_descriptors.push(HTLCDescriptor {
@@ -5129,6 +5102,13 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 				chan_utils::get_to_countersignatory_with_anchors_redeemscript(&payment_point).to_p2wsh();
 		}
 
+		let channel_id = channel_id.unwrap_or(ChannelId::v1_from_funding_outpoint(outpoint));
+		if counterparty_node_id.is_none() {
+			panic!("Found monitor for channel {} with no updates since v0.0.118.\
+				These monitors are no longer supported.\
+				To continue, run a v0.1 release, send/route a payment over the channel or close it.", channel_id);
+		}
+
 		Ok((best_block.block_hash, ChannelMonitor::from_impl(ChannelMonitorImpl {
 			latest_update_id,
 			commitment_transaction_number_obscure_factor,
@@ -5140,7 +5120,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 
 			channel_keys_id,
 			holder_revocation_basepoint,
-			channel_id: channel_id.unwrap_or(ChannelId::v1_from_funding_outpoint(outpoint)),
+			channel_id,
 			funding_info,
 			first_confirmed_funding_txo: first_confirmed_funding_txo.0.unwrap(),
 			current_counterparty_commitment_txid,
@@ -5184,7 +5164,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			spendable_txids_confirmed: spendable_txids_confirmed.unwrap(),
 
 			best_block,
-			counterparty_node_id,
+			counterparty_node_id: counterparty_node_id.unwrap(),
 			initial_counterparty_commitment_info,
 			balances_empty_height,
 			failed_back_htlc_ids: new_hash_set(),

lightning/src/chain/mod.rs

@@ -304,7 +304,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
 	///
 	/// For details on asynchronous [`ChannelMonitor`] updating and returning
 	/// [`MonitorEvent::Completed`] here, see [`ChannelMonitorUpdateStatus::InProgress`].
-	fn release_pending_monitor_events(&self) -> Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, Option<PublicKey>)>;
+	fn release_pending_monitor_events(&self) -> Vec<(OutPoint, ChannelId, Vec<MonitorEvent>, PublicKey)>;
 }
 
 /// The `Filter` trait defines behavior for indicating chain activity of interest pertaining to

lightning/src/ln/async_signer_tests.rs

@@ -18,7 +18,6 @@ use bitcoin::transaction::Version;
 
 use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
 use crate::chain::ChannelMonitorUpdateStatus;
-use crate::chain::transaction::OutPoint;
 use crate::events::bump_transaction::WalletSource;
 use crate::events::{ClosureReason, Event};
 use crate::ln::chan_utils::ClosingTransaction;
@@ -1091,9 +1090,4 @@ fn do_test_closing_signed(extra_closing_signed: bool, reconnect: bool) {
 	assert!(nodes[1].node.list_channels().is_empty());
 	check_closed_event!(nodes[0], 1, ClosureReason::LocallyInitiatedCooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
 	check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyInitiatedCooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
-
-	// Check that our maps have been updated after async signing channel closure.
-	let funding_outpoint = OutPoint { txid: funding_tx.compute_txid(), index: 0 };
-	assert!(nodes[0].node().outpoint_to_peer.lock().unwrap().get(&funding_outpoint).is_none());
-	assert!(nodes[1].node().outpoint_to_peer.lock().unwrap().get(&funding_outpoint).is_none());
 }

lightning/src/ln/channel.rs

@@ -3457,6 +3457,18 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			!matches!(self.channel_state, ChannelState::AwaitingChannelReady(flags) if flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH))
 	}
 
+	fn unset_funding_info(&mut self, funding: &mut FundingScope) {
+		debug_assert!(
+			matches!(self.channel_state, ChannelState::FundingNegotiated)
+				|| matches!(self.channel_state, ChannelState::AwaitingChannelReady(_))
+		);
+		funding.channel_transaction_parameters.funding_outpoint = None;
+		self.channel_id = self.temporary_channel_id.expect(
+			"temporary_channel_id should be set since unset_funding_info is only called on funded \
+			 channels that were unfunded immediately beforehand"
+		);
+	}
+
 	fn validate_commitment_signed<L: Deref>(
 		&self, funding: &FundingScope, holder_commitment_point: &HolderCommitmentPoint,
 		msg: &msgs::CommitmentSigned, logger: &L,
@@ -4516,7 +4528,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 				self.latest_monitor_update_id += 1;
 				Some((self.get_counterparty_node_id(), funding_txo, self.channel_id(), ChannelMonitorUpdate {
 					update_id: self.latest_monitor_update_id,
-					counterparty_node_id: Some(self.counterparty_node_id),
 					updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast }],
 					channel_id: Some(self.channel_id()),
 				}))
@@ -5095,7 +5106,6 @@ impl<SP: Deref> FundedChannel<SP> where
 		self.context.latest_monitor_update_id += 1;
 		let monitor_update = ChannelMonitorUpdate {
 			update_id: self.context.latest_monitor_update_id,
-			counterparty_node_id: Some(self.context.counterparty_node_id),
 			updates: vec![ChannelMonitorUpdateStep::PaymentPreimage {
 				payment_preimage: payment_preimage_arg.clone(),
 				payment_info,
@@ -5310,14 +5320,7 @@ impl<SP: Deref> FundedChannel<SP> where
 	/// Further, the channel must be immediately shut down after this with a call to
 	/// [`ChannelContext::force_shutdown`].
 	pub fn unset_funding_info(&mut self) {
-		debug_assert!(matches!(
-			self.context.channel_state, ChannelState::AwaitingChannelReady(_)
-		));
-		self.funding.channel_transaction_parameters.funding_outpoint = None;
-		self.context.channel_id = self.context.temporary_channel_id.expect(
-			"temporary_channel_id should be set since unset_funding_info is only called on funded \
-			 channels that were unfunded immediately beforehand"
-		);
+		self.context.unset_funding_info(&mut self.funding);
 	}
 
 	/// Handles a channel_ready message from our peer. If we've already sent our channel_ready
@@ -5710,7 +5713,6 @@ impl<SP: Deref> FundedChannel<SP> where
 		self.context.latest_monitor_update_id += 1;
 		let mut monitor_update = ChannelMonitorUpdate {
 			update_id: self.context.latest_monitor_update_id,
-			counterparty_node_id: Some(self.context.counterparty_node_id),
 			updates: vec![ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo {
 				commitment_tx,
 				htlc_outputs,
@@ -5792,7 +5794,6 @@ impl<SP: Deref> FundedChannel<SP> where
 
 			let mut monitor_update = ChannelMonitorUpdate {
 				update_id: self.context.latest_monitor_update_id + 1, // We don't increment this yet!
-				counterparty_node_id: Some(self.context.counterparty_node_id),
 				updates: Vec::new(),
 				channel_id: Some(self.context.channel_id()),
 			};
@@ -5985,7 +5986,6 @@ impl<SP: Deref> FundedChannel<SP> where
 		self.context.latest_monitor_update_id += 1;
 		let mut monitor_update = ChannelMonitorUpdate {
 			update_id: self.context.latest_monitor_update_id,
-			counterparty_node_id: Some(self.context.counterparty_node_id),
 			updates: vec![ChannelMonitorUpdateStep::CommitmentSecret {
 				idx: self.context.cur_counterparty_commitment_transaction_number + 1,
 				secret: msg.per_commitment_secret,
@@ -7257,7 +7257,6 @@ impl<SP: Deref> FundedChannel<SP> where
 			self.context.latest_monitor_update_id += 1;
 			let monitor_update = ChannelMonitorUpdate {
 				update_id: self.context.latest_monitor_update_id,
-				counterparty_node_id: Some(self.context.counterparty_node_id),
 				updates: vec![ChannelMonitorUpdateStep::ShutdownScript {
 					scriptpubkey: self.get_closing_scriptpubkey(),
 				}],
@@ -8543,7 +8542,6 @@ impl<SP: Deref> FundedChannel<SP> where
 		self.context.latest_monitor_update_id += 1;
 		let monitor_update = ChannelMonitorUpdate {
 			update_id: self.context.latest_monitor_update_id,
-			counterparty_node_id: Some(self.context.counterparty_node_id),
 			updates: vec![ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo {
 				commitment_txid: counterparty_commitment_txid,
 				htlc_outputs: htlcs.clone(),
@@ -8755,7 +8753,6 @@ impl<SP: Deref> FundedChannel<SP> where
 			self.context.latest_monitor_update_id += 1;
 			let monitor_update = ChannelMonitorUpdate {
 				update_id: self.context.latest_monitor_update_id,
-				counterparty_node_id: Some(self.context.counterparty_node_id),
 				updates: vec![ChannelMonitorUpdateStep::ShutdownScript {
 					scriptpubkey: self.get_closing_scriptpubkey(),
 				}],
@@ -9315,6 +9312,14 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 		} else { None };
 		(open_channel, funding_created)
 	}
+
+	/// Unsets the existing funding information.
+	///
+	/// The channel must be immediately shut down after this with a call to
+	/// [`ChannelContext::force_shutdown`].
+	pub fn unset_funding_info(&mut self) {
+		self.context.unset_funding_info(&mut self.funding);
+	}
 }
 
 /// A not-yet-funded inbound (from counterparty) channel using V1 channel establishment.