simln-lib: add htlc interceptor for simulated nodes #261

elnosh · 2025-05-06T15:18:53Z

Mostly took changes from these 2 commits to add an Interceptor trait:

Some of the things I changed from those commits:

intercept_htlc now returns a Result to communicate the result of the intercepted htlc instead of sending it through a channel.
spawns a task in a JoinSet for each interceptor's intercept_htlc so that if any of those holds the htlc for a long time it does not block the other interceptors. It then waits for the completion of the tasks in the joinset. If any of them returns a result to fail the htlc, it drops the JoinSet since there is no need to wait for completion of other tasks because the htlc will fail anyways.

carlaKC · 2025-05-06T17:40:59Z

Driveby comment: could we split up the logic adding a unique index and the interceptor into separate commits?
For the sake of breaking up into smaller logical chunks / reviewability.

carlaKC

Main comments are around how we handle shutdown in a reaonsable way. Specifically:

An interceptor has had a critical failure, how does it tell the simulator to shut down
The simultor needs to terminate the interceptors, how can we cleanly do this so that interception code isn't left in a bad state

carlaKC · 2025-05-09T13:34:54Z

simln-lib/src/sim_node.rs

+    #[error("DuplicateCustomRecord: key {0}")]
+    DuplicateCustomRecord(u64),
+    #[error("InterceptorError: {0}")]
+    InterceptorError(Box<dyn Error + Send + Sync + 'static>),


Let's go ahead and define this as an error type in the top level library?

I think that simln could use an overhaul in the way we do errors (I did not understand rust errors when we started this project 🙈 ), and defining a reasonable general error type seems like a good start.

carlaKC · 2025-05-09T13:36:06Z

simln-lib/src/sim_node.rs

+    /// The short channel id for the incoming channel that this htlc was delivered on.
+    pub incoming_htlc: HtlcRef,


nit: note that this is a unique identifier for the htlc

carlaKC · 2025-05-09T13:36:45Z

simln-lib/src/sim_node.rs

+    /// Custom records provided by the incoming htlc.
+    pub incoming_custom_records: CustomRecords,
+
+    /// The short channel id for the outgoing channel that this htlc should be forwarded over.


nit: expand doc to note that None indicates that the intercepting node is the receiver.

carlaKC · 2025-05-09T14:03:08Z

simln-lib/src/sim_node.rs

+                );
+
+                let interceptor_clone = Arc::clone(interceptor);
+                intercepts.spawn(async move { interceptor_clone.intercept_htlc(request).await });


This might be a good spot for futures::future::join_all?

Rather than needing to spawn a new task here, we can just call (but not await) the intercept_htlc method and then await them in whatever order they arrive after that?

Author's choice. - I don't see us running with so many interceptors that creating these tasks starts to give us a performanc hit.

looked into join_all but if iiuc it will wait until all futures resolve. I think we'd prefer to get the responses as the tasks finish rather than waiting for all to finish? that way we can fail the other interceptors if one failed the htlc.

I think we'd prefer to get the responses as the tasks finish rather than waiting for all to finish? that way we can fail the other interceptors if one failed the htlc.

Yeah indeed, we want to hear back from the first completing one so that we can cancel the rest. Could use FuturesUnordered but I think that's an over-optimization (it's for lots of futures), so happy to leave as-is 👍

carlaKC · 2025-05-09T14:07:42Z

simln-lib/src/sim_node.rs

+                        }
+                    },
+                    Ok(Err(e)) => {
+                        drop(intercepts);


I think that we may need a more gentle shutdown than dropping the task (which afaik will force abort the intercept_htlc call).

I can see it being difficult to write interceptor code that works with another system when your code may be run half way and then killed - could end up with funny states. My instinct is to provides a triggered pair and instruct interceptors to listen on it for shutdown signals? Open to other approaches as well.

I do think that the ability to shut down all the other interceptors once the first error is reached is a really, really nice feature.

sounds good 👍 provide that trigger in the InterceptRequest and send on it if we need to fail other interceptors?
just to note, although intercept_htlc may force-aborted, interceptor should still get notified about htlc resolution through notify_resolution but I think we'd still like to shutdown gently through intercept_htlc?

provide that trigger in the InterceptRequest and send on it if we need to fail other interceptors?

I think that we should handle the triggering?

Give each interceptor a listener

On first interceptor error, trigger shutdown

Then interceptors are responsible for listening on that shutdown listener and getting their state in order before they exit. We still wait for each to finish, but that should be relatively quick because we've signaled that it's shutdown time.

Interceptor should still get notified about htlc resolution through notify_resolution

As-is I don't think we'd notify if the HTLC is failed by the interceptor? It'll never be "fully" forwarded by the node, so we don't notify it being resolved.

Edit: although coming to think of this, we probably do want to notify the failure even if one of the interceptors has returned a fail outcome - the others may have returned success and aren't aware that it actually never ended up going through.

Give each interceptor a listener

On first interceptor error, trigger shutdown

Then interceptors are responsible for listening on that shutdown listener and getting their state in order before they exit. We still wait for each to finish, but that should be relatively quick because we've signaled that it's shutdown time.

got it, will do.

As-is I don't think we'd notify if the HTLC is failed by the interceptor? It'll never be "fully" forwarded by the node, so we don't notify it being resolved.

I thought we would. Because if an interceptor returns an error, we then return it in add_htlcs and then call remove_htlcs here: https://github.com/elnosh/sim-ln/blob/2de405fdcb37c8ffe4bd8d2cc0077ef7099cc3ed/simln-lib/src/sim_node.rs#L1274 which internally calls notify_resolution for each interceptors

I thought we would.

Ok cool, forgot when that is/isn't called bu sgtm!

carlaKC · 2025-05-09T14:09:01Z

simln-lib/src/sim_node.rs

+pub trait Interceptor: Send + Sync {
+    /// Implemented by HTLC interceptors that provide input on the resolution of HTLCs forwarded in the simulation.
+    async fn intercept_htlc(&self, req: InterceptRequest)
+        -> Result<CustomRecords, ForwardingError>;


If we only have one layer of Result here, how does the intercept_htlc call tell the simulator that it's hit a ciritical error and wants to shut down (rather than a forwarding error for this payment specifically)?

I wasn't sure if that's something we'd want. I didn't like the idea of nested Results. It could instead be communicated through specific variants in ForwardingError? Simulation could check with is_critical if the ForwardingError returned warrants a shutdown.

It could instead be communicated through specific variants in ForwardingError

That seems like a bit of a layering violation to me. AnInterceptorError for interceptors that might want to fail forwards with their own ForwardingFalure reason that isn't defined in simln (eg, you don't have enough reputation) makes sense to me, but we're stretching its definition a bit to cover things like unexpected internal state errors.

Nested results are definitely ugly, but I think that it more clearly represents the two types of failures that we could have here? One for "fail this HTLC", one for "something has gone wrong with my interception". If we define a type for the inner result the function signatures won't be too ugly at least.

fair! will change 👍

elnosh · 2025-05-09T20:08:07Z

An interceptor has had a critical failure, how does it tell the simulator to shut down

I wasn't sure if letting the interceptor shutdown the simulation was something we'd want but it could be done through specific variants in the ForwardingError returned? Simulation could check with is_critical if the ForwardingError returned warrants a shutdown. Although Interceptors should be aware of this to trigger a shutdown by providing specific variants.

The simultor needs to terminate the interceptors, how can we cleanly do this so that interception code isn't left in a bad state

should we include something like a channel or shutdown signal in InterceptRequest?

carlaKC · 2025-05-12T12:31:59Z

WDYT about adapting this latency interceptor and surfacing it on the CI?

Nice to have a user of the interceptor that we can run + test in master rather than merging in some unused code and having to test it externally.

elnosh · 2025-05-12T13:25:50Z

sgtm, as I was doing exactly that and testing externally heh.

elnosh · 2025-05-12T15:55:50Z

simln-lib/src/sim_node.rs

+    async fn notify_resolution(
+        &self,
+        _res: InterceptResolution,
+    ) -> Result<(), Box<dyn Error + Send + Sync + 'static>> {
+        Ok(())
+    }


rethinking this, I'm wondering what type of error should this be or if this method should return a Result at all? Leaning towards this should not return a Result since the simulation is just notifying the interceptor of a resolution.

Perhaps the interceptor wants to signal shutdown on the notify_resolution because it got information it wasn't expecting? Or a database became unavailable, etc.

elnosh added 2 commits May 7, 2025 10:23

simln-lib: add unique index to htlcs in ChannelState

07c87f1

simln-lib: add htlc Interceptor trait to simulated node

2de405f

elnosh force-pushed the interceptor branch from 62a573e to 2de405f Compare May 7, 2025 15:45

carlaKC reviewed May 9, 2025

View reviewed changes

elnosh commented May 12, 2025

View reviewed changes

elnosh mentioned this pull request May 12, 2025

simln-lib: refactor ForwardingError to nested results #265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simln-lib: add htlc interceptor for simulated nodes #261

simln-lib: add htlc interceptor for simulated nodes #261

elnosh commented May 6, 2025

carlaKC commented May 6, 2025

carlaKC left a comment

carlaKC May 9, 2025

carlaKC May 9, 2025

carlaKC May 9, 2025

carlaKC May 9, 2025

elnosh May 9, 2025

carlaKC May 9, 2025

carlaKC May 9, 2025

elnosh May 9, 2025

carlaKC May 9, 2025 •

edited

Loading

elnosh May 9, 2025

carlaKC May 9, 2025

carlaKC May 9, 2025

elnosh May 9, 2025

carlaKC May 9, 2025

elnosh May 9, 2025

elnosh commented May 9, 2025 •

edited

Loading

carlaKC commented May 12, 2025

elnosh commented May 12, 2025

elnosh May 12, 2025

carlaKC May 12, 2025

		/// The short channel id for the incoming channel that this htlc was delivered on.
		pub incoming_htlc: HtlcRef,

simln-lib: add htlc interceptor for simulated nodes #261

Are you sure you want to change the base?

simln-lib: add htlc interceptor for simulated nodes #261

Conversation

elnosh commented May 6, 2025

carlaKC commented May 6, 2025

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elnosh commented May 9, 2025 • edited Loading

carlaKC commented May 12, 2025

elnosh commented May 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC May 9, 2025 •

edited

Loading

elnosh commented May 9, 2025 •

edited

Loading