Require option_static_remotekey in channel/channelmonitor.

TheBlueMatt · TheBlueMatt · commit daf2aa6353a5 · 2020-04-29T15:26:11.000-04:00
This simplifies channelmonitor quite nicely (as expected) as we
never have to be concerned with learning data in a DataLossProtect
which is require for us to claim our funds from the latest remote
commitment transaction.
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
@@ -74,6 +74,8 @@ pub enum SpendableOutputDescriptor {
 		/// The output which is referenced by the given outpoint
 		output: TxOut,
 	},
+	// TODO: Note that because key is now static and exactly what is provided by us, we should drop
+	// this in favor of StaticOutput:
 	/// An output to a P2WPKH, spendable exclusively by the given private key.
 	/// The witness in the spending input, is, thus, simply:
 	/// <BIP 143 signature generated with the given key> <public key derived from the given key>
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
@@ -262,11 +262,9 @@ pub struct TxCreationKeys {
 	pub(crate) b_htlc_key: PublicKey,
 	/// A's Payment Key (which isn't allowed to be spent from for some delay)
 	pub(crate) a_delayed_payment_key: PublicKey,
-	/// B's Payment Key
-	pub(crate) b_payment_key: PublicKey,
 }
 impl_writeable!(TxCreationKeys, 33*6,
-	{ per_commitment_point, revocation_key, a_htlc_key, b_htlc_key, a_delayed_payment_key, b_payment_key });
+	{ per_commitment_point, revocation_key, a_htlc_key, b_htlc_key, a_delayed_payment_key });
 
 /// One counterparty's public keys which do not change over the life of a channel.
 #[derive(Clone, PartialEq)]
@@ -301,14 +299,13 @@ impl_writeable!(ChannelPublicKeys, 33*5, {
 
 
 impl TxCreationKeys {
-	pub(crate) fn new<T: secp256k1::Signing + secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, a_delayed_payment_base: &PublicKey, a_htlc_base: &PublicKey, b_revocation_base: &PublicKey, b_payment_base: &PublicKey, b_htlc_base: &PublicKey) -> Result<TxCreationKeys, secp256k1::Error> {
+	pub(crate) fn new<T: secp256k1::Signing + secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, a_delayed_payment_base: &PublicKey, a_htlc_base: &PublicKey, b_revocation_base: &PublicKey, b_htlc_base: &PublicKey) -> Result<TxCreationKeys, secp256k1::Error> {
 		Ok(TxCreationKeys {
 			per_commitment_point: per_commitment_point.clone(),
 			revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &b_revocation_base)?,
 			a_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &a_htlc_base)?,
 			b_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &b_htlc_base)?,
 			a_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &a_delayed_payment_base)?,
-			b_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &b_payment_base)?,
 		})
 	}
 }
@@ -537,7 +534,6 @@ impl LocalCommitmentTransaction {
 					a_htlc_key: dummy_key.clone(),
 					b_htlc_key: dummy_key.clone(),
 					a_delayed_payment_key: dummy_key.clone(),
-					b_payment_key: dummy_key.clone(),
 				},
 			feerate_per_kw: 0,
 			per_htlc: Vec::new()
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -252,7 +252,7 @@ impl MsgHandleErrInternal {
 						},
 					},
 				},
-				ChannelError::CloseDelayBroadcast { msg, .. } => LightningError {
+				ChannelError::CloseDelayBroadcast(msg) => LightningError {
 					err: msg,
 					action: msgs::ErrorAction::SendErrorMessage {
 						msg: msgs::ErrorMessage {
@@ -575,8 +575,9 @@ macro_rules! break_chan_entry {
 				if let Some(short_id) = chan.get_short_channel_id() {
 					$channel_state.short_to_id.remove(&short_id);
 				}
-				break Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, chan.force_shutdown(true), $self.get_channel_update(&chan).ok())) },
-			Err(ChannelError::CloseDelayBroadcast { .. }) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
+				break Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, chan.force_shutdown(true), $self.get_channel_update(&chan).ok()))
+			},
+			Err(ChannelError::CloseDelayBroadcast(_)) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
 		}
 	}
 }
@@ -596,22 +597,12 @@ macro_rules! try_chan_entry {
 				}
 				return Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, chan.force_shutdown(true), $self.get_channel_update(&chan).ok()))
 			},
-			Err(ChannelError::CloseDelayBroadcast { msg, update }) => {
+			Err(ChannelError::CloseDelayBroadcast(msg)) => {
 				log_error!($self, "Channel {} need to be shutdown but closing transactions not broadcast due to {}", log_bytes!($entry.key()[..]), msg);
 				let (channel_id, mut chan) = $entry.remove_entry();
 				if let Some(short_id) = chan.get_short_channel_id() {
 					$channel_state.short_to_id.remove(&short_id);
 				}
-				if let Err(e) = $self.monitor.update_monitor(chan.get_funding_txo().unwrap(), update) {
-					match e {
-						// Upstream channel is dead, but we want at least to fail backward HTLCs to save
-						// downstream channels. In case of PermanentFailure, we are not going to be able
-						// to claim back to_remote output on remote commitment transaction. Doesn't
-						// make a difference here, we are concern about HTLCs circuit, not onchain funds.
-						ChannelMonitorUpdateErr::PermanentFailure => {},
-						ChannelMonitorUpdateErr::TemporaryFailure => {},
-					}
-				}
 				let shutdown_res = chan.force_shutdown(false);
 				return Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, shutdown_res, $self.get_channel_update(&chan).ok()))
 			}
@@ -1665,7 +1656,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 											}
 											Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(true), self.get_channel_update(&channel).ok()))
 										},
-										ChannelError::CloseDelayBroadcast { .. } => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
+										ChannelError::CloseDelayBroadcast(_) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
 									};
 									handle_errors.push((their_node_id, err));
 									continue;
diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs
@@ -589,11 +589,6 @@ pub(super) enum ChannelMonitorUpdateStep {
 		idx: u64,
 		secret: [u8; 32],
 	},
-	/// Indicates our channel is likely a stale version, we're closing, but this update should
-	/// allow us to spend what is ours if our counterparty broadcasts their latest state.
-	RescueRemoteCommitmentTXInfo {
-		their_current_per_commitment_point: PublicKey,
-	},
 	/// Used to indicate that the no future updates will occur, and likely that the latest local
 	/// commitment transaction(s) should be broadcast, as the channel has been force-closed.
 	ChannelForceClosed {
@@ -636,12 +631,8 @@ impl Writeable for ChannelMonitorUpdateStep {
 				idx.write(w)?;
 				secret.write(w)?;
 			},
-			&ChannelMonitorUpdateStep::RescueRemoteCommitmentTXInfo { ref their_current_per_commitment_point } => {
-				4u8.write(w)?;
-				their_current_per_commitment_point.write(w)?;
-			},
 			&ChannelMonitorUpdateStep::ChannelForceClosed { ref should_broadcast } => {
-				5u8.write(w)?;
+				4u8.write(w)?;
 				should_broadcast.write(w)?;
 			},
 		}
@@ -691,11 +682,6 @@ impl Readable for ChannelMonitorUpdateStep {
 				})
 			},
 			4u8 => {
-				Ok(ChannelMonitorUpdateStep::RescueRemoteCommitmentTXInfo {
-					their_current_per_commitment_point: Readable::read(r)?,
-				})
-			},
-			5u8 => {
 				Ok(ChannelMonitorUpdateStep::ChannelForceClosed {
 					should_broadcast: Readable::read(r)?
 				})
@@ -721,7 +707,7 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 
 	destination_script: Script,
 	broadcasted_local_revokable_script: Option<(Script, SecretKey, Script)>,
-	broadcasted_remote_payment_script: Option<(Script, SecretKey)>,
+	remote_payment_script: Script,
 	shutdown_script: Script,
 
 	keys: ChanSigner,
@@ -817,7 +803,7 @@ impl<ChanSigner: ChannelKeys> PartialEq for ChannelMonitor<ChanSigner> {
 			self.commitment_transaction_number_obscure_factor != other.commitment_transaction_number_obscure_factor ||
 			self.destination_script != other.destination_script ||
 			self.broadcasted_local_revokable_script != other.broadcasted_local_revokable_script ||
-			self.broadcasted_remote_payment_script != other.broadcasted_remote_payment_script ||
+			self.remote_payment_script != other.remote_payment_script ||
 			self.keys.pubkeys() != other.keys.pubkeys() ||
 			self.funding_info != other.funding_info ||
 			self.current_remote_commitment_txid != other.current_remote_commitment_txid ||
@@ -881,13 +867,7 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 			writer.write_all(&[1; 1])?;
 		}
 
-		if let Some(ref broadcasted_remote_payment_script) = self.broadcasted_remote_payment_script {
-			writer.write_all(&[0; 1])?;
-			broadcasted_remote_payment_script.0.write(writer)?;
-			broadcasted_remote_payment_script.1.write(writer)?;
-		} else {
-			writer.write_all(&[1; 1])?;
-		}
+		self.remote_payment_script.write(writer)?;
 		self.shutdown_script.write(writer)?;
 
 		self.keys.write(writer)?;
@@ -1062,6 +1042,8 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
 		let our_channel_close_key_hash = WPubkeyHash::hash(&shutdown_pubkey.serialize());
 		let shutdown_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_close_key_hash[..]).into_script();
+		let payment_base_key_hash = WPubkeyHash::hash(&keys.pubkeys().payment_basepoint.serialize());
+		let remote_payment_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_base_key_hash[..]).into_script();
 
 		let mut onchain_tx_handler = OnchainTxHandler::new(destination_script.clone(), keys.clone(), their_to_self_delay, logger.clone());
 
@@ -1090,7 +1072,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 
 			destination_script: destination_script.clone(),
 			broadcasted_local_revokable_script: None,
-			broadcasted_remote_payment_script: None,
+			remote_payment_script,
 			shutdown_script,
 
 			keys,
@@ -1227,17 +1209,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		}
 	}
 
-	pub(super) fn provide_rescue_remote_commitment_tx_info(&mut self, their_revocation_point: PublicKey) {
-		if let Ok(payment_key) = chan_utils::derive_public_key(&self.secp_ctx, &their_revocation_point, &self.keys.pubkeys().payment_basepoint) {
-			let to_remote_script =  Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0)
-				.push_slice(&WPubkeyHash::hash(&payment_key.serialize())[..])
-				.into_script();
-			if let Ok(to_remote_key) = chan_utils::derive_private_key(&self.secp_ctx, &their_revocation_point, &self.keys.payment_base_key()) {
-				self.broadcasted_remote_payment_script = Some((to_remote_script, to_remote_key));
-			}
-		}
-	}
-
 	/// Informs this monitor of the latest local (ie broadcastable) commitment transaction. The
 	/// monitor watches for timeouts and may broadcast it if we approach such a timeout. Thus, it
 	/// is important that any clones of this channel monitor (including remote clones) by kept
@@ -1302,8 +1273,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage),
 				ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } =>
 					self.provide_secret(idx, secret)?,
-				ChannelMonitorUpdateStep::RescueRemoteCommitmentTXInfo { their_current_per_commitment_point } =>
-					self.provide_rescue_remote_commitment_tx_info(their_current_per_commitment_point),
 				ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
 			}
 		}
@@ -1333,8 +1302,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage),
 				ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } =>
 					self.provide_secret(idx, secret)?,
-				ChannelMonitorUpdateStep::RescueRemoteCommitmentTXInfo { their_current_per_commitment_point } =>
-					self.provide_rescue_remote_commitment_tx_info(their_current_per_commitment_point),
 				ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast } => {
 					self.lockdown_from_offchain = true;
 					if should_broadcast {
@@ -1449,20 +1416,12 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			let revocation_pubkey = ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.keys.pubkeys().revocation_basepoint));
 			let revocation_key = ignore_error!(chan_utils::derive_private_revocation_key(&self.secp_ctx, &per_commitment_key, &self.keys.revocation_base_key()));
 			let b_htlc_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &self.keys.pubkeys().htlc_basepoint));
-			let local_payment_key = ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, &per_commitment_point, &self.keys.payment_base_key()));
 			let delayed_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.their_delayed_payment_base_key));
 			let a_htlc_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.their_htlc_base_key));
 
 			let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.our_to_self_delay, &delayed_key);
 			let revokeable_p2wsh = revokeable_redeemscript.to_v0_p2wsh();
 
-			self.broadcasted_remote_payment_script = {
-				// Note that the Network here is ignored as we immediately drop the address for the
-				// script_pubkey version
-				let payment_hash160 = WPubkeyHash::hash(&PublicKey::from_secret_key(&self.secp_ctx, &local_payment_key).serialize());
-				Some((Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_hash160[..]).into_script(), local_payment_key))
-			};
-
 			// First, process non-htlc outputs (to_local & to_remote)
 			for (idx, outp) in tx.output.iter().enumerate() {
 				if outp.script_pubkey == revokeable_p2wsh {
@@ -1603,14 +1562,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					let b_htlc_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, revocation_point, &self.keys.pubkeys().htlc_basepoint));
 					let htlc_privkey = ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, revocation_point, &self.keys.htlc_base_key()));
 					let a_htlc_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, revocation_point, &self.their_htlc_base_key));
-					let local_payment_key = ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, revocation_point, &self.keys.payment_base_key()));
-
-					self.broadcasted_remote_payment_script = {
-						// Note that the Network here is ignored as we immediately drop the address for the
-						// script_pubkey version
-						let payment_hash160 = WPubkeyHash::hash(&PublicKey::from_secret_key(&self.secp_ctx, &local_payment_key).serialize());
-						Some((Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_hash160[..]).into_script(), local_payment_key))
-					};
 
 					// Then, try to find htlc outputs
 					for (_, &(ref htlc, _)) in per_commitment_data.iter().enumerate() {
@@ -2176,15 +2127,13 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					});
 					break;
 				}
-			} else if let Some(ref broadcasted_remote_payment_script) = self.broadcasted_remote_payment_script {
-				if broadcasted_remote_payment_script.0 == outp.script_pubkey {
-					spendable_output = Some(SpendableOutputDescriptor::DynamicOutputP2WPKH {
-						outpoint: BitcoinOutPoint { txid: tx.txid(), vout: i as u32 },
-						key: broadcasted_remote_payment_script.1,
-						output: outp.clone(),
-					});
-					break;
-				}
+			} else if self.remote_payment_script == outp.script_pubkey {
+				spendable_output = Some(SpendableOutputDescriptor::DynamicOutputP2WPKH {
+					outpoint: BitcoinOutPoint { txid: tx.txid(), vout: i as u32 },
+					key: self.keys.payment_base_key().clone(),
+					output: outp.clone(),
+				});
+				break;
 			} else if outp.script_pubkey == self.shutdown_script {
 				spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
 					outpoint: BitcoinOutPoint { txid: tx.txid(), vout: i as u32 },
@@ -2240,15 +2189,7 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for (BlockHas
 			1 => { None },
 			_ => return Err(DecodeError::InvalidValue),
 		};
-		let broadcasted_remote_payment_script = match <u8 as Readable>::read(reader)? {
-			0 => {
-				let payment_address = Readable::read(reader)?;
-				let payment_key = Readable::read(reader)?;
-				Some((payment_address, payment_key))
-			},
-			1 => { None },
-			_ => return Err(DecodeError::InvalidValue),
-		};
+		let remote_payment_script = Readable::read(reader)?;
 		let shutdown_script = Readable::read(reader)?;
 
 		let keys = Readable::read(reader)?;
@@ -2464,7 +2405,7 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for (BlockHas
 
 			destination_script,
 			broadcasted_local_revokable_script,
-			broadcasted_remote_payment_script,
+			remote_payment_script,
 			shutdown_script,
 
 			keys,
diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
@@ -89,15 +89,15 @@ mod sealed {
 			// Byte 0
 			,
 			// Byte 1
-			,
+			StaticRemoteKey,
 			// Byte 2
 			,
 		],
 		optional_features: [
 			// Byte 0
 			DataLossProtect | InitialRoutingSync | UpfrontShutdownScript,
 			// Byte 1
-			VariableLengthOnion | StaticRemoteKey | PaymentSecret,
+			VariableLengthOnion | PaymentSecret,
 			// Byte 2
 			BasicMPP,
 		],
@@ -107,15 +107,15 @@ mod sealed {
 			// Byte 0
 			,
 			// Byte 1
-			,
+			StaticRemoteKey,
 			// Byte 2
 			,
 		],
 		optional_features: [
 			// Byte 0
 			DataLossProtect | UpfrontShutdownScript,
 			// Byte 1
-			VariableLengthOnion | StaticRemoteKey | PaymentSecret,
+			VariableLengthOnion | PaymentSecret,
 			// Byte 2
 			BasicMPP,
 		],
@@ -608,11 +608,11 @@ mod tests {
 		{
 			// Check that the flags are as expected:
 			// - option_data_loss_protect
-			// - var_onion_optin | static_remote_key | payment_secret
+			// - var_onion_optin | static_remote_key (req) | payment_secret
 			// - basic_mpp
 			assert_eq!(node_features.flags.len(), 3);
 			assert_eq!(node_features.flags[0], 0b00000010);
-			assert_eq!(node_features.flags[1], 0b10100010);
+			assert_eq!(node_features.flags[1], 0b10010010);
 			assert_eq!(node_features.flags[2], 0b00000010);
 		}
 
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -6807,7 +6807,7 @@ fn test_data_loss_protect() {
 	// We want to be sure that :
 	// * we don't broadcast our Local Commitment Tx in case of fallen behind
 	// * we close channel in case of detecting other being fallen behind
-	// * we are able to claim our own outputs thanks to remote my_current_per_commitment_point
+	// * we are able to claim our own outputs thanks to to_remote being static
 	let keys_manager;
 	let fee_estimator;
 	let tx_broadcaster;
@@ -6868,9 +6868,9 @@ fn test_data_loss_protect() {
 
 	let reestablish_0 = get_chan_reestablish_msgs!(nodes[1], nodes[0]);
 
-	// Check we update monitor following learning of per_commitment_point from B
+	// Check we don't broadcast any transactions following learning of per_commitment_point from B
 	nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_0[0]);
-	check_added_monitors!(nodes[0], 2);
+	check_added_monitors!(nodes[0], 1);
 
 	{
 		let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
@@ -641,6 +641,10 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
 													peer.sync_status = InitSyncTracker::ChannelsSyncing(0);
 													peers.peers_needing_send.insert(peer_descriptor.clone());
 												}
+												if !msg.features.supports_static_remote_key() {
+													log_debug!(self, "Peer {} does not support static remote key, disconnecting with no_connection_possible", log_pubkey!(peer.their_node_id.unwrap()));
+													return Err(PeerHandleError{ no_connection_possible: true });
+												}
 
 												if !peer.outbound {
 													let mut features = InitFeatures::known();
diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs
