From 4abc939aa0f10c896972bcf4d8c76478a04b368c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 21 Sep 2023 20:03:13 +0000 Subject: [PATCH 1/5] Correct test comments missed in f254c56585688a187a58dc284477cb1cd39 --- lightning-persister/src/fs_store.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs index 42b28018fe9..c8bd3f563fd 100644 --- a/lightning-persister/src/fs_store.rs +++ b/lightning-persister/src/fs_store.rs @@ -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] @@ -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(); From 11b228b7b0d0f4f8cf0ca4829f89a9b281359606 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 21 Sep 2023 20:19:08 +0000 Subject: [PATCH 2/5] Correct ChannelUnavailable error docs on `send_payment_with_route` Monitor update failure can no longer lead to a `ChannelUnavailable` error, but more common cases such as the peer being disconnected always could, so those should be documented clearer. --- lightning/src/ln/channelmanager.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e2641d2cf16..23b45077af0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -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. /// From 974ba314a29fa35f2ed662b9357fa9cd701e444d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 21 Sep 2023 20:21:13 +0000 Subject: [PATCH 3/5] Refer to an "outage" over a "timeout", for when failure happens Timeouts may be worth retrying, but an outage is a more general term which obviously cannot be retried. --- lightning/src/chain/chainmonitor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index a49b3b42687..60d50bf0adb 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -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. /// From 39094d4472df101ebf783bf2dc547ee11b863cf0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 26 Sep 2023 16:37:40 +0000 Subject: [PATCH 4/5] Correct comment in `shutdown_on_unfunded_channel` which described the script type incorrectly. --- lightning/src/ln/shutdown_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 07bb72ce9cb..47361693693 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -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(); From 34dd48c585cacb5dd5d127964bcf6da7abff9692 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Sep 2023 20:45:27 +0000 Subject: [PATCH 5/5] Add more details about async persistence completion/backgrounding This clarifies somewhat that async persistence should run indefinitely or keep trying via polling, and that either is acceptable. --- lightning/src/chain/chainmonitor.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 60d50bf0adb..6d003df720f 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -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 ///