Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions ci/ci-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,50 @@ cargo test -p lightning --verbose --color always --features dnssec
cargo check -p lightning --verbose --color always --features dnssec
cargo doc -p lightning --document-private-items --features dnssec

echo -e "\n\nChecking and testing lightning with safe_channels"
cargo test -p lightning --verbose --color always --features safe_channels -- \
--skip channel_holding_cell_serialize \
--skip test_blocked_chan_preimage_release \
--skip test_durable_preimages_on_closed_channel \
--skip test_inbound_reload_without_init_mon \
--skip test_inverted_mon_completion_order \
--skip test_outbound_reload_without_init_mon \
--skip test_partial_claim_mon_update_compl_actions \
--skip test_reload_mon_update_completion_actions \
--skip test_multi_post_event_actions \
--skip test_anchors_aggregated_revoked_htlc_tx \
--skip test_anchors_monitor_fixes_counterparty_payment_script_on_reload \
--skip test_claim_event_never_handled \
--skip test_lost_timeout_monitor_events \
--skip no_double_pay_with_stale_channelmanager \
--skip test_onion_failure_stale_channel_update \
--skip no_missing_sent_on_midpoint_reload \
--skip no_missing_sent_on_reload \
--skip retry_with_no_persist \
--skip test_completed_payment_not_retryable_on_reload \
--skip test_fulfill_restart_failure \
--skip test_payment_metadata_consistency \
--skip test_priv_forwarding_rejection \
--skip test_quiescence_termination_on_disconnect \
--skip forwarded_payment_no_manager_persistence \
--skip intercepted_payment_no_manager_persistence \
--skip removed_payment_no_manager_persistence \
--skip test_data_loss_protect \
--skip test_htlc_localremoved_persistence \
--skip test_manager_serialize_deserialize_events \
--skip test_manager_serialize_deserialize_inconsistent_monitor \
--skip test_no_txn_manager_serialize_deserialize \
--skip test_partial_claim_before_restart \
--skip test_reload_partial_funding_batch \
--skip test_unconf_chan \
--skip test_unconf_chan_via_funding_unconfirmed \
--skip test_unconf_chan_via_listen \
--skip test_propose_splice_while_disconnected \
--skip test_splice_reestablish \
--skip test_splice_state_reset_on_disconnect
cargo check -p lightning --verbose --color always --features safe_channels
cargo doc -p lightning --document-private-items --features safe_channels

echo -e "\n\nChecking and testing Block Sync Clients with features"

cargo test -p lightning-block-sync --verbose --color always --features rest-client
Expand Down
1 change: 1 addition & 0 deletions lightning/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ _externalize_tests = ["inventory", "_test_utils"]
# Allow signing of local transactions that may have been revoked or will be revoked, for functional testing (e.g. justice tx handling).
# This is unsafe to use in production because it may result in the counterparty publishing taking our funds.
unsafe_revoked_tx_signing = []
safe_channels = []

std = []

Expand Down
170 changes: 169 additions & 1 deletion lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ use crate::ln::chan_utils::{
self, ChannelTransactionParameters, CommitmentTransaction, CounterpartyCommitmentSecrets,
HTLCClaim, HTLCOutputInCommitment, HolderCommitmentTransaction,
};
#[cfg(feature = "safe_channels")]
use crate::ln::channel::read_check_data;
use crate::ln::channel::INITIAL_COMMITMENT_NUMBER;
use crate::ln::channel_keys::{
DelayedPaymentBasepoint, DelayedPaymentKey, HtlcBasepoint, HtlcKey, RevocationBasepoint,
Expand Down Expand Up @@ -111,6 +113,10 @@ pub struct ChannelMonitorUpdate {
/// Will be `None` for `ChannelMonitorUpdate`s constructed on LDK versions prior to 0.0.121 and
/// always `Some` otherwise.
pub channel_id: Option<ChannelId>,

/// The encoded channel data associated with this ChannelMonitor, if any.
#[cfg(feature = "safe_channels")]
pub encoded_channel: Option<Vec<u8>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you explore encoding the FundedChannel itself rather than a bag of bytes? I guess it would require propagating the signer trait parameter, which is kind of annoying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it, but indeed, the signer parameter is annoying. But your comment did make me think again. I will try and see what we can do with a copy of FundedChannel just for data storage. Perhaps that's useful too once we start stripping out the redundant fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed while trying this is that ChannelContext contains blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>. PendingChannelMonitorUpdate will contain FundedChannel snapshots. Nesting 😨

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

impl ChannelMonitorUpdate {
Expand Down Expand Up @@ -156,9 +162,16 @@ impl Writeable for ChannelMonitorUpdate {
for update_step in self.updates.iter() {
update_step.write(w)?;
}
#[cfg(not(feature = "safe_channels"))]
write_tlv_fields!(w, {
// 1 was previously used to store `counterparty_node_id`
(3, self.channel_id, option),
});
#[cfg(feature = "safe_channels")]
write_tlv_fields!(w, {
// 1 was previously used to store `counterparty_node_id`
(3, self.channel_id, option),
(5, self.encoded_channel, option)
});
Ok(())
}
Expand All @@ -176,11 +189,24 @@ impl Readable for ChannelMonitorUpdate {
}
}
let mut channel_id = None;
#[cfg(not(feature = "safe_channels"))]
read_tlv_fields!(r, {
// 1 was previously used to store `counterparty_node_id`
(3, channel_id, option),
});
Ok(Self { update_id, updates, channel_id })
#[cfg(feature = "safe_channels")]
let mut encoded_channel = None;
#[cfg(feature = "safe_channels")]
read_tlv_fields!(r, {
// 1 was previously used to store `counterparty_node_id`
(3, channel_id, option),
(5, encoded_channel, option)
});
Ok(Self {
update_id, updates, channel_id,
#[cfg(feature = "safe_channels")]
encoded_channel
})
}
}

Expand Down Expand Up @@ -1402,6 +1428,11 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
/// make deciding whether to do so simple, here we track whether this monitor was last written
/// prior to 0.1.
written_by_0_1_or_later: bool,

/// The serialized channel state as provided via the last `ChannelMonitorUpdate` or via a call to
/// [`ChannelMonitor::update_encoded_channel`].
Comment on lines +1432 to +1433
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe note that this is a FundedChannel and explain why we're encoding it here?

#[cfg(feature = "safe_channels")]
encoded_channel: Option<Vec<u8>>,
}

// Returns a `&FundingScope` for the one we are currently observing/handling commitment transactions
Expand Down Expand Up @@ -1521,6 +1552,12 @@ const MIN_SERIALIZATION_VERSION: u8 = 1;
pub(crate) fn write_chanmon_internal<Signer: EcdsaChannelSigner, W: Writer>(
channel_monitor: &ChannelMonitorImpl<Signer>, _is_stub: bool, writer: &mut W,
) -> Result<(), Error> {
// Check that the encoded channel (if present) is consistent with the rest of the monitor. This sets an invariant
// for the safe_channels feature.
#[cfg(feature = "safe_channels")]
if let Some(ref encoded_channel) = channel_monitor.encoded_channel {
channel_monitor.check_encoded_channel_consistency(encoded_channel);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be better to have the above docs (or duplicate them) on the method itself

}
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

channel_monitor.latest_update_id.write(writer)?;
Expand Down Expand Up @@ -1733,6 +1770,7 @@ pub(crate) fn write_chanmon_internal<Signer: EcdsaChannelSigner, W: Writer>(
_ => channel_monitor.pending_monitor_events.clone(),
};

#[cfg(not(feature = "safe_channels"))]
write_tlv_fields!(writer, {
(1, channel_monitor.funding_spend_confirmed, option),
(3, channel_monitor.htlcs_resolved_on_chain, required_vec),
Expand All @@ -1757,6 +1795,32 @@ pub(crate) fn write_chanmon_internal<Signer: EcdsaChannelSigner, W: Writer>(
(37, channel_monitor.funding_seen_onchain, required),
});

#[cfg(feature = "safe_channels")]
write_tlv_fields!(writer, {
(1, channel_monitor.funding_spend_confirmed, option),
(3, channel_monitor.htlcs_resolved_on_chain, required_vec),
(5, pending_monitor_events, required_vec),
(7, channel_monitor.funding_spend_seen, required),
(9, channel_monitor.counterparty_node_id, required),
(11, channel_monitor.confirmed_commitment_tx_counterparty_output, option),
(13, channel_monitor.spendable_txids_confirmed, required_vec),
(15, channel_monitor.counterparty_fulfilled_htlcs, required),
(17, channel_monitor.initial_counterparty_commitment_info, option),
(19, channel_monitor.channel_id, required),
(21, channel_monitor.balances_empty_height, option),
(23, channel_monitor.holder_pays_commitment_tx_fee, option),
(25, channel_monitor.payment_preimages, required),
(27, channel_monitor.first_negotiated_funding_txo, required),
(29, channel_monitor.initial_counterparty_commitment_tx, option),
(31, channel_monitor.funding.channel_parameters, required),
(32, channel_monitor.pending_funding, optional_vec),
(33, channel_monitor.htlcs_resolved_to_user, required),
(34, channel_monitor.alternative_funding_confirmed, option),
(35, channel_monitor.is_manual_broadcast, required),
(37, channel_monitor.funding_seen_onchain, required),
(39, channel_monitor.encoded_channel, option),
});

Ok(())
}

Expand Down Expand Up @@ -1994,6 +2058,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
alternative_funding_confirmed: None,

written_by_0_1_or_later: true,
#[cfg(feature = "safe_channels")]
encoded_channel: None,
})
}

Expand Down Expand Up @@ -2114,6 +2180,19 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
inner.update_monitor(updates, broadcaster, fee_estimator, &logger)
}

/// Gets the encoded channel data, if any, associated with this ChannelMonitor.
#[cfg(feature = "safe_channels")]
pub fn get_encoded_channel(&self) -> Option<Vec<u8>> {
self.inner.lock().unwrap().encoded_channel.clone()
}

/// Updates the encoded channel data associated with this ChannelMonitor. To clear the encoded channel data (for
/// example after shut down of a channel), pass an empty vector.
#[cfg(feature = "safe_channels")]
pub fn update_encoded_channel(&self, encoded: Vec<u8>) {
self.inner.lock().unwrap().update_encoded_channel(encoded);
}

/// Gets the update_id from the latest ChannelMonitorUpdate which was applied to this
/// ChannelMonitor.
///
Expand Down Expand Up @@ -2310,14 +2389,17 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
self.inner.lock().unwrap().sign_to_local_justice_tx(justice_tx, input_idx, value, commitment_number)
}

#[cfg(not(feature = "safe_channels"))]
pub(crate) fn get_min_seen_secret(&self) -> u64 {
self.inner.lock().unwrap().get_min_seen_secret()
}

#[cfg(not(feature = "safe_channels"))]
pub(crate) fn get_cur_counterparty_commitment_number(&self) -> u64 {
self.inner.lock().unwrap().get_cur_counterparty_commitment_number()
}

#[cfg(not(feature = "safe_channels"))]
pub(crate) fn get_cur_holder_commitment_number(&self) -> u64 {
self.inner.lock().unwrap().get_cur_holder_commitment_number()
}
Expand Down Expand Up @@ -2719,6 +2801,55 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
}

impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
#[cfg(feature = "safe_channels")]
fn check_encoded_channel_consistency(&self, encoded: &[u8]) {
let encoded_channel_reader = &mut &encoded[..];
let check_res = read_check_data(encoded_channel_reader);
if let Ok(check_data) = check_res {
debug_assert!(
check_data.cur_holder_commitment_transaction_number
<= self.get_cur_holder_commitment_number(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document generally why it's okay for the channel to be behind the monitor (but not ahead)?

"cur_holder_commitment_transaction_number - channel: {} vs monitor: {}",
check_data.cur_holder_commitment_transaction_number,
self.get_cur_holder_commitment_number()
);
debug_assert!(
check_data.revoked_counterparty_commitment_transaction_number
<= self.get_min_seen_secret(),
"revoked_counterparty_commitment_transaction_number - channel: {} vs monitor: {}",
check_data.revoked_counterparty_commitment_transaction_number,
self.get_min_seen_secret()
);
debug_assert!(
check_data.cur_counterparty_commitment_transaction_number
<= self.get_cur_counterparty_commitment_number(),
"cur_counterparty_commitment_transaction_number - channel: {} vs monitor: {}",
check_data.cur_counterparty_commitment_transaction_number,
self.get_cur_counterparty_commitment_number()
);
debug_assert!(
check_data.latest_monitor_update_id >= self.get_latest_update_id(),
"latest_monitor_update_id - channel: {} vs monitor: {}",
check_data.latest_monitor_update_id,
self.get_latest_update_id()
);
} else {
debug_assert!(false, "Failed to read check data from encoded channel");
}
}

#[cfg(feature = "safe_channels")]
fn update_encoded_channel(&mut self, encoded: Vec<u8>) {
if encoded.len() > 0 {
// Check that the encoded channel is consistent with the rest of the monitor. This sets an invariant for the
// safe_channels feature.
self.check_encoded_channel_consistency(&encoded);
self.encoded_channel = Some(encoded);
} else {
self.encoded_channel = None;
}
}

/// Helper for get_claimable_balances which does the work for an individual HTLC, generating up
/// to one `Balance` for the HTLC.
#[rustfmt::skip]
Expand Down Expand Up @@ -4405,6 +4536,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

// Assume that if the update contains no encoded channel, that the channel remained unchanged. We
// therefore do not update the monitor.
#[cfg(feature="safe_channels")]
if let Some(encoded_channel) = updates.encoded_channel.as_ref() {
self.update_encoded_channel(encoded_channel.clone());
}

if ret.is_ok() && self.no_further_updates_allowed() && is_pre_close_update {
log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent");
Err(())
Expand Down Expand Up @@ -6644,6 +6782,33 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
let mut alternative_funding_confirmed = None;
let mut is_manual_broadcast = RequiredWrapper(None);
let mut funding_seen_onchain = RequiredWrapper(None);
#[cfg(not(feature="safe_channels"))]
read_tlv_fields!(reader, {
(1, funding_spend_confirmed, option),
(3, htlcs_resolved_on_chain, optional_vec),
(5, pending_monitor_events, optional_vec),
(7, funding_spend_seen, option),
(9, counterparty_node_id, option),
(11, confirmed_commitment_tx_counterparty_output, option),
(13, spendable_txids_confirmed, optional_vec),
(15, counterparty_fulfilled_htlcs, option),
(17, initial_counterparty_commitment_info, option),
(19, channel_id, option),
(21, balances_empty_height, option),
(23, holder_pays_commitment_tx_fee, option),
(25, payment_preimages_with_info, option),
(27, first_negotiated_funding_txo, (default_value, outpoint)),
(29, initial_counterparty_commitment_tx, option),
(31, channel_parameters, (option: ReadableArgs, None)),
(32, pending_funding, optional_vec),
(33, htlcs_resolved_to_user, option),
(34, alternative_funding_confirmed, option),
(35, is_manual_broadcast, (default_value, false)),
(37, funding_seen_onchain, (default_value, true)),
});
#[cfg(feature="safe_channels")]
let mut encoded_channel = None;
#[cfg(feature="safe_channels")]
read_tlv_fields!(reader, {
(1, funding_spend_confirmed, option),
(3, htlcs_resolved_on_chain, optional_vec),
Expand All @@ -6666,6 +6831,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
(34, alternative_funding_confirmed, option),
(35, is_manual_broadcast, (default_value, false)),
(37, funding_seen_onchain, (default_value, true)),
(39, encoded_channel, option),
});
// Note that `payment_preimages_with_info` was added (and is always written) in LDK 0.1, so
// we can use it to determine if this monitor was last written by LDK 0.1 or later.
Expand Down Expand Up @@ -6843,6 +7009,8 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
alternative_funding_confirmed,

written_by_0_1_or_later,
#[cfg(feature="safe_channels")]
encoded_channel,
});

if counterparty_node_id.is_none() {
Expand Down
Loading