From 16d0f2ff4a4dd968f30e1fbbf938a332267ddcf2 Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Mon, 1 May 2023 22:23:20 +0200 Subject: [PATCH 1/3] Make `DataLossProtect` fields required and remove wrappers The fields provided by `DataLossProtect` have been mandatory since https://github.com/lightning/bolts/pull/754/commits/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. --- lightning/src/ln/channel.rs | 78 +++++++++++++--------------------- lightning/src/ln/msgs.rs | 85 +++++++------------------------------ 2 files changed, 44 insertions(+), 119 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8b5e89d9441..040129aab8f 100644 --- a/lightning/src/ln/channel.rs +++ b/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 Channel { } 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 Channel { // 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 Channel { // 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,12 +7307,7 @@ 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. @@ -7330,12 +7315,7 @@ mod tests { 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] diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 4b2eb9674fa..779cf152825 100644 --- a/lightning/src/ln/msgs.rs +++ b/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, + /// 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(&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: &mut R) -> Result { - 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(); From 20cd856aa5d2834a557663ea67ee0139ad522586 Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Mon, 1 May 2023 22:52:30 +0200 Subject: [PATCH 2/3] Remove `OptionalField` and move `shutdown_scriptpubkey` into TLV stream As pointed out in https://github.com/lightning/bolts/pull/754/commits/6656b70, we can move the `shutdown_scriptpubkey` field into the TLV streams of `OpenChannel` and `AcceptChannel` without affecting the resulting encoding. We use `WithoutLength` encoding here to ensure that we do not encode a length prefix along with `Script` as is normally the case. --- lightning/src/ln/channel.rs | 14 +++--- lightning/src/ln/msgs.rs | 80 ++++-------------------------- lightning/src/ln/shutdown_tests.rs | 7 ++- lightning/src/util/ser.rs | 17 ++++++- lightning/src/util/ser_macros.rs | 5 +- 5 files changed, 40 insertions(+), 83 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 040129aab8f..7778b92025d 100644 --- a/lightning/src/ln/channel.rs +++ b/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}; +use crate::ln::msgs::DecodeError; 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}; @@ -1314,7 +1314,7 @@ impl Channel { let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() { match &msg.shutdown_scriptpubkey { - &OptionalField::Present(ref script) => { + &Some(ref script) => { // Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything if script.len() == 0 { None @@ -1326,7 +1326,7 @@ impl Channel { } }, // Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel - &OptionalField::Absent => { + &None => { return Err(ChannelError::Close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned())); } } @@ -2191,7 +2191,7 @@ impl Channel { let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() { match &msg.shutdown_scriptpubkey { - &OptionalField::Present(ref script) => { + &Some(ref script) => { // Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything if script.len() == 0 { None @@ -2203,7 +2203,7 @@ impl Channel { } }, // Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel - &OptionalField::Absent => { + &None => { return Err(ChannelError::Close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned())); } } @@ -5318,7 +5318,7 @@ impl Channel { htlc_basepoint: keys.htlc_basepoint, first_per_commitment_point, channel_flags: if self.config.announced_channel {1} else {0}, - shutdown_scriptpubkey: OptionalField::Present(match &self.shutdown_scriptpubkey { + shutdown_scriptpubkey: Some(match &self.shutdown_scriptpubkey { Some(script) => script.clone().into_inner(), None => Builder::new().into_script(), }), @@ -5384,7 +5384,7 @@ impl Channel { delayed_payment_basepoint: keys.delayed_payment_basepoint, htlc_basepoint: keys.htlc_basepoint, first_per_commitment_point, - shutdown_scriptpubkey: OptionalField::Present(match &self.shutdown_scriptpubkey { + shutdown_scriptpubkey: Some(match &self.shutdown_scriptpubkey { Some(script) => script.clone().into_inner(), None => Builder::new().into_script(), }), diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 779cf152825..df6a2aba3b9 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -199,8 +199,8 @@ pub struct OpenChannel { pub first_per_commitment_point: PublicKey, /// The channel flags to be used pub channel_flags: u8, - /// Optionally, a request to pre-set the to-sender output's `scriptPubkey` for when we collaboratively close - pub shutdown_scriptpubkey: OptionalField