Skip to content

Hold runtime lock during stop and reduce timeout values across the board#538

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.

Sign up for GitHub

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

Closed
tnull wants to merge 3 commits into from
Closed

Hold runtime lock during stop and reduce timeout values across the board #538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6de3fa3
Hold runtime lock during shutdown
tnull May 13, 2025
5a521c1
f comment
tnull May 15, 2025
d76160d
Reduce syncing and shutdown timeouts considerably
tnull May 13, 2025
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
2 changes: 1 addition & 1 deletion src/chain/electrum.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
electrum_client: Arc<ElectrumClient>,
Expand Down
7 changes: 5 additions & 2 deletions src/config.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;

@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?

@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.

For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?

@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?

Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.

Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.

@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?

Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.

So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?

@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?

Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?

So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?

Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.

@joostjagerjoostjagerMay 15, 2025•
edited
Loading

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 meant whatever we are waiting for that is outside ldk-node.

Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.

Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?

@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?

No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.

FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.


// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down
17 changes: 10 additions & 7 deletions src/lib.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
default_user_config, may_announce_channel, ChannelConfig, Config,
LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
///
/// After this returns most API methods will return [`Error::NotRunning`].
pub fn stop(&self) -> Result<(), Error> {
let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
// We hold the write lock for the duration of this method to avoid any conflicts with
// inflight tasks when users would potentially concurrently try restart the node before
// `stop` returned.
let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.
let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

#[cfg(tokio_unstable)]
let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
let event_handling_stopped_logger = Arc::clone(&self.logger);
let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

// FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
// event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
// should drop this considerably post upgrading to BDK 1.0.
let timeout_res = tokio::task::block_in_place(move || {
runtime.block_on(async {
tokio::time::timeout(
Duration::from_secs(100),
Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
event_handling_stopped_receiver.changed(),
)
.await
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" + '
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
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('^' + ".*" + '
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
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('^' + ".*" + '
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
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" + '
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
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('^' + ".*" + '
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
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('^' + ".*" + '
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
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); }
})();
})();
Hold runtime lock during `stop` and reduce timeout values across the board by tnull · Pull Request #538 · lightningdevkit/ldk-node · GitHub
Skip to content
Hold runtime lock during stop and reduce timeout values across the board#538

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.

 Sign up for GitHub

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

 Closed

tnull
wants to merge
3
commits into

from

 Closed


Hold runtime lock during stop and reduce timeout values across the board
#538
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
6de3fa3

Hold runtime lock during shutdown


tnull May 13, 2025
5a521c1

f comment


tnull May 15, 2025
d76160d

Reduce syncing and shutdown timeouts considerably


tnull May 13, 2025

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


2 changes: 1 addition & 1 deletion
src/chain/electrum.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -40,7 +40,7 @@ use std::time::{Duration, Instant};

const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 20;
const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

pub(crate) struct ElectrumRuntimeClient {
	electrum_client: Arc<ElectrumClient>,
Expand Down

7 changes: 5 additions & 2 deletions
src/config.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,13 @@ pub(crate) const NODE_ANN_BCAST_INTERVAL: Duration = Duration::from_secs(60 * 60
pub(crate) const WALLET_SYNC_INTERVAL_MINIMUM_SECS: u64 = 10;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 90;
pub(crate) const BDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 20;

// The timeout after which we abort a wallet syncing operation.
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 30;
pub(crate) const LDK_WALLET_SYNC_TIMEOUT_SECS: u64 = 10;
@joostjagerjoostjagerMay 13, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
@joostjagerjoostjagerMay 13, 2025

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.

Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
For the BDK timeout, it also seems that stop returns Ok(), so also no indication other than log that something is wrong?
@tnulltnullMay 14, 2025

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.

What changed so that huge values are 'hopefully' not required anymore? Did they remove that central mutex?
Yes, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Side question: isn't it better to not have a timeout? I don't know if it is desired to have background processes running when stop returns.
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
@joostjagerjoostjagerMay 14, 2025

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, they dropped that mutex that used to be held for the entire duration of syncing the wallet with BDK 1.0. Here, we just do (late) accommodations since the behavior since the upgrade improved considerably.
Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Hmm, I'm not sure. In general we should never reach that timeout, even though we recently got some reports to the contrary. But not having a timeout at all might also lead users to just kill the process after some (even more random time), which might have worse consequences.
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
@tnulltnullMay 15, 2025

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.

Is there a rationale for the new timeout values? I'd imagine they should match BDK timeouts plus something? Or is the BDK timeout behavior complex and not reducable to a single value?
Not quite sure what you mean with 'BDK timeout'? The individual electrum/esplora clients might have separate timeouts on a per-request basis, is that what you were referring to here?
So when "our" timeout expires and the node is reported to be stopped, wouldn't the process then typically be terminated anyway?
Yes, but we'd have more control over what we require finishing before timing out. Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
@joostjagerjoostjagerMay 15, 2025•

edited

Loading

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 meant whatever we are waiting for that is outside ldk-node.
Although, indeed, if we're currently would for some reason get blocked in the events processing and just timeout and move on, we might miss the final persistence round of the BP on shutdown.
Doesn't this mean that we're better off / safer without the timeout? Or should the timeout be restricted to just the external processes and not the whole event handler?
@tnulltnullMay 16, 2025

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.

Doesn't this mean that we're better off / safer without the timeout?
No, the timeout generally gives us control on which parts of the shutdown we wait and which we might skip. We could see to eventually do it in a more fine-grained fashion, but generally the timeout is really meant to be a last resort, as we don't expect the event handler to block for longer durations at a time.
FWIW, I recently start playing around with wrapping the event handling method itself in a timeout (rather than timing out waiting on the BP to shut down), which would at least mean the BP would shutdown 'normally'. However, this would mean we'd need to error with ReplayEvent if we hit the timeout, and I'm not fully convinced yet this is the way to go, but maybe we should.

// The timeout after which we give up waiting on LDK's event handler to exit on shutdown.
pub(crate) const LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS: u64 = 30;

// The timeout after which we abort a fee rate cache update operation.
pub(crate) const FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 5;
Expand Down

17 changes: 10 additions & 7 deletions
src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -126,8 +126,9 @@ pub use builder::NodeBuilder as Builder;

use chain::ChainSource;
use config::{
	default_user_config, may_announce_channel, ChannelConfig, Config, NODE_ANN_BCAST_INTERVAL,
	PEER_RECONNECTION_INTERVAL, RGS_SYNC_INTERVAL,
	default_user_config, may_announce_channel, ChannelConfig, Config,
	LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS, NODE_ANN_BCAST_INTERVAL, PEER_RECONNECTION_INTERVAL,
	RGS_SYNC_INTERVAL,
};
use connection::ConnectionManager;
use event::{EventHandler, EventQueue};
Expand DownExpand Up@@ -643,7 +644,12 @@ impl Node {
	///
	/// After this returns most API methods will return [`Error::NotRunning`].
	pub fn stop(&self) -> Result<(), Error> {
 let runtime = self.runtime.write().unwrap().take().ok_or(Error::NotRunning)?;
 // We hold the write lock for the duration of this method to avoid any conflicts with
 // inflight tasks when users would potentially concurrently try restart the node before
 // `stop` returned.
 let mut runtime_lock = self.runtime.write().unwrap();
Comment thread
tnull marked this conversation as resolved.

 let runtime = runtime_lock.take().ok_or(Error::NotRunning)?;

 #[cfg(tokio_unstable)]
 let metrics_runtime = Arc::clone(&runtime);

Expand DownExpand Up@@ -672,13 +678,10 @@ impl Node {
 let event_handling_stopped_logger = Arc::clone(&self.logger);
 let mut event_handling_stopped_receiver = self.event_handling_stopped_sender.subscribe();

 // FIXME: For now, we wait up to 100 secs (BDK_WALLET_SYNC_TIMEOUT_SECS + 10) to allow
 // event handling to exit gracefully even if it was blocked on the BDK wallet syncing. We
 // should drop this considerably post upgrading to BDK 1.0.
 let timeout_res = tokio::task::block_in_place(move || {
 runtime.block_on(async {
 tokio::time::timeout(
 Duration::from_secs(100),
 Duration::from_secs(LDK_EVENT_HANDLER_SHUTDOWN_TIMEOUT_SECS),
 event_handling_stopped_receiver.changed(),
 )
 .await
Expand Down