Skip to content

Commit

Permalink
Make DataLossProtect fields required and remove wrappers
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dunxen
dunxen committed May 1, 2023
1 parent 0e8da58 commit 16d0f2f
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 119 deletions.
78 changes: 29 additions & 49 deletions lightning/src/ln/channel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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.");
}
}

Expand Down Expand Up @@ -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(),
Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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]
Expand Down
85 changes: 15 additions & 70 deletions lightning/src/ln/msgs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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())
Expand All @@ -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();
Expand Down

0 comments on commit 16d0f2f

Please sign in to comment.