Skip to content

Drop the ChannelMonitorUpdateStatus::PermanentFailure variant #2562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 21, 2023
Merged
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
4 changes: 2 additions & 2 deletions fuzz/src/chanmon_consistency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ impl TestChainMonitor {
}
}
impl chain::Watch<TestChannelSigner> for TestChainMonitor {
fn watch_channel(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor<TestChannelSigner>) -> chain::ChannelMonitorUpdateStatus {
fn watch_channel(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor<TestChannelSigner>) -> Result<chain::ChannelMonitorUpdateStatus, ()> {
let mut ser = VecWriter(Vec::new());
monitor.write(&mut ser).unwrap();
if let Some(_) = self.latest_monitors.lock().unwrap().insert(funding_txo, (monitor.get_latest_update_id(), ser.0)) {
Expand Down Expand Up @@ -500,7 +500,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
let res = (<(BlockHash, ChanMan)>::read(&mut Cursor::new(&$ser.0), read_args).expect("Failed to read manager").1, chain_monitor.clone());
for (funding_txo, mon) in monitors.drain() {
assert_eq!(chain_monitor.chain_monitor.watch_channel(funding_txo, mon),
ChannelMonitorUpdateStatus::Completed);
Ok(ChannelMonitorUpdateStatus::Completed));
}
res
} }
Expand Down
6 changes: 3 additions & 3 deletions lightning-persister/src/fs_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ mod tests {
}

// Test that if the store's path to channel data is read-only, writing a
// monitor to it results in the store returning a PermanentFailure.
// monitor to it results in the store returning an InProgress.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be UnrecoverableError now

// Windows ignores the read-only flag for folders, so this test is Unix-only.
#[cfg(not(target_os = "windows"))]
#[test]
Expand Down Expand Up @@ -470,7 +470,7 @@ mod tests {
index: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

10 lines up is a reference to permanent failure, though I suppose it's not really incorrect per se. Also in the test name.

};
match store.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
ChannelMonitorUpdateStatus::PermanentFailure => {},
ChannelMonitorUpdateStatus::UnrecoverableError => {},
_ => panic!("unexpected result from persisting new channel")
}

Expand Down Expand Up @@ -507,7 +507,7 @@ mod tests {
index: 0
};
match store.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
ChannelMonitorUpdateStatus::PermanentFailure => {},
ChannelMonitorUpdateStatus::UnrecoverableError => {},
_ => panic!("unexpected result from persisting new channel")
}

Expand Down
204 changes: 106 additions & 98 deletions lightning/src/chain/chainmonitor.rs

Large diffs are not rendered by default.

55 changes: 22 additions & 33 deletions lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ pub enum MonitorEvent {
HTLCEvent(HTLCUpdate),

/// A monitor event that the Channel's commitment transaction was confirmed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this piece of docs still correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good point, it should be changed, yes!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed at #2668

CommitmentTxConfirmed(OutPoint),
HolderForceClosed(OutPoint),

/// Indicates a [`ChannelMonitor`] update has completed. See
/// [`ChannelMonitorUpdateStatus::InProgress`] for more information on how this is used.
Expand All @@ -150,24 +150,18 @@ pub enum MonitorEvent {
/// same [`ChannelMonitor`] have been applied and persisted.
monitor_update_id: u64,
},

/// Indicates a [`ChannelMonitor`] update has failed. See
/// [`ChannelMonitorUpdateStatus::PermanentFailure`] for more information on how this is used.
///
/// [`ChannelMonitorUpdateStatus::PermanentFailure`]: super::ChannelMonitorUpdateStatus::PermanentFailure
UpdateFailed(OutPoint),
}
impl_writeable_tlv_based_enum_upgradable!(MonitorEvent,
// Note that Completed and UpdateFailed are currently never serialized to disk as they are
// generated only in ChainMonitor
// Note that Completed is currently never serialized to disk as it is generated only in
// ChainMonitor.
(0, Completed) => {
(0, funding_txo, required),
(2, monitor_update_id, required),
},
;
(2, HTLCEvent),
(4, CommitmentTxConfirmed),
(6, UpdateFailed),
(4, HolderForceClosed),
// 6 was `UpdateFailed` until LDK 0.0.117
);

/// Simple structure sent back by `chain::Watch` when an HTLC from a forward channel is detected on
Expand Down Expand Up @@ -1037,7 +1031,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe

writer.write_all(&(self.pending_monitor_events.iter().filter(|ev| match ev {
MonitorEvent::HTLCEvent(_) => true,
MonitorEvent::CommitmentTxConfirmed(_) => true,
MonitorEvent::HolderForceClosed(_) => true,
_ => false,
}).count() as u64).to_be_bytes())?;
for event in self.pending_monitor_events.iter() {
Expand All @@ -1046,7 +1040,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe
0u8.write(writer)?;
upd.write(writer)?;
},
MonitorEvent::CommitmentTxConfirmed(_) => 1u8.write(writer)?,
MonitorEvent::HolderForceClosed(_) => 1u8.write(writer)?,
_ => {}, // Covered in the TLV writes below
}
}
Expand Down Expand Up @@ -1488,21 +1482,20 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
self.inner.lock().unwrap().counterparty_node_id
}

/// Used by ChannelManager deserialization to broadcast the latest holder state if its copy of
/// the Channel was out-of-date.
/// Used by [`ChannelManager`] deserialization to broadcast the latest holder state if its copy
/// of the channel state was out-of-date.
///
/// You may also use this to broadcast the latest local commitment transaction, either because
/// a monitor update failed with [`ChannelMonitorUpdateStatus::PermanentFailure`] or because we've
/// fallen behind (i.e. we've received proof that our counterparty side knows a revocation
/// secret we gave them that they shouldn't know).
/// a monitor update failed or because we've fallen behind (i.e. we've received proof that our
Copy link
Contributor

Choose a reason for hiding this comment

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

Mh, remind me how exactly we know would know that a monitor update failed for good at this point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We know when we've fallen behind thanks to the your_last_per_commitment_secret field in ChannelReestablish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mh, but assuming we now might get InProgess it still is kinda unclear for a user how/when to act exactly on what. The updated version reads a bit like "you may use this if X and Y happens, but we don't really tell you if they happened. Ah, btw., this might be unsafe. Good luck.". Do we at least need to mention UnrecoverableError here?

We know when we've fallen behind thanks to the your_last_per_commitment_secret field in ChannelReestablish.

Maybe I'm overlooking something, but access to that isn't exposed somewhere in the API really?

/// counterparty side knows a revocation secret we gave them that they shouldn't know).
///
/// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty
/// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't
/// close channel with their commitment transaction after a substantial amount of time. Best
/// may be to contact the other node operator out-of-band to coordinate other options available
/// to you. In any-case, the choice is up to you.
/// to you.
///
/// [`ChannelMonitorUpdateStatus::PermanentFailure`]: super::ChannelMonitorUpdateStatus::PermanentFailure
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
where L::Target: Logger {
self.inner.lock().unwrap().get_latest_holder_commitment_txn(logger)
Expand Down Expand Up @@ -2533,7 +2526,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
txs.push(tx);
}
broadcaster.broadcast_transactions(&txs);
self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
}

pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()>
Expand Down Expand Up @@ -2599,6 +2592,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => {
log_trace!(logger, "Updating ChannelMonitor with commitment secret");
if let Err(e) = self.provide_secret(*idx, *secret) {
debug_assert!(false, "Latest counterparty commitment secret was invalid");
log_error!(logger, "Providing latest counterparty commitment secret failed/was refused:");
log_error!(logger, " {}", e);
ret = Err(());
Expand Down Expand Up @@ -2645,7 +2639,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
log_error!(logger, " in channel monitor for channel {}!", &self.funding_info.0.to_channel_id());
log_error!(logger, " Read the docs for ChannelMonitor::get_latest_holder_commitment_txn and take manual action!");
} else {
// If we generated a MonitorEvent::CommitmentTxConfirmed, the ChannelManager
// If we generated a MonitorEvent::HolderForceClosed, the ChannelManager
// will still give us a ChannelForceClosed event with !should_broadcast, but we
// shouldn't print the scary warning above.
log_info!(logger, "Channel off-chain state closed after we broadcasted our latest commitment transaction.");
Expand Down Expand Up @@ -3490,7 +3484,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone(), self.channel_value_satoshis, self.onchain_tx_handler.channel_type_features().clone());
let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), self.best_block.height(), self.best_block.height());
claimable_outpoints.push(commitment_package);
self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
// Although we aren't signing the transaction directly here, the transaction will be signed
// in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject
// new channel updates.
Expand Down Expand Up @@ -4254,7 +4248,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
for _ in 0..pending_monitor_events_len {
let ev = match <u8 as Readable>::read(reader)? {
0 => MonitorEvent::HTLCEvent(Readable::read(reader)?),
1 => MonitorEvent::CommitmentTxConfirmed(funding_info.0),
1 => MonitorEvent::HolderForceClosed(funding_info.0),
_ => return Err(DecodeError::InvalidValue)
};
pending_monitor_events.as_mut().unwrap().push(ev);
Expand Down Expand Up @@ -4413,13 +4407,12 @@ mod tests {
use crate::chain::chaininterface::LowerBoundedFeeEstimator;

use super::ChannelMonitorUpdateStep;
use crate::{check_added_monitors, check_closed_broadcast, check_closed_event, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash, unwrap_send_err};
use crate::{check_added_monitors, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash, unwrap_send_err};
use crate::chain::{BestBlock, Confirm};
use crate::chain::channelmonitor::ChannelMonitor;
use crate::chain::package::{weight_offered_htlc, weight_received_htlc, weight_revoked_offered_htlc, weight_revoked_received_htlc, WEIGHT_REVOKED_OUTPUT};
use crate::chain::transaction::OutPoint;
use crate::sign::InMemorySigner;
use crate::events::ClosureReason;
use crate::ln::{PaymentPreimage, PaymentHash};
use crate::ln::chan_utils;
use crate::ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
Expand Down Expand Up @@ -4485,18 +4478,14 @@ mod tests {
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 100_000);
unwrap_send_err!(nodes[1].node.send_payment_with_route(&route, payment_hash,
RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
), true, APIError::ChannelUnavailable { ref err },
assert!(err.contains("ChannelMonitor storage failure")));
check_added_monitors!(nodes[1], 2); // After the failure we generate a close-channel monitor update
check_closed_broadcast!(nodes[1], true);
check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: "ChannelMonitor storage failure".to_string() },
[nodes[0].node.get_our_node_id()], 100000);
), false, APIError::MonitorUpdateInProgress, {});
check_added_monitors!(nodes[1], 1);

// Build a new ChannelMonitorUpdate which contains both the failing commitment tx update
// and provides the claim preimages for the two pending HTLCs. The first update generates
// an error, but the point of this test is to ensure the later updates are still applied.
let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().rev().skip(1).next().unwrap().clone();
let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().rev().next().unwrap().clone();
assert_eq!(replay_update.updates.len(), 1);
if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
} else { panic!(); }
Expand Down
Loading