Skip to content

Fix Critical Error handling in HTLC interception #294

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix Critical Error handling in HTLC interception #294

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f19be5
sim-ln/refactor:handle graceful shutdown after critical error happens
mubarak23 Jul 1, 2025
12b5de4
sim-ln/refactor:test intercept htlc with critical error from one inte…
mubarak23 Jul 2, 2025
587f220
sim-ln/refactor:update comment
mubarak23 Jul 2, 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
47 changes: 45 additions & 2 deletions simln-lib/src/sim_node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,8 @@ pub struct HtlcRef {
/// If any interceptor returns a `ForwardingError`, it triggers a shutdown signal to all other
/// interceptors and waits for them to shutdown. The forwarding error is then returned.
/// If any interceptor returns a `CriticalError`, it is immediately returned to trigger a
/// simulation shutdown. TODO: If a critical error happens, we could instead trigger the shutdown
/// simulation shutdown.
// If a critical error happens, we could instead trigger the shutdown
/// for the interceptors and let them finish before returning.
/// While waiting on the interceptors, it listens on the shutdown_listener for any signals from
/// upstream and trigger shutdowns to the interceptors if needed.
Expand Down Expand Up @@ -880,6 +881,8 @@ async fn handle_intercepted_htlc(
// the HTLC. If any of the interceptors did return an error, we send a shutdown signal
// to the other interceptors that may have not returned yet.
let mut interceptor_failure = None;
let mut critical_error = None;

'get_resp: loop {
tokio::select! {
res = intercepts.join_next() => {
Expand Down Expand Up @@ -917,8 +920,11 @@ async fn handle_intercepted_htlc(
interceptor_failure = Some(fwd_error);
interceptor_trigger.trigger();
},
// Interceptor returned a CriticalError, Trigger a shutdown and store the error
// to return after all interceptors have finished.
Err(e) => {
return Err(e);
critical_error = Some(e);
interceptor_trigger.trigger();
},
}
}
Expand All @@ -933,6 +939,11 @@ async fn handle_intercepted_htlc(
}
}

// if we have a critical error, returned it after all interceptors have finished.
if let Some(e) = critical_error {
return Err(e);
}

if let Some(e) = interceptor_failure {
return Ok(Err(e));
}
Expand Down Expand Up @@ -2470,6 +2481,38 @@ mod tests {
assert!(response.unwrap().unwrap() == CustomRecords::from([(1000, vec![1])]));
}

/// Tests intercepted htlc with a critical error from one interceptor.
#[tokio::test]
async fn test_intercepted_htlc_critical_error() {
// Interceptor that will return a critical error.
let mut mock_interceptor = MockTestInterceptor::new();
mock_interceptor
.expect_intercept_htlc()
.returning(|_| Err(CriticalError::InterceptorError("critical failure".into())));
mock_interceptor
.expect_notify_resolution()
.returning(|_| Ok(()));

let (interceptor_trigger, interceptor_listener) = triggered::trigger();
let mock_request = create_intercept_request(interceptor_listener);

let mock_interceptor: Arc<dyn Interceptor> = Arc::new(mock_interceptor);
let interceptors = vec![mock_interceptor];
let (_, shutdown_listener) = triggered::trigger();

let result = handle_intercepted_htlc(
mock_request,
&interceptors,
interceptor_trigger,
shutdown_listener,
)
.await;

assert!(result.is_err());
let err_string = format!("{:?}", result.unwrap_err());
assert!(err_string.contains("critical failure"));
}
Comment on lines +2486 to +2514
Copy link
Collaborator

@elnosh elnosh Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

hmm this checks that the returned error is the one returned from interceptor but not sure if @carlaKC meant to add a case where other interceptors received a shutdown signal if a critical error was returned from another one. Although we are just mocking interceptors so I'm babbling here.

Copy link
Contributor

Choose a reason for hiding this comment

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

to add a case where other interceptors received a shutdown signal if a critical error was returned from another one

This is what I meant.

Sadly I don't think that we'll be able to mock the interceptors to check that each of them trigger shutdown. Was thinking of a very simple interceptor that just blocks until we get shutdown and then we add a timeout on the test to make sure that nothgin is blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so there is no way to get an interceptor to blocked until we got a shutdown

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean? You can implement an interceptor that listens on the shutdown listener for the purpose of the test and that should block?

We wait for all the interceptors to exit when we get the shutdown signal, so they should block if the trigger isn't hit by a critical error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,
i will probably look into how mock interceptor are done i.e

create_intercept_request

then i will implement an interceptor that listens on the shutdown listener and mock it


/// Tests a long resolving interceptor gets correctly interrupted during a shutdown.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_shutdown_intercepted_htlc() {
Expand Down