intercept htlcs #1601

johncantrell97 · 2022-07-07T03:15:31Z

This PR aims to add a new fake_scid Namespace (Namespace::Intercept) that allows htlcs forwarded with scids from this namespace to be surfaced to end-users via a new event (PaymentIntercepted). The end-user must then either fail using fail_intercepted_payment or forward it using forward_intercepted_payment methods on ChannelManager.

The main use-case in mind is for enabling on-demand channel creation though there are likely other interesting use-cases that are also enabled by it.

I need help with updating serialization (read/write) for ChannelManager because we will need to add a new HashMap for storing the intercepted payments that we are waiting on the end-user to tell us what to do with them.

I'm also not sure the best way to fail these. Please see the note in fail_intercepted_payment about one thought I had.

I'm also not exactly sure what other functionality might be missing. I added ability to generate a fake intercepter scid on the channel manager but there's nothing with route hints yet. I'm not sure if it makes sense to add that to LDK or if it should be the end-user's responsibility because we can't be sure what they're doing with them? I guess they'll always at least want to include the fake_scid as the last hop but they'll likely want to specify a hop before that as well -- which in the on-demand channel / lsp use-case ldk won't know about this node.

Lastly, I wrote about this on discord but I'm not sure what is needed in terms of signaling CM persistence when htlcs are drained from forward_htlcs => pending_intermittent_payments and then again moved back into foward_htlcs once signaled from user. Feels like it's possible we lose an htlc in one of these transitions if the node is killed.

johncantrell97 · 2022-07-07T03:42:02Z

lightning/src/ln/channelmanager.rs

+															new_events.push(events::Event::PaymentIntercepted {
+																short_channel_id: short_chan_id,
+																payment_hash,
+																inbound_amount_msats: 0,


I'm not sure best way to get this or if it's really that important to surface it? Does it need to be piped through from earlier steps?

Oh, hmmm, yea, that's annoying. We'll have to add it to HTLCForwardInfo::AddHTLC, I guess. I think we can still cheat and make the Event non-Optional - in general if a user is using a new feature its okay if it doesnt work if they downgrade to an old LDK. If an AddHTLC is missing the previous_htlc_amount we can just fail the HTLC.

yeah that makes sense. we can fail it safely because if it's missing that attribute it means it was created on a version that didn't support interception, right?

johncantrell97 · 2022-07-07T03:42:36Z

lightning/src/ln/channelmanager.rs

+		if let Some(_payment) = pending_intercept {
+			// TODO: what's best way to fail this? in `process_pending_htlc_forwards` it uses that `fail_forward` macro
+			//			 awkward way could be to just use the forward_intercepted_payment code and pick a random scid
+		}


best way to handle this? see comment

You'll have to move the process_pending_htlc_forwards fail_forward macro out into a util method, constructing the HTLCSource object and call fail_htlc_backwards_internal.

johncantrell97 · 2022-07-07T03:43:10Z

lightning/src/ln/channelmanager.rs

+
+							Ok(())
+					},
+					_ => Err(APIError::APIMisuseError { err: "impossible".to_string() })


Not sure how to handle this exactly. It should be impossible to get into this state.

Two options: (a) move the contents of HTLCForwardInfo::AddHTLC into a struct and just store that struct here, or (b) just unreachable it. (b) kinda sucks but its less of a pain touching a bunch of code.

with a) i assume when you say "store that struct here" you mean in pending_intercepted_payments?

b) does kind of suck but I guess would you accept a PR that just did that?

I'd survive (b), but you can also do (a) in a separate PR. Indeed, I meant in the new field for tracking intercepted payments.

Wait, did you? I intepreted (a) as also implying that we should make HTLCForwardInfo::AddHTLC be AddHTLC(NewStruct).

oh whoops, i misunderstood. i thought you meant create a new separate struct that has all the info, not literally store the struct in HTLCForwardInfo::AddHTLC.

ok I can try to refactor it to what you meant.

johncantrell97 · 2022-07-07T03:43:59Z

lightning/src/ln/channelmanager.rs

+															});
+														},
+														hash_map::Entry::Occupied(_) => {
+															fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None);


do we want to fail with this same error message in this case?

The error code itself should remain, but the text isn't sent to peers, its just printed locally, so we can add more detail there.

codecov-commenter · 2022-07-07T15:54:18Z

Codecov Report

Base: 90.73% // Head: 90.62% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (e421b59) compared to base (7544030).
Patch coverage: 54.90% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1601      +/-   ##
==========================================
- Coverage   90.73%   90.62%   -0.11%     
==========================================
  Files          87       87              
  Lines       46713    46791      +78     
  Branches    46713    46791      +78     
==========================================
+ Hits        42383    42403      +20     
- Misses       4330     4388      +58

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`36.20% <0.00%> (-1.94%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.64% <57.37%> (-0.53%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.68% <100.00%> (ø)`
lightning/src/util/scid_utils.rs	`99.28% <100.00%> (+0.09%)`	⬆️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.90% <0.00%> (-0.15%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2022-07-10T17:15:42Z

lightning/src/ln/channelmanager.rs

@@ -1432,7 +1463,7 @@ macro_rules! handle_chan_restoration_locked {
 		let chanmon_update_is_none = chanmon_update.is_none();
 		let counterparty_node_id = $channel_entry.get().get_counterparty_node_id();
 		let res = loop {
-			let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve
+			let forwards: Vec<(PendingHTLCInfo, u64, Option<u64>)> = $pending_forwards; // Force type-checking to resolve


Now that you've threaded it through everywhere - does it not make more sense to add this as a field in PendingHTLCInfo? It looks like that's constructed in the receive pipeline so we still have the amount field, then you shouldn't need to touch channel.rs

if i add it to PendingHTLCInfo then I wouldn't also have to thread through this extra Option in the tuple, right?

It does seem to make sense to just put it in there. it will still need to be an Option because they're serialized as part of pending forwards, right?

There's sort of another weirdness:

seems like in the Receive case, we are setting amt_to_forward to the 'prev/incoming amt' which ends up being fine because the logic differs for receives even though it's the same object.

i guess if i add this new field, would we want to update the receive logic as well to use it? though old PendingHtlcInfo's will still have the forward field set. so we'd probably just want to store the incoming amount in both fields.. which is going to look pretty confusing.

if i add it to PendingHTLCInfo then I wouldn't also have to thread through this extra Option in the tuple, right?

Yep.

it will still need to be an Option because they're serialized as part of pending forwards, right?

Yep.

i guess if i add this new field, would we want to update the receive logic as well to use it? though old PendingHtlcInfo's will still have the forward field set. so we'd probably just want to store the incoming amount in both fields.. which is going to look pretty confusing.

Probably just use the existing field (and maybe add a debug_assert that they're equivalent, if the new one is Some) - dealing with backwards compat to switch to a new field just sounds annoying. A debug_assert is self-documenting.

TheBlueMatt · 2022-07-10T17:16:12Z

lightning/src/ln/channelmanager.rs

@@ -6527,6 +6654,7 @@ impl_writeable_tlv_based_enum!(HTLCForwardInfo,
 		(2, prev_short_channel_id, required),
 		(4, prev_htlc_id, required),
 		(6, prev_funding_outpoint, required),
+		(8, prev_htlc_amount, option)


New fields generally need to be odd - this allows older versions of LDK to simply ignore the field when reading instead of it causing a read error.

johncantrell97 · 2022-07-11T12:40:13Z

lightning/src/ln/channelmanager.rs

+			intercept_id.write(writer)?;
+			pending_intercepted_payment.write(writer)?;
+		}
+


not sure where exactly this should be written. seemed like last was a good idea.

johncantrell97 · 2022-07-11T12:41:36Z

lightning/src/ln/channelmanager.rs

+				return Err(DecodeError::InvalidValue);
+			}
+		}
+


tried to read it last as well but the reader var gets moved into the read_tlv_fields! for outbound_payments below. just put it here for now so it compiles but I'm pretty sure this won't work.

johncantrell97 · 2022-07-11T14:14:21Z

ok moved the incoming amount to PendingHTLCInfo and now channel.rs is untouched. I also attempted to update the ChannelManager read/write but I'm not entirely confident there. I basically just added it to the end 🤷‍♂️

let me know what else might be missing when you get a chance.

TheBlueMatt · 2022-07-11T18:02:40Z

lightning/src/ln/channelmanager.rs

@@ -3055,6 +3088,63 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		Ok(())
 	}

+	/// Fails the intercepted payment indicated by intercept_id.  This should really only be called in response
+	/// to a PaymentIntercepted event


Can you link the event with [` and `]

TheBlueMatt · 2022-07-11T18:03:49Z

lightning/src/ln/channelmanager.rs

+		match pending_intercept {
+			None => Err(APIError::APIMisuseError { err: "Payment with that InterceptId not found".to_string() }),
+			Some(payment) => {
+					let routing = match payment.forward_info.routing {


nit: spurious indentation here.

TheBlueMatt · 2022-07-11T18:05:34Z

lightning/src/ln/channelmanager.rs

+
+							Ok(())
+					},
+					_ => Err(APIError::APIMisuseError { err: "impossible".to_string() })


Wait, did you? I intepreted (a) as also implying that we should make HTLCForwardInfo::AddHTLC be AddHTLC(NewStruct).

TheBlueMatt · 2022-07-11T18:05:57Z

lightning/src/ln/channelmanager.rs

@@ -131,6 +132,13 @@ pub(super) enum PendingHTLCStatus {
 	Fail(HTLCFailureMsg),
 }

+pub(super) struct PendingInterceptedHTLC {


Are we gonna meke HTLCForwardInfo::AddHTLC just hold this (and maybe rename it).

Yeah, my bad. agreed we should rename it if so.

Looking at this now I'm a little confused. Does HTLCForwardInfo::AddHTLC need to "hold" this struct? They are literally exactly the same already. Was this not the case back in July when this was first written?

Unless I'm misremembering something couldn't I just get rid of this PendingInterceptedHTLC struct entirely and just store (or clone) the entire HTLCForwardInfo as the intercepted htlc instead of essentially copying it into this new struct?

I guess it would mean matching on HTLCForwardInfo anywhere I need to use it and ignoring/erroring if we came across a ::FailHTLC when we expected an ::AddHTLC.

Do you know what you meant by hold it when it's already the same data?

Oh whoops. There's a separate comment thread in here about this. You meant change it to HTLCForwardInfo::AddHTLC(RenamedPendingInterceptedHTLC).

Is the reason to update the AddHTLC variant to hold a struct instead of using named fields so that when I need to store the "pending intercepted htlcs" I can take (or clone) this inner struct instead of having to store the entire HTLCForwardInfo? Thus also eliminating the need to match against it.

So I starting working on this and it's a bit of a pain so I wanted to make sure it still makes sense to do this before I continue.

A couple things I wanted to highlight or questions I have about it if we do want to move forward with that idea:

There's a lot of places where you match on HTLCForwardInfo::AddHTLC { fields...} and do a nested match on the PendingRoutingHTLC::Forward variant at the same time. I was doing this instead in the new version:

// destructuring to avoid having to write `pending_add_htlc_info.<field_name>` everywhere let PendingAddHTLCInfo { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo { routing, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, .. }, prev_funding_outpoint } = pending_add_htlc_info; // instead of the nested match, putting the whole arm inside this if let instead :/ if let PendingHTLCRouting::Forward { onion_packet, .. } = routing { ... }

Is there a cleaner way to do this?

How can I insert the PendingAddHTLCInfo (what I renamed it for now) into my map without cloning it? Is it possible? Is it okay if I have to clone it to insert it? Otherwise it's fields get used / thus moved before I need to insert.

Is there an issue with serialization / backwards compat? The old enums will be serialized with the ::AddHTLC struct variant and the new version will want to deserialize with this tuple / nested struct version. Is this okay?

Is the reason to update the AddHTLC variant to hold a struct instead of using named fields so that when I need to store the "pending intercepted htlcs" I can take (or clone) this inner struct instead of having to store the entire HTLCForwardInfo? Thus also eliminating the need to match against it.

Yea, basically.

Is there a cleaner way to do this?

Yes, you can directly match on the whole thing. This should work fine:

match enum_a { EnumA::VariantA(VariantAStruct { inner_enum: EnumB::VariantA { ... }, ... } => {} }

How can I insert the PendingAddHTLCInfo (what I renamed it for now) into my map without cloning it? Is it possible?

I'll have to look at the final code, but for a first draft just clone it.

Is there an issue with serialization / backwards compat?

Yes, we're going to have to look at this closely. But having to hand-write some serialization glue isn't a good reason to avoid cleaning up the logic to be what we want IMO. Happy to help write that glue.

Ok sounds good, just wanted to make sure it made sense before continuing down this path. Thanks!

TheBlueMatt · 2022-07-11T18:10:03Z

lightning/src/ln/channelmanager.rs

+				phantom_shared_secret: None,
+			});
+
+			let failure_reason = HTLCFailReason::Reason { failure_code: 0x4000 | 10, data: Vec::new() };


Actually, on second thought, lets use temporary_channel_failure here - x4000|10 means "the channel you tried to send to is bogus and will never exist", which is probably true in most cases, but the user may just have failed to talk to the client to open the JIT channel or whatever, so we may not want to do that.

Actually, we might extend fail_intercepted_payment to return a custom data message from the client ?

TheBlueMatt · 2022-07-11T18:13:20Z

lightning/src/ln/channelmanager.rs

+						PendingHTLCRouting::Forward { onion_packet, .. } => {
+							PendingHTLCRouting::Forward { onion_packet, short_channel_id: scid }
+						},
+						_ => payment.forward_info.routing


We need some clarity from the LSP spec folks on if this suffices - can we forward the existing onion or do they want to demand that we support unwrapping the onion differently. Id strongly prefer we keep this, but it may not suffice depending on what users want to do. At a minimum this needs to be documented.

yeah, my gut is that we might as well support it. I assume this means surfacing the original onion in the event in addition to being able to pass a new one here, right?

I don't think there's any value in surfaacing the existing onion - its encrypted for the final recipient so its not like we can do anything with it. I suppose we can add a "create new onion" option as a followup, but I'd kinda like to understand where the LSP folks are gonna go before we get that far - the other thing is the LSP folks may find mpp to be important, in which case this whole PR is probably not useful - the LSP will be the one to create the invoice, will receive the payment as a "normal" payment, then the LSP dev can open a channel, call send_payment with the same payment hash and call it a day, no need for the complexity here [1].

[1] Well, okay, we still need to probably actually support that, cause we have to bubble payment preimages through even if both channels go offline, but it will merit an entirely different approach than this PR.

oh right, that makes sense.

weren't there other non-lsp use-cases mentioned in here ( #680 ) for a feature like this?

also it seems like this can still be useful for non-mpp supporting lsp. i guess it really comes down to what trade-offs are being made with the mpp supporting lsp and if there's a strong case to support both flows.

Hmm, not sure what you mean by the 680 reference here - I think that's ultimately related to this but only in a "code cleanup" kinda way, not in any actual behavior changes.

oh i guess i misunderstood 680 as well. i thought it alluded to being able surface htlcs to end-user similarly to this 'intercept' pr to enable them to add additional relay logic at the user-level.

I mean, yea, it did talk about that, but its not ultimately required here.

TheBlueMatt · 2022-07-11T18:13:37Z

lightning/src/ln/channelmanager.rs

+						..payment.forward_info
+					};
+
+					let mut per_source_pending_forward = vec![(payment.prev_short_channel_id, payment.prev_funding_outpoint, vec![(pending_htlc_info, payment.prev_htlc_id)])];


The outer vec can just be a [], doesn't need to be a vec.

TheBlueMatt · 2022-07-11T18:14:14Z

lightning/src/ln/channelmanager.rs

+						_ => payment.forward_info.routing
+					};
+
+					let pending_htlc_info = PendingHTLCInfo {


Once you move ::AddHTLC to use the same struct it'd be nice to DRY this up with other forwarding.

TheBlueMatt · 2022-07-11T18:15:21Z

lightning/src/ln/channelmanager.rs

@@ -6696,6 +6833,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 			(11, self.probing_cookie_secret, required),
 		});

+		(pending_intercepted_payments.len() as u64).write(writer)?;


We can't add new fields at the end, they have to go in the TLVs.

yeah that makes sense, whoops.

ariard

Could be nice to have a look on the pending or future spec objects which would require or be easier to implement with such HTLC intercept interface (e.g trampoline or lightning/bolts#1008). To check if we fit them well.

ariard · 2022-07-15T00:30:26Z

lightning/src/ln/channelmanager.rs

+		loop {
+			let scid_candidate = fake_scid::Namespace::Intercept.get_fake_scid(best_block.height(), &self.genesis_hash, &self.fake_scid_rand_bytes, &self.keys_manager);
+			// Ensure the generated scid doesn't conflict with a real channel.
+			match channel_state.short_to_id.entry(scid_candidate) {


Btw, I wonder if this reorg-robust. At block N, we have real channel funding transaction X confirmed at index 2. We draw out the scid N:3:4, we're good. Block N gets reorged-out and real channel funding transaction X get confirmed anew at index 3. Now I think we have a conflict ?

We only generate fake scids up to 144 blocks before our highest known height IIUC

ariard · 2022-07-15T00:59:38Z

lightning/src/ln/channelmanager.rs

+																prev_funding_outpoint,
+															};
+															entry.insert(pending_intercepted_payment);
+															new_events.push(events::Event::PaymentIntercepted {


Okay I think we might have an issue here with the way we're implementing BOLT4 routing policy checks. Currently, we implement those checks even before to accept the inbound HTLC in internal_update_add_htlc(). From my memory, the spec does not mandate the order of the checks and it's not problematic as long as we don't provide an internal interface to manipulate relayed HTLCs. As we aim to do so with current PR, it might a source of confusion for the consumer, where additional routing policy checks or LSP callback actions are offered through the new PaymentIntercepted. I'm thinking of the "additional" routing policy checks could be even to turn off some of the default check (e.g bypassing CLTV_FAR_FAR_AWAYin the context of channel jamming mitigations experimentations or whatever).

So I don't know if the approach as suggested #680 could make more sense.I don't hold specifically to this approach, I just care it covers all the near-term use-cases we have mind.

ariard · 2022-07-15T01:01:44Z

lightning/src/ln/channelmanager.rs

+				phantom_shared_secret: None,
+			});
+
+			let failure_reason = HTLCFailReason::Reason { failure_code: 0x4000 | 10, data: Vec::new() };


Actually, we might extend fail_intercepted_payment to return a custom data message from the client ?

ViktorTigerstrom

Nice, seems like a very useful feature to have indeed! Left some suggestions on a few things I noticed when looking through this, other than whats been raised by the other reviewers already.

ViktorTigerstrom · 2022-07-17T23:49:05Z

lightning/src/ln/channelmanager.rs

+					};
+
+					let mut per_source_pending_forward = vec![(payment.prev_short_channel_id, payment.prev_funding_outpoint, vec![(pending_htlc_info, payment.prev_htlc_id)])];
+					self.forward_htlcs(&mut per_source_pending_forward);


Hmmm wouldn't we want to check that there's actually been a channel opened for the scid before we try forwarding over it by checking that there now exists an entry for the scid in the short_to_id map (now short_to_chan_info), and return an error if there's no such channel?
Else we will just end up generating a new PaymentIntercepted event for the same fake intercept scid over and over again (for every forward_intercepted_payment try) once we try to forward the htlc, which I think will be confusing for the user as no error will occur (given that I've followed the code correctly).

Edit: Ah I realized now that the scid in the forward_intercepted_payment parameters is of course ment to be another than the short_channel_id in the PaymentIntercepted event. Perhaps this should be clarified in the docs to avoid confusion :). I still think it would make sense to include a short_to_chan_info key check in this function though, to give a clear message to the user rather than failing the forward.

Added this in #1835, good catch IMO!

ViktorTigerstrom · 2022-07-17T23:52:16Z

lightning/src/ln/channelmanager.rs

@@ -3055,6 +3088,63 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		Ok(())
 	}

+	/// Fails the intercepted payment indicated by intercept_id.  This should really only be called in response
+	/// to a PaymentIntercepted event
+	pub fn fail_intercepted_payment(&self, intercept_id: InterceptId) {


I think we'd like to return a Result<(), APIError> for this API function, similar to our other API functions (such as for cases when the pending_intercept didn't exists etc) :)?

ViktorTigerstrom · 2022-07-19T23:27:43Z

lightning/src/ln/channelmanager.rs

 																Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])),
 																Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data, Some(phantom_shared_secret))
 															}
 														},
 														_ => panic!(),
 													}
+												} else if forward_info.amt_incoming.is_some() && fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, short_chan_id) {


I thought of a potential attack vector here that I wanted to raise in the review club earlier today, but unfortunately we didn't have time to go over the question about such vectors:

Shouldn't we check here that the fake_scid is a scid that we have actually generated, and not only that it's within the Intercept namespace?

Else, if another peer is able to generate a fake_scid that we determine to be in the Intercept namespace but that we didn't generate ourselves, we face 2 issues:

Since the PaymentIntercepted event is serialized for peers we have no active channel to (and isn't dropped on disconnect like other events for that type of peers), as well as PendingInterceptedHTLC that aren't handled, random peers we have no channels to could potentially be able cause extensive serialization of huge amount of events/structs.

Probably even worse, just one successful fake_scid attack by a random peer that passes the is_valid_intercept, will serialize the PaymentIntercepted event (which according to comments above may be a required field) + PendingInterceptedHTLC (which has only required fields) for any LDK user, even users that didn't explicitly choose to use the new Intercept HTLC feature. That will break the backwards compatibility for such users, hence opening up for a backwards compatibility attack.

Looking at the implemention for fake_scids + namespaces, it shouldn't be too expensive for random peers to brute force a fake_scids that we will accept as a valid Intercept fake_scid?

I think we therefore also need to add a way to track which fake_scids we have actually generated to not face this issue? I guess one alternative solution to just solve issue (2) would also be to add a config flag for enabling Intercept HTLC support, which defaults to false.

Good points, thanks!! I started a new discussion on the superceding PR here: #1835 (comment).

Re: (1) I'm not sure they could force us to store a ton of stuff since the payments would presumably be failed back as they come in by users that don't support intercept scid forwards (maybe the docs on the event should clarify that you MUST handle it)?

TheBlueMatt · 2022-10-05T04:30:08Z

Let me know when you want additional rounds of review here!

johncantrell97 · 2022-10-05T10:24:08Z

Let me know when you want additional rounds of review here!

Thanks, I just squashed all the old commits to ease the rebase. I'm going to review what I was doing and the last round of comments here. I think there's a few things that were never addressed.

johncantrell97 · 2022-10-05T14:33:26Z

Let me know when you want additional rounds of review here!

This PR is kind of a mess with (outdated) comments already but would love feedback on these questions: https://github.com/lightningdevkit/rust-lightning/pull/1601/files#r987995094

moneyball · 2022-10-13T16:14:08Z

Are we waiting on other contributors to respond to @johncantrell97's questions?

johncantrell97 · 2022-10-13T16:16:44Z

Are we waiting on other contributors to respond to @johncantrell97's questions?

Not at the moment. Think I could use a hint from matt about what existing routing code can be dry'd up now that I moved the new struct into the existing enum.

TheBlueMatt

This basically LGTM, could use some commit separation and a test, though. I don't remember exactly what this code looked like before, but I find it quite nice that we're not adding a new type.

TheBlueMatt · 2022-10-17T18:37:16Z

lightning/src/ln/channelmanager.rs

-	AddHTLC {
-		forward_info: PendingHTLCInfo,
+#[derive(Clone)]
+pub(super) struct PendingAddHTLCInfo {


Can we make this refactor in a separate commit?

TheBlueMatt · 2022-10-17T18:42:49Z

lightning/src/ln/channelmanager.rs

+					};
+
+					let mut per_source_pending_forward = vec![(payment.prev_short_channel_id, payment.prev_funding_outpoint, vec![(pending_htlc_info, payment.prev_htlc_id)])];
+					self.forward_htlcs(&mut per_source_pending_forward);


Ideally we'd actually just do the forward right away rather than waiting a full forwarding-timer-tick. Its not really a huge deal, but waiting the full timer tick kinda sucks for timing analysis reasons.

TheBlueMatt · 2022-10-17T18:43:20Z

lightning/src/util/scid_utils.rs

@@ -91,6 +92,7 @@ pub(crate) mod fake_scid {
 	pub(crate) enum Namespace {
 		Phantom,
 		OutboundAlias,
+		Intercept


Can we add this variant in a separate commit.

moneyball · 2022-11-03T01:11:14Z

What's the next step here?

ariard · 2022-11-03T04:01:13Z

Could be nice to have a look on the pending or future spec objects which would require or be easier to implement with such HTLC intercept interface (e.g trampoline or lightning/bolts#1008). To check if we fit them well.

I still think we should check if this proposed intercept HTLCs interface answers well the use-cases we have in mind such as trampoline/offline receive/jamming mitigation strategies and others. I still think there are previous review conceptual comments not addressed.

TheBlueMatt · 2022-11-04T22:55:36Z

I think the one done here implements it mostly. There are some cases where we want the user to be in-path for all htlc forwards, but I think we can reuse most of the infrastructure here for that. The nice thing about this PR is it gives user control of how much they want to see/control and how much they want LDK to "do for them". A similar concern applies to trampoline - we really want LDK to just "do it for users", its dumb to make users write a lot of boilerplate just to implement trampoline when there's an exact way they have to do it. So, I think we should move this PR forward if possible (@johncantrell97 ?), and we can go from there and add additional extensibility in HTLC forwarding.

valentinewallace · 2022-11-04T23:01:35Z

FYI @TheBlueMatt, as discussed offline with @johncantrell97, I'm almost done with a PR that supercedes this one (containing all of this PR's changes + tests)

valentinewallace · 2022-11-07T22:56:33Z

Superceded by #1835. I think all the feedback here has been addressed but lmk if something's missing

johncantrell97 force-pushed the 2022-07-htlc-intercept branch from b3d8288 to dfa45d6 Compare July 7, 2022 03:21

johncantrell97 commented Jul 7, 2022

View reviewed changes

johncantrell97 force-pushed the 2022-07-htlc-intercept branch 4 times, most recently from 823f566 to 2557243 Compare July 7, 2022 15:51

johncantrell97 force-pushed the 2022-07-htlc-intercept branch 2 times, most recently from b2b0f6f to 312b7cb Compare July 9, 2022 17:56

johncantrell97 marked this pull request as ready for review July 9, 2022 19:44

TheBlueMatt reviewed Jul 10, 2022

View reviewed changes

johncantrell97 force-pushed the 2022-07-htlc-intercept branch from d9e0c45 to c027a24 Compare July 11, 2022 11:16

johncantrell97 commented Jul 11, 2022

View reviewed changes

johncantrell97 force-pushed the 2022-07-htlc-intercept branch from f98ae49 to 28870ff Compare July 11, 2022 14:07

ariard added the Review club label Jul 11, 2022

TheBlueMatt reviewed Jul 11, 2022

View reviewed changes

ariard reviewed Jul 15, 2022

View reviewed changes

ViktorTigerstrom reviewed Jul 18, 2022

View reviewed changes

ViktorTigerstrom reviewed Jul 19, 2022

View reviewed changes

ariard removed the Review club label Jul 27, 2022

johncantrell97 force-pushed the 2022-07-htlc-intercept branch from 28870ff to c42f450 Compare October 5, 2022 01:37

add ability to intercept htlcs

e421b59

johncantrell97 force-pushed the 2022-07-htlc-intercept branch from c42f450 to e421b59 Compare October 6, 2022 01:09

TheBlueMatt reviewed Oct 17, 2022

View reviewed changes

valentinewallace mentioned this pull request Nov 7, 2022

Intercept HTLC forwards for JIT channels #1835

Merged

valentinewallace closed this Nov 7, 2022

intercept htlcs #1601

intercept htlcs #1601

Conversation

johncantrell97 commented Jul 7, 2022

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

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

codecov-commenter commented Jul 7, 2022 • edited Loading

Codecov Report

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

johncantrell97 commented Jul 11, 2022

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

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

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

ariard 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

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

ViktorTigerstrom Jul 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 5, 2022

johncantrell97 commented Oct 5, 2022 • edited Loading

johncantrell97 commented Oct 5, 2022

moneyball commented Oct 13, 2022

johncantrell97 commented Oct 13, 2022

TheBlueMatt 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

moneyball commented Nov 3, 2022

ariard commented Nov 3, 2022

TheBlueMatt commented Nov 4, 2022

codecov-commenter commented Jul 7, 2022 •

edited

Loading

ViktorTigerstrom Jul 17, 2022 •

edited

Loading

ViktorTigerstrom Jul 19, 2022 •

edited

Loading

johncantrell97 commented Oct 5, 2022 •

edited

Loading