Make DataLossProtect fields required and remove wrappers

The fields provided by `DataLossProtect` have been mandatory since
lightning/bolts@6656b70, regardless
of whether `option_dataloss_protect` or `option_remote_key` feature bits
are set.

We move the fields out of `DataLossProtect` to make encoding definitions
more succinct with `impl_writeable_msg!` and to reduce boilerplate.

This paves the way for completely removing `OptionalField` in subsequent
commits.

Author: dunxen

lightning/src/ln/channel.rs

@@ -25,7 +25,7 @@ use bitcoin::secp256k1;
 use crate::ln::{PaymentPreimage, PaymentHash};
 use crate::ln::features::{ChannelTypeFeatures, InitFeatures};
 use crate::ln::msgs;
-use crate::ln::msgs::{DecodeError, OptionalField, DataLossProtect};
+use crate::ln::msgs::{DecodeError, OptionalField};
 use crate::ln::script::{self, ShutdownScript};
 use crate::ln::channelmanager::{self, CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 use crate::ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction};
@@ -4043,32 +4043,27 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 		}
 
 		if msg.next_remote_commitment_number > 0 {
-			match msg.data_loss_protect {
-				OptionalField::Present(ref data_loss) => {
-					let expected_point = self.holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.secp_ctx);
-					let given_secret = SecretKey::from_slice(&data_loss.your_last_per_commitment_secret)
-						.map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?;
-					if expected_point != PublicKey::from_secret_key(&self.secp_ctx, &given_secret) {
-						return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
+			let expected_point = self.holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.secp_ctx);
+			let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret)
+				.map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?;
+			if expected_point != PublicKey::from_secret_key(&self.secp_ctx, &given_secret) {
+				return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
+			}
+			if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number {
+				macro_rules! log_and_panic {
+					($err_msg: expr) => {
+						log_error!(logger, $err_msg, log_bytes!(self.channel_id), log_pubkey!(self.counterparty_node_id));
+						panic!($err_msg, log_bytes!(self.channel_id), log_pubkey!(self.counterparty_node_id));
 					}
-					if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number {
-						macro_rules! log_and_panic {
-							($err_msg: expr) => {
-								log_error!(logger, $err_msg, log_bytes!(self.channel_id), log_pubkey!(self.counterparty_node_id));
-								panic!($err_msg, log_bytes!(self.channel_id), log_pubkey!(self.counterparty_node_id));
-							}
-						}
-						log_and_panic!("We have fallen behind - we have received proof that if we broadcast our counterparty is going to claim all our funds.\n\
-							This implies you have restarted with lost ChannelMonitor and ChannelManager state, the first of which is a violation of the LDK chain::Watch requirements.\n\
-							More specifically, this means you have a bug in your implementation that can cause loss of funds, or you are running with an old backup, which is unsafe.\n\
-							If you have restored from an old backup and wish to force-close channels and return to operation, you should start up, call\n\
-							ChannelManager::force_close_without_broadcasting_txn on channel {} with counterparty {} or\n\
-							ChannelManager::force_close_all_channels_without_broadcasting_txn, then reconnect to peer(s).\n\
-							Note that due to a long-standing bug in lnd you may have to reach out to peers running lnd-based nodes to ask them to manually force-close channels\n\
-							See https://github.com/lightningdevkit/rust-lightning/issues/1565 for more info.");
-					}
-				},
-				OptionalField::Absent => {}
+				}
+				log_and_panic!("We have fallen behind - we have received proof that if we broadcast our counterparty is going to claim all our funds.\n\
+					This implies you have restarted with lost ChannelMonitor and ChannelManager state, the first of which is a violation of the LDK chain::Watch requirements.\n\
+					More specifically, this means you have a bug in your implementation that can cause loss of funds, or you are running with an old backup, which is unsafe.\n\
+					If you have restored from an old backup and wish to force-close channels and return to operation, you should start up, call\n\
+					ChannelManager::force_close_without_broadcasting_txn on channel {} with counterparty {} or\n\
+					ChannelManager::force_close_all_channels_without_broadcasting_txn, then reconnect to peer(s).\n\
+					Note that due to a long-standing bug in lnd you may have to reach out to peers running lnd-based nodes to ask them to manually force-close channels\n\
+					See https://github.com/lightningdevkit/rust-lightning/issues/1565 for more info.");
 			}
 		}
 
@@ -5651,19 +5646,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 		// valid, and valid in fuzzing mode's arbitrary validity criteria:
 		let mut pk = [2; 33]; pk[1] = 0xff;
 		let dummy_pubkey = PublicKey::from_slice(&pk).unwrap();
-		let data_loss_protect = if self.cur_counterparty_commitment_transaction_number + 1 < INITIAL_COMMITMENT_NUMBER {
+		let remote_last_secret = if self.cur_counterparty_commitment_transaction_number + 1 < INITIAL_COMMITMENT_NUMBER {
 			let remote_last_secret = self.commitment_secrets.get_secret(self.cur_counterparty_commitment_transaction_number + 2).unwrap();
 			log_trace!(logger, "Enough info to generate a Data Loss Protect with per_commitment_secret {} for channel {}", log_bytes!(remote_last_secret), log_bytes!(self.channel_id()));
-			OptionalField::Present(DataLossProtect {
-				your_last_per_commitment_secret: remote_last_secret,
-				my_current_per_commitment_point: dummy_pubkey
-			})
+			remote_last_secret
 		} else {
 			log_info!(logger, "Sending a data_loss_protect with no previous remote per_commitment_secret for channel {}", log_bytes!(self.channel_id()));
-			OptionalField::Present(DataLossProtect {
-				your_last_per_commitment_secret: [0;32],
-				my_current_per_commitment_point: dummy_pubkey,
-			})
+			[0;32]
 		};
 		msgs::ChannelReestablish {
 			channel_id: self.channel_id(),
@@ -5685,7 +5674,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			// dropped this channel on disconnect as it hasn't yet reached FundingSent so we can't
 			// overflow here.
 			next_remote_commitment_number: INITIAL_COMMITMENT_NUMBER - self.cur_counterparty_commitment_transaction_number - 1,
-			data_loss_protect,
+			your_last_per_commitment_secret: remote_last_secret,
+			my_current_per_commitment_point: dummy_pubkey,
 		}
 	}
 
@@ -7016,7 +7006,7 @@ mod tests {
 	use crate::ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
 	use crate::ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
 	use crate::ln::features::ChannelTypeFeatures;
-	use crate::ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate, MAX_VALUE_MSAT};
+	use crate::ln::msgs::{ChannelUpdate, DecodeError, UnsignedChannelUpdate, MAX_VALUE_MSAT};
 	use crate::ln::script::ShutdownScript;
 	use crate::ln::chan_utils;
 	use crate::ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
@@ -7317,25 +7307,15 @@ mod tests {
 		let msg = node_b_chan.get_channel_reestablish(&&logger);
 		assert_eq!(msg.next_local_commitment_number, 1); // now called next_commitment_number
 		assert_eq!(msg.next_remote_commitment_number, 0); // now called next_revocation_number
-		match msg.data_loss_protect {
-			OptionalField::Present(DataLossProtect { your_last_per_commitment_secret, .. }) => {
-				assert_eq!(your_last_per_commitment_secret, [0; 32]);
-			},
-			_ => panic!()
-		}
+		assert_eq!(msg.your_last_per_commitment_secret, [0; 32]);
 
 		// Check that the commitment point in Node A's channel_reestablish message
 		// is sane.
 		node_a_chan.remove_uncommitted_htlcs_and_mark_paused(&&logger);
 		let msg = node_a_chan.get_channel_reestablish(&&logger);
 		assert_eq!(msg.next_local_commitment_number, 1); // now called next_commitment_number
 		assert_eq!(msg.next_remote_commitment_number, 0); // now called next_revocation_number
-		match msg.data_loss_protect {
-			OptionalField::Present(DataLossProtect { your_last_per_commitment_secret, .. }) => {
-				assert_eq!(your_last_per_commitment_secret, [0; 32]);
-			},
-			_ => panic!()
-		}
+		assert_eq!(msg.your_last_per_commitment_secret, [0; 32]);
 	}
 
 	#[test]

lightning/src/ln/msgs.rs

@@ -458,20 +458,6 @@ pub struct UpdateFee {
 	pub feerate_per_kw: u32,
 }
 
-#[derive(Clone, Debug, PartialEq, Eq)]
-/// Proof that the sender knows the per-commitment secret of the previous commitment transaction.
-///
-/// This is used to convince the recipient that the channel is at a certain commitment
-/// number even if they lost that data due to a local failure. Of course, the peer may lie
-/// and even later commitments may have been revoked.
-pub struct DataLossProtect {
-	/// Proof that the sender knows the per-commitment secret of a specific commitment transaction
-	/// belonging to the recipient
-	pub your_last_per_commitment_secret: [u8; 32],
-	/// The sender's per-commitment point for their current commitment transaction
-	pub my_current_per_commitment_point: PublicKey,
-}
-
 /// A [`channel_reestablish`] message to be sent to or received from a peer.
 ///
 /// [`channel_reestablish`]: https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#message-retransmission
@@ -483,8 +469,11 @@ pub struct ChannelReestablish {
 	pub next_local_commitment_number: u64,
 	/// The next commitment number for the recipient
 	pub next_remote_commitment_number: u64,
-	/// Optionally, a field proving that next_remote_commitment_number-1 has been revoked
-	pub data_loss_protect: OptionalField<DataLossProtect>,
+	/// Proof that the sender knows the per-commitment secret of a specific commitment transaction
+	/// belonging to the recipient
+	pub your_last_per_commitment_secret: [u8; 32],
+	/// The sender's per-commitment point for their current commitment transaction
+	pub my_current_per_commitment_point: PublicKey,
 }
 
 /// An [`announcement_signatures`] message to be sent to or received from a peer.
@@ -1362,42 +1351,13 @@ impl_writeable_msg!(AnnouncementSignatures, {
 	bitcoin_signature
 }, {});
 
-impl Writeable for ChannelReestablish {
-	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		self.channel_id.write(w)?;
-		self.next_local_commitment_number.write(w)?;
-		self.next_remote_commitment_number.write(w)?;
-		match self.data_loss_protect {
-			OptionalField::Present(ref data_loss_protect) => {
-				(*data_loss_protect).your_last_per_commitment_secret.write(w)?;
-				(*data_loss_protect).my_current_per_commitment_point.write(w)?;
-			},
-			OptionalField::Absent => {}
-		}
-		Ok(())
-	}
-}
-
-impl Readable for ChannelReestablish{
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		Ok(Self {
-			channel_id: Readable::read(r)?,
-			next_local_commitment_number: Readable::read(r)?,
-			next_remote_commitment_number: Readable::read(r)?,
-			data_loss_protect: {
-				match <[u8; 32] as Readable>::read(r) {
-					Ok(your_last_per_commitment_secret) =>
-						OptionalField::Present(DataLossProtect {
-							your_last_per_commitment_secret,
-							my_current_per_commitment_point: Readable::read(r)?,
-						}),
-					Err(DecodeError::ShortRead) => OptionalField::Absent,
-					Err(e) => return Err(e)
-				}
-			}
-		})
-	}
-}
+impl_writeable_msg!(ChannelReestablish, {
+	channel_id,
+	next_local_commitment_number,
+	next_remote_commitment_number,
+	your_last_per_commitment_secret,
+	my_current_per_commitment_point,
+}, {});
 
 impl_writeable_msg!(ClosingSigned,
 	{ channel_id, fee_satoshis, signature },
@@ -2162,23 +2122,7 @@ mod tests {
 	use core::convert::TryFrom;
 
 	#[test]
-	fn encoding_channel_reestablish_no_secret() {
-		let cr = msgs::ChannelReestablish {
-			channel_id: [4, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0],
-			next_local_commitment_number: 3,
-			next_remote_commitment_number: 4,
-			data_loss_protect: OptionalField::Absent,
-		};
-
-		let encoded_value = cr.encode();
-		assert_eq!(
-			encoded_value,
-			vec![4, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 4]
-		);
-	}
-
-	#[test]
-	fn encoding_channel_reestablish_with_secret() {
+	fn encoding_channel_reestablish() {
 		let public_key = {
 			let secp_ctx = Secp256k1::new();
 			PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&hex::decode("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap())
@@ -2188,7 +2132,8 @@ mod tests {
 			channel_id: [4, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0],
 			next_local_commitment_number: 3,
 			next_remote_commitment_number: 4,
-			data_loss_protect: OptionalField::Present(msgs::DataLossProtect { your_last_per_commitment_secret: [9;32], my_current_per_commitment_point: public_key}),
+			your_last_per_commitment_secret: [9;32],
+			my_current_per_commitment_point: public_key,
 		};
 
 		let encoded_value = cr.encode();