Require option_static_remotekey in channel/channelmonitor.

TheBlueMatt · TheBlueMatt · commit ab3d2fd381a4 · 2020-03-09T15:57:25.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/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
@@ -73,6 +73,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
@@ -259,11 +259,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)]
@@ -298,14 +296,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)?,
 		})
 	}
 }
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
@@ -244,7 +244,7 @@ impl MsgHandleErrInternal {
 						},
 					},
 				},
-				ChannelError::CloseDelayBroadcast { msg, .. } => LightningError {
+				ChannelError::CloseDelayBroadcast(msg) => LightningError {
 					err: msg,
 					action: msgs::ErrorAction::SendErrorMessage {
 						msg: msgs::ErrorMessage {
@@ -541,7 +541,7 @@ macro_rules! break_chan_entry {
 				}
 				break Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, chan.force_shutdown(), $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"); }
+			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"); }
 		}
 	}
 }
@@ -561,22 +561,12 @@ macro_rules! try_chan_entry {
 				}
 				return Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, chan.force_shutdown(), $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 mut shutdown_res = chan.force_shutdown();
 				if shutdown_res.0.len() >= 1 {
 					log_error!($self, "You have a toxic local commitment transaction {} avaible in channel monitor, read comment in ChannelMonitor::get_latest_local_commitment_txn to be informed of manual action to take", shutdown_res.0[0].txid());
@@ -1635,7 +1625,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 											}
 											Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(), 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"); }
 									};
 									match handle_error!(self, err, their_node_id, channel_state) {
 										Ok(_) => unreachable!(),
diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs
@@ -616,11 +616,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,
-	},
 }
 
 impl Writeable for ChannelMonitorUpdateStep {
@@ -658,10 +653,6 @@ 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)?;
-			},
 		}
 		Ok(())
 	}
@@ -710,11 +701,6 @@ impl Readable for ChannelMonitorUpdateStep {
 					secret: Readable::read(r)?,
 				})
 			},
-			4u8 => {
-				Ok(ChannelMonitorUpdateStep::RescueRemoteCommitmentTXInfo {
-					their_current_per_commitment_point: Readable::read(r)?,
-				})
-			},
 			_ => Err(DecodeError::InvalidValue),
 		}
 	}
@@ -775,11 +761,6 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 	pending_htlcs_updated: Vec<HTLCUpdate>,
 	pending_events: Vec<events::Event>,
 
-	// Thanks to data loss protection, we may be able to claim our non-htlc funds
-	// back, this is the script we have to spend from but we need to
-	// scan every commitment transaction for that
-	to_remote_rescue: Option<(Script, SecretKey)>,
-
 	// Used to track onchain events, i.e transactions parts of channels confirmed on chain, on which
 	// we have to take actions once they reach enough confs. Key is a block height timer, i.e we enforce
 	// actions when we receive a block with given height. Actions depend on OnchainEvent type.
@@ -831,7 +812,6 @@ impl<ChanSigner: ChannelKeys> PartialEq for ChannelMonitor<ChanSigner> {
 			self.payment_preimages != other.payment_preimages ||
 			self.pending_htlcs_updated != other.pending_htlcs_updated ||
 			self.pending_events.len() != other.pending_events.len() || // We trust events to round-trip properly
-			self.to_remote_rescue != other.to_remote_rescue ||
 			self.onchain_events_waiting_threshold_conf != other.onchain_events_waiting_threshold_conf ||
 			self.outputs_to_watch != other.outputs_to_watch
 		{
@@ -1009,13 +989,6 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 		}
 
 		self.last_block_hash.write(writer)?;
-		if let Some((ref to_remote_script, ref local_key)) = self.to_remote_rescue {
-			writer.write_all(&[1; 1])?;
-			to_remote_script.write(writer)?;
-			local_key.write(writer)?;
-		} else {
-			writer.write_all(&[0; 1])?;
-		}
 
 		writer.write_all(&byte_utils::be64_to_array(self.onchain_events_waiting_threshold_conf.len() as u64))?;
 		for (ref target, ref events) in self.onchain_events_waiting_threshold_conf.iter() {
@@ -1120,8 +1093,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			pending_htlcs_updated: Vec::new(),
 			pending_events: Vec::new(),
 
-			to_remote_rescue: None,
-
 			onchain_events_waiting_threshold_conf: HashMap::new(),
 			outputs_to_watch: HashMap::new(),
 
@@ -1229,22 +1200,6 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		}
 	}
 
-	pub(super) fn provide_rescue_remote_commitment_tx_info(&mut self, their_revocation_point: PublicKey) {
-		match self.key_storage {
-			Storage::Local { ref payment_base_key, ref keys, .. } => {
-				if let Ok(payment_key) = chan_utils::derive_public_key(&self.secp_ctx, &their_revocation_point, &keys.pubkeys().payment_basepoint) {
-					let to_remote_script =  Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0)
-						.push_slice(&Hash160::hash(&payment_key.serialize())[..])
-						.into_script();
-					if let Ok(to_remote_key) = chan_utils::derive_private_key(&self.secp_ctx, &their_revocation_point, &payment_base_key) {
-						self.to_remote_rescue = Some((to_remote_script, to_remote_key));
-					}
-				}
-			},
-			Storage::Watchtower { .. } => {}
-		}
-	}
-
 	/// 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
@@ -1287,8 +1242,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),
 			}
 		}
 		self.latest_update_id = updates.update_id;
@@ -1313,8 +1266,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),
 			}
 		}
 		self.latest_update_id = updates.update_id;
@@ -1436,7 +1387,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					(ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &keys.pubkeys().revocation_basepoint)),
 					ignore_error!(chan_utils::derive_private_revocation_key(&self.secp_ctx, &per_commitment_key, &revocation_base_key)),
 					ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &keys.pubkeys().htlc_basepoint)),
-					Some(ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, &per_commitment_point, &payment_base_key))))
+					Some(payment_base_key))
 				},
 				Storage::Watchtower { .. } => {
 					unimplemented!()
@@ -1466,7 +1417,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 				} else if Some(&outp.script_pubkey) == local_payment_p2wpkh.as_ref() {
 					spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
 						outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
-						key: local_payment_key.unwrap(),
+						key: local_payment_key.unwrap().clone(),
 						output: outp.clone(),
 					});
 				}
@@ -1620,13 +1571,11 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 						if outp.script_pubkey.is_v0_p2wpkh() {
 							match self.key_storage {
 								Storage::Local { ref payment_base_key, .. } => {
-									if let Ok(local_key) = chan_utils::derive_private_key(&self.secp_ctx, &revocation_point, &payment_base_key) {
-										spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
-											outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
-											key: local_key,
-											output: outp.clone(),
-										});
-									}
+									spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
+										outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
+										key: payment_base_key.clone(),
+										output: outp.clone(),
+									});
 								},
 								Storage::Watchtower { .. } => {}
 							}
@@ -1653,15 +1602,24 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					}
 				}
 			}
-		} else if let Some((ref to_remote_rescue, ref local_key)) = self.to_remote_rescue {
-			for (idx, outp) in tx.output.iter().enumerate() {
-				if to_remote_rescue == &outp.script_pubkey {
-					spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
-						outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
-						key: local_key.clone(),
-						output: outp.clone(),
-					});
-				}
+		} else {
+			match self.key_storage {
+				Storage::Local { ref payment_base_key, .. } => {
+					let payment_hash160 = Hash160::hash(&PublicKey::from_secret_key(&self.secp_ctx, &payment_base_key).serialize());
+					let our_payment_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0)
+						.push_slice(&payment_hash160)
+						.into_script();
+					for (idx, outp) in tx.output.iter().enumerate() {
+						if our_payment_script == outp.script_pubkey {
+							spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
+								outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
+								key: payment_base_key.clone(),
+								output: outp.clone(),
+							});
+						}
+					}
+				},
+				Storage::Watchtower { .. } => unimplemented!(),
 			}
 		}
 		(claimable_outpoints, (commitment_txid, watch_outputs), spendable_outputs)
@@ -2524,15 +2482,6 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for (Sha256dH
 		}
 
 		let last_block_hash: Sha256dHash = Readable::read(reader)?;
-		let to_remote_rescue = match <u8 as Readable>::read(reader)? {
-			0 => None,
-			1 => {
-				let to_remote_script = Readable::read(reader)?;
-				let local_key = Readable::read(reader)?;
-				Some((to_remote_script, local_key))
-			}
-			_ => return Err(DecodeError::InvalidValue),
-		};
 
 		let waiting_threshold_conf_len: u64 = Readable::read(reader)?;
 		let mut onchain_events_waiting_threshold_conf = HashMap::with_capacity(cmp::min(waiting_threshold_conf_len as usize, MAX_ALLOC_SIZE / 128));
@@ -2598,8 +2547,6 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for (Sha256dH
 			pending_htlcs_updated,
 			pending_events,
 
-			to_remote_rescue,
-
 			onchain_events_waiting_threshold_conf,
 			outputs_to_watch,
 
@@ -2652,7 +2599,6 @@ mod tests {
 						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(),
 					}
 				}
 			}
diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
@@ -89,6 +89,7 @@ impl InitFeatures {
 				1 << 1 |          // data_loss_protect supported
 				1 << 5,           // upfront_shutdown_script supported
 				1 << (9  - 8*1) | // variable_length_onion supported
+				1 << (12 - 8*1) | // static_remotekey required
 				1 << (15 - 8*1),  // basic_mpp supportedyment_secret supported
 				1 << (17 - 8*2)   // basic_mpp supported
 			],
@@ -210,8 +211,8 @@ impl<T: sealed::Context> Features<T> {
 				// unknown, upfront_shutdown_script, unknown (actually initial_routing_sync, but it
 				// is only valid as an optional feature), and data_loss_protect:
 				0 => (byte & 0b01000100),
-				// payment_secret, unknown, unknown, var_onion_optin:
-				1 => (byte & 0b00010100),
+				// payment_secret, static_remotekey, unknown, var_onion_optin:
+				1 => (byte & 0b00000100),
 				// unknown, unknown, unknown, basic_mpp:
 				2 => (byte & 0b01010100),
 				// fallback, all even bits set:
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -6553,7 +6553,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;
@@ -6614,10 +6614,8 @@ 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], 1);
-
 	{
 		let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
 		assert_eq!(node_txn.len(), 0);
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
@@ -637,6 +637,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::supported();
diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs
@@ -35,7 +35,6 @@ impl EnforcingChannelKeys {
 	fn check_keys<T: secp256k1::Signing + secp256k1::Verification>(&self, secp_ctx: &Secp256k1<T>,
 	                                                               keys: &TxCreationKeys) {
 		let revocation_base = PublicKey::from_secret_key(secp_ctx, &self.inner.revocation_base_key());
-		let payment_base = PublicKey::from_secret_key(secp_ctx, &self.inner.payment_base_key());
 		let htlc_base = PublicKey::from_secret_key(secp_ctx, &self.inner.htlc_base_key());
 
 		let remote_points = self.inner.remote_channel_pubkeys.as_ref().unwrap();
@@ -45,7 +44,6 @@ impl EnforcingChannelKeys {
 		                                        &remote_points.delayed_payment_basepoint,
 		                                        &remote_points.htlc_basepoint,
 		                                        &revocation_base,
-		                                        &payment_base,
 		                                        &htlc_base).unwrap();
 		if keys != &keys_expected { panic!("derived different per-tx keys") }
 	}
