Skip to content

Doc and comment followups to #2562 #2591

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 5 commits into from
Sep 29, 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 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 an InProgress.
// monitor to it results in the store returning an UnrecoverableError.
// Windows ignores the read-only flag for folders, so this test is Unix-only.
#[cfg(not(target_os = "windows"))]
#[test]
Expand All @@ -458,7 +458,7 @@ mod tests {
let update_id = update_map.get(&added_monitors[0].0.to_channel_id()).unwrap();

// Set the store's directory to read-only, which should result in
// returning a permanent failure when we then attempt to persist a
// returning an unrecoverable failure when we then attempt to persist a
// channel update.
let path = &store.get_data_dir();
let mut perms = fs::metadata(path).unwrap().permissions();
Expand Down
7 changes: 5 additions & 2 deletions lightning/src/chain/chainmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl MonitorUpdateId {
/// If at some point no further progress can be made towards persisting the pending updates, the
/// node should simply shut down.
///
/// * If the persistence has failed and cannot be retried further (e.g. because of some timeout),
/// * If the persistence has failed and cannot be retried further (e.g. because of an outage),
/// [`ChannelMonitorUpdateStatus::UnrecoverableError`] can be used, though this will result in
/// an immediate panic and future operations in LDK generally failing.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is some inherent gap about how clients should or will implement async persist.

Imo, they might just start a thread or future, on completion just call channel_monitor_updated. (other method is queued messages)
In case of thread/future, if there is a failure inside of that, i don't expect them to be implementing "infinite retries in thread" for each InProgress monitor_update individually. (Whereas current documentation for async-persist seems to assume that)

Hence #2562 (comment)

Let me know if i misunderstand something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For a future, I think they absolutely will retry infinitely in the task - there's very little overhead for the task and there's no reason to move to batching it. If they're doing a full OS thread, I agree, probably doesn't make sense to spawn a whole thread rather hopefully you simply notify an existing background thread. Still, I'm not quite sure what you're suggesting I change here - can you provide a concrete suggested phrasing change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I didn't feel it is alright to have many infinite retrying resources in system (that could be anything) if there is an outage or db failure. (But its ok, whatever it is.)

For Asynchronous persistence case,
We need to mention Implementations should retry all pending persistence operations in the background with [`ChainMonitor::list_pending_monitor_updates`] and [`ChainMonitor::get_monitor`].

Currently we mention this only for sync case in these docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For async users I kinda assume they dont do that polling but instead just spawn a task that loop {}s forever.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can assume that, I think users will trip up on this. Since async has many different ways to be implemented and this is more related to failure handling in async.

We can have something like:
"
Implementations should either implement async persistence by retrying infinitely in a loop OR retry all pending persistence operations in the background using [ChainMonitor::list_pending_monitor_updates] and [ChainMonitor::get_monitor].
"

Expand All @@ -113,7 +113,10 @@ impl MonitorUpdateId {
/// [`ChainMonitor::channel_monitor_updated`] must be called once for *each* update which occurs.
///
/// If at some point no further progress can be made towards persisting a pending update, the node
/// should simply shut down.
/// should simply shut down. Until then, the background task should either loop indefinitely, or
/// persistence should be regularly retried with [`ChainMonitor::list_pending_monitor_updates`]
/// and [`ChainMonitor::get_monitor`] (note that if a full monitor is persisted all pending
/// monitor updates may be marked completed).
///
/// # Using remote watchtowers
///
Expand Down
5 changes: 2 additions & 3 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3388,9 +3388,8 @@ where
/// In general, a path may raise:
/// * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee,
/// node public key) is specified.
/// * [`APIError::ChannelUnavailable`] if the next-hop channel is not available for updates
/// (including due to previous monitor update failure or new permanent monitor update
/// failure).
/// * [`APIError::ChannelUnavailable`] if the next-hop channel is not available as it has been
/// closed, doesn't exist, or the peer is currently disconnected.
/// * [`APIError::MonitorUpdateInProgress`] if a new monitor update failure prevented sending the
/// relevant updates.
///
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/shutdown_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ fn shutdown_on_unfunded_channel() {
nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 1_000_000, 100_000, 0, None).unwrap();
let open_chan = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());

// P2WSH
// Create a dummy P2WPKH script
let script = Builder::new().push_int(0)
.push_slice(&[0; 20])
.into_script();
Expand Down