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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Doc and comment followups to #2562 by TheBlueMatt · Pull Request #2591 · lightningdevkit/rust-lightning · GitHub
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
4 changes: 2 additions & 2 deletions lightning-persister/src/fs_store.rs
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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