Skip to content
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
47 changes: 47 additions & 0 deletions lightning/src/chain/chainmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,53 @@ where

self.pending_send_only_events.lock().unwrap().push(send_peer_storage_event)
}

/// Loads a [`ChannelMonitor`] which already exists on disk after startup.
///
/// Using this over [`chain::Watch::watch_channel`] avoids re-persisting a [`ChannelMonitor`]
/// that hasn't changed, slowing down startup.
///
/// Note that this method *can* be used if additional blocks were replayed against the
Copy link
Contributor

Choose a reason for hiding this comment

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

Understanding the assumptions and restrictions of this API is still relatively hard. In particular, how can users be sure which of the APIs on ChannelMonitor are safe to use and which ones change(d) its state in some way that will not be replayed again on a restart.?

And, say users currently just use safe APIs or don't access ChannelMonitor, start using load_existing_monitor, and then some time down the road decide they need to mess with ChannelMonitors for some reason. Do we expect them to remember the fineprint of this API?

IMO, it would be still very much preferable to end up with a single API method for loading monitors that does the optimal thing under the hood, depending on whether the monitor state is 'dirty' or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I agree in principle, but we also can't perfectly know - someone can in theory apply a ChannelMonitorUpdate that won't be replayed on startup. But, ultimately, I think this will work in 99.99% of cases - a user deciding to do a Thing during their startup routing and then deciding not to it again if they hit a crash and restart seems kinda wild, unless they have a bug and don't persist but do delete the instructions to do the Thing.

Im open to ideas for how to better phrase this, I added a quick "basically this should never happen" when squasing tho:

$ git diff-tree -U5 d79e08f86 cfca2dc97
diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index 951aad7f78..58a2dbcdb3 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -833,11 +833,11 @@ where
 	///
 	/// Note that this method *can* be used if additional blocks were replayed against the
 	/// [`ChannelMonitor`] or if a [`ChannelMonitorUpdate`] loaded from disk was replayed such that
 	/// it will replayed on startup, and in general can only *not* be used if you directly accessed
 	/// the [`ChannelMonitor`] and changed its state in some way that will not be replayed again on
-	/// a restart.
+	/// a restart. Such direct access should generally never occur for most LDK-based nodes.
 	///
 	/// For [`ChannelMonitor`]s which were last serialized by an LDK version prior to 0.1 this will
 	/// fall back to calling [`chain::Watch::watch_channel`] and persisting the [`ChannelMonitor`].
 	/// See the release notes for LDK 0.1 for more information on this requirement.
 	///

Copy link
Contributor

Choose a reason for hiding this comment

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

With ldk-node more and more being the predominant way to run this code, we can still ensure that it is safe in most cases.

/// [`ChannelMonitor`] or if a [`ChannelMonitorUpdate`] loaded from disk was replayed such that
/// it will replayed on startup, and in general can only *not* be used if you directly accessed
/// the [`ChannelMonitor`] and changed its state in some way that will not be replayed again on
/// a restart. Such direct access should generally never occur for most LDK-based nodes.
///
/// For [`ChannelMonitor`]s which were last serialized by an LDK version prior to 0.1 this will
/// fall back to calling [`chain::Watch::watch_channel`] and persisting the [`ChannelMonitor`].
/// See the release notes for LDK 0.1 for more information on this requirement.
///
/// [`ChannelMonitor`]s which do not need to be persisted (i.e. were last written by LDK 0.1 or
/// later) will be loaded without persistence and this method will return
/// [`ChannelMonitorUpdateStatus::Completed`].
pub fn load_existing_monitor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure: for ldk-node, the idea is that it will call into this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I anticipate nearly every implementation will call this instead.

&self, channel_id: ChannelId, monitor: ChannelMonitor<ChannelSigner>,
) -> Result<ChannelMonitorUpdateStatus, ()> {
if !monitor.written_by_0_1_or_later() {
return chain::Watch::watch_channel(self, channel_id, monitor);
}

let logger = WithChannelMonitor::from(&self.logger, &monitor, None);
let mut monitors = self.monitors.write().unwrap();
let entry = match monitors.entry(channel_id) {
hash_map::Entry::Occupied(_) => {
log_error!(logger, "Failed to add new channel data: channel monitor for given channel ID is already present");
return Err(());
},
hash_map::Entry::Vacant(e) => e,
};
log_trace!(
logger,
"Loaded existing ChannelMonitor for channel {}",
log_funding_info!(monitor)
);
if let Some(ref chain_source) = self.chain_source {
monitor.load_outputs_to_watch(chain_source, &self.logger);
}
entry.insert(MonitorHolder { monitor, pending_monitor_updates: Mutex::new(Vec::new()) });

Ok(ChannelMonitorUpdateStatus::Completed)
}
}

impl<
Expand Down
16 changes: 16 additions & 0 deletions lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1300,6 +1300,11 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
// found at `Self::funding`. We don't use the term "renegotiated", as the currently locked
// `FundingScope` could be one that was renegotiated.
alternative_funding_confirmed: Option<(Txid, u32)>,

/// [`ChannelMonitor`]s written by LDK prior to 0.1 need to be re-persisted after startup. To
/// 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,
}

// Macro helper to access holder commitment HTLC data (including both non-dust and dust) while
Expand Down Expand Up @@ -1803,6 +1808,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
prev_holder_htlc_data: None,

alternative_funding_confirmed: None,

written_by_0_1_or_later: true,
})
}

Expand Down Expand Up @@ -1936,6 +1943,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
self.inner.lock().unwrap().get_funding_txo()
}

pub(crate) fn written_by_0_1_or_later(&self) -> bool {
self.inner.lock().unwrap().written_by_0_1_or_later
}

/// Gets the funding script of the channel this ChannelMonitor is monitoring for.
pub fn get_funding_script(&self) -> ScriptBuf {
self.inner.lock().unwrap().get_funding_script()
Expand Down Expand Up @@ -6080,6 +6091,9 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
(32, pending_funding, optional_vec),
(34, alternative_funding_confirmed, 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.
let written_by_0_1_or_later = payment_preimages_with_info.is_some();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment why this determines the version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought it was pretty self-explanitory given the code, but, sure :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't to me. payment_preimage_with_info was also added in 0.1 I assume?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, answered in diff. Ty.

if let Some(payment_preimages_with_info) = payment_preimages_with_info {
if payment_preimages_with_info.len() != payment_preimages.len() {
return Err(DecodeError::InvalidValue);
Expand Down Expand Up @@ -6250,6 +6264,8 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
prev_holder_htlc_data,

alternative_funding_confirmed,

written_by_0_1_or_later,
})))
}
}
Expand Down
5 changes: 5 additions & 0 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15396,9 +15396,13 @@ impl Readable for VecDeque<(Event, Option<EventCompletionAction>)> {
/// This is important if you have replayed a nontrivial number of blocks in step (4), allowing
/// you to avoid having to replay the same blocks if you shut down quickly after startup. It is
/// otherwise not required.
///
/// Note that if you're using a [`ChainMonitor`] for your [`chain::Watch`] implementation, you
/// will likely accomplish this as a side-effect of calling [`chain::Watch::watch_channel`] in
/// the next step.
///
/// If you wish to avoid this for performance reasons, use
/// [`ChainMonitor::load_existing_monitor`].
/// 7) Move the [`ChannelMonitor`]s into your local [`chain::Watch`]. If you're using a
/// [`ChainMonitor`], this is done by calling [`chain::Watch::watch_channel`].
///
Expand All @@ -15413,6 +15417,7 @@ impl Readable for VecDeque<(Event, Option<EventCompletionAction>)> {
/// which you've already broadcasted the transaction.
///
/// [`ChainMonitor`]: crate::chain::chainmonitor::ChainMonitor
/// [`ChainMonitor::load_existing_monitor`]: crate::chain::chainmonitor::ChainMonitor::load_existing_monitor
pub struct ChannelManagerReadArgs<
'a,
M: Deref,
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1324,8 +1324,8 @@ pub fn _reload_node<'a, 'b, 'c>(
for monitor in monitors_read.drain(..) {
let channel_id = monitor.channel_id();
assert_eq!(
node.chain_monitor.watch_channel(channel_id, monitor),
Ok(ChannelMonitorUpdateStatus::Completed)
node.chain_monitor.load_existing_monitor(channel_id, monitor),
Ok(ChannelMonitorUpdateStatus::Completed),
);
check_added_monitors!(node, 1);
}
Expand Down
29 changes: 29 additions & 0 deletions lightning/src/util/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,35 @@ impl<'a> TestChainMonitor<'a> {
self.latest_monitor_update_id.lock().unwrap().get(channel_id).unwrap().clone();
self.chain_monitor.channel_monitor_updated(*channel_id, latest_update).unwrap();
}

pub fn load_existing_monitor(
&self, channel_id: ChannelId, monitor: ChannelMonitor<TestChannelSigner>,
) -> Result<chain::ChannelMonitorUpdateStatus, ()> {
#[cfg(feature = "std")]
if let Some(blocker) = &*self.write_blocker.lock().unwrap() {
blocker.recv().unwrap();
}

// At every point where we get a monitor update, we should be able to send a useful monitor
// to a watchtower and disk...
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems out of place? Or maybe I don't understand the purpose of this fn. What I see is serialize the monitor, deserialize it and then load.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let mut w = TestVecWriter(Vec::new());
monitor.write(&mut w).unwrap();
let new_monitor = <(BlockHash, ChannelMonitor<TestChannelSigner>)>::read(
&mut io::Cursor::new(&w.0),
(self.keys_manager, self.keys_manager),
)
.unwrap()
.1;
// Note that a ChannelMonitor might not round-trip exactly here as we have tests that were
// serialized prior to LDK 0.1 and re-serializing them will flip the "written after LDK
// 0.1" flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the consequence of this, or the intention of this message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

self.latest_monitor_update_id
.lock()
.unwrap()
.insert(channel_id, (monitor.get_latest_update_id(), monitor.get_latest_update_id()));
self.added_monitors.lock().unwrap().push((channel_id, monitor));
self.chain_monitor.load_existing_monitor(channel_id, new_monitor)
}
}
impl<'a> chain::Watch<TestChannelSigner> for TestChainMonitor<'a> {
fn watch_channel(
Expand Down
Loading