Add CandidateRouteHop to channel_penalty_msat inputs #2551

jbesraa · 2023-09-04T14:56:26Z

resolves #2509

codecov-commenter · 2023-09-04T17:08:11Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (37150b4) 88.62% compared to head (6d7f156) 88.48%.

❗ Current head 6d7f156 differs from pull request most recent head cbde4a7. Consider uploading reports for the commit cbde4a7 to get more accurate results

Files	Patch %	Lines
lightning/src/routing/scoring.rs	94.14%	22 Missing ⚠️
lightning/src/routing/router.rs	98.29%	1 Missing and 1 partial ⚠️
lightning/src/util/test_utils.rs	93.33%	2 Missing ⚠️
lightning-background-processor/src/lib.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2551      +/-   ##
==========================================
- Coverage   88.62%   88.48%   -0.14%     
==========================================
  Files         115      115              
  Lines       90446    90309     -137     
  Branches    90446    90309     -137     
==========================================
- Hits        80156    79912     -244     
- Misses       7883     7996     +113     
+ Partials     2407     2401       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

Nice! This looks pretty good, thanks for doing this.

TheBlueMatt · 2023-09-05T17:13:14Z

lightning/src/routing/router.rs

 		hint: &'a (BlindedPayInfo, BlindedPath),
+		/// The index of the hint in the original list of blinded hints.


A bunch of the docs could use a bit more detail - in general, docs should list (a) what it is, (b) when and why a user would want to use it, and (c) how a user would use it. For example, here, we should say something about how this exists to give each possible CandidateRouteHop a unique ID for the duration of a given routefinding call.

lightning/src/routing/router.rs

jbesraa · 2023-09-13T05:18:06Z

Apologies for the delay here. I suspect ill only have time to revisit this during the weekend.

TheBlueMatt · 2023-09-14T22:19:04Z

No particular rush. Given how close we are to 0.0.117 we may not get this over the line anyway, so it may need to wait three weeks.

lightning/src/routing/router.rs

jbesraa · 2023-09-18T15:00:25Z

pushed some changes but this still WIP
one of the MPP tests is failing and iam not sure why, any hint about that would be much appreciated.

lightning/src/routing/router.rs

jkczyz · 2023-09-20T18:39:15Z

lightning/src/routing/router.rs

+	/// Returns short_channel_id if known.
+	/// For `FirstHop` we assume `get_outbound_payment_scid` is always set, this assumption is checked in
+	/// `find_route` method.
+	/// For `Blinded` and `OneHopBlinded` we return None because we don't know the channel id.


s/we don't know the channel id/the next hop is blinded

lightning/src/routing/router.rs

jkczyz · 2023-09-20T18:49:23Z

lightning/src/routing/router.rs

+		let target = match candidate.target() {
+			Some(target) => target,
+			None => return 0,
+		};
+		let short_channel_id = match candidate.short_channel_id() {
+			Some(short_channel_id) => short_channel_id,
+			None => return 0,
+		};


I don't think we should return 0 here. Rather, it would be better to have these cases delegate to the underlying scorer just like in the else clause below.

jkczyz · 2023-09-20T19:46:45Z

lightning/src/util/test_utils.rs

+							};
+							let channel = match channel.as_directed_to(&NodeId::from_pubkey(&hop.pubkey)) {
+								Some(channel) => channel,
+								None => continue,


This should be a panic, I think.

jkczyz · 2023-09-20T19:47:55Z

lightning/src/util/test_utils.rs

@@ -1359,4 +1389,4 @@ impl WalletSource for TestWalletSource {
 		}
 		Ok(tx)
 	}
-}
+}


newline needed

jkczyz · 2023-09-20T19:59:41Z

lightning/src/routing/router.rs

@@ -1228,7 +1224,7 @@ fn iter_equal<I1: Iterator, I2: Iterator>(mut iter_a: I1, mut iter_b: I2)
 pub struct PathBuildingHop<'a> {
 	// Note that this should be dropped in favor of loading it from CandidateRouteHop, but doing so
 	// is a larger refactor and will require careful performance analysis.
-	node_id: NodeId,
+	// node_id: NodeId,


Please remove commented out code here and elsewhere. The comment above can also be dropped now.

lightning/src/routing/router.rs

jbesraa · 2023-10-02T16:17:11Z

@jkczyz @TheBlueMatt thanks for the review. I reverted back the last commit (removing node_id_ from PathBuildingHop) as I have a question I need to clear my head around first:
in the above implementation i assume that fn source refers to the current hop that is being examined as a candidate, and the target as the next hop.

well, now after writing it, thats not correct, right?
target should be the examined hop, ie the candidate, and source should refer the hop before it that will link to it if its a good candidate?

jkczyz · 2023-10-03T02:34:22Z

@jkczyz @TheBlueMatt thanks for the review. I reverted back the last commit (removing node_id_ from PathBuildingHop) as I have a question I need to clear my head around first: in the above implementation i assume that fn source refers to the current hop that is being examined as a candidate, and the target as the next hop.

well, now after writing it, thats not correct, right? target should be the examined hop, ie the candidate, and source should refer the hop before it that will link to it if its a good candidate?

From the perspective of scoring, the payment is moving from the source to the target for the hop being considered.

But the routing algorithm is working backwards from the recipient to the sender. When it calls add_entry!, the the source node is passed as $src_node_id and the target as $dest_node_id (perhaps we should rename dest to target). That said, the terminology should be the same as scoring. For instance, you'll notice for first hops, we pass our own node id as the source and our channel counterparty as the target/destination.

Anyhow, that's a long way of saying that the node_id in PathBuildingHop is the target from CandidateRouteHop as seen here:

rust-lightning/lightning/src/routing/router.rs

Line 1860 in 9b1b724

node_id: $dest_node_id.clone(),

You may want to add an assert there to confirm they are the same before removing it.

jbesraa · 2023-10-03T11:48:42Z

thank you @jkczyz
I think I found the problem
The target function in CandidateRouteHop should return maybe_dummy_node_id for Blinded_ hops as this is what the get_route function relays on to terminate the the loop

jkczyz

Sorry about the delay. We were focused on the 117 release and were traveling for an offsite.

Some of these comments will need to be applied wherever relevant.

jkczyz · 2023-10-11T22:21:21Z

lightning/src/routing/gossip.rs

-	channel: &'a ChannelInfo,
+	/// Provides information about a channel between two nodes.
+	pub channel: &'a ChannelInfo,


Does this need to be pub?

jkczyz · 2023-10-11T22:34:52Z

lightning/src/routing/scoring.rs

@@ -2548,32 +2553,40 @@ mod tests {
 			inflight_htlc_msat: 0,
 			effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024_000, htlc_maximum_msat: 1_000 },
 		};
-		assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage, &params), 0);
+		let channel = network_graph.read_only().channel(42).unwrap().to_owned();
+		let (info, target) = channel.as_directed_from(&source).unwrap();


In the scoring tests, the target here is shadowing an earlier target, but neither are used resulting in a build warning.

Looks like this was resolved in some of the other tests, but not here.

jkczyz · 2023-10-11T22:37:22Z

lightning/src/routing/scoring.rs

@@ -2548,32 +2553,40 @@ mod tests {
 			inflight_htlc_msat: 0,
 			effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024_000, htlc_maximum_msat: 1_000 },
 		};
-		assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage, &params), 0);
+		let channel = network_graph.read_only().channel(42).unwrap().to_owned();


Could we avoid the to_owned here by adding another variable for the read-only graph? Otherwise, this is performing a clone.

In some places I was able to take out the to_owned but in others, I couldnt as I got an error saying the lock is not free. how is that possible when we are using RwLock, cant multiple functions read from the lock in the same time?

You'll need something like this:

let network_graph = network_graph.read_only(); let channel = network_graph.channel(42).unwrap();

Hard to say why it would sometimes work without the extra variable without seeing an example.

jkczyz · 2023-10-11T22:38:04Z

lightning/src/routing/scoring.rs

+				source_node_id: node_a,
+				target_node_id: *target
+		};
+		// assert_eq!(scorer.channel_penalty_msat(42, &node_a, &node_b, usage, &params), 128);


Please remove commented out code.

jkczyz · 2023-10-11T22:40:26Z

lightning/src/routing/scoring.rs

+
+		let channel = network_graph.read_only().channel(42).unwrap().to_owned();
+		let (info, target) = channel.as_directed_from(&node_a).unwrap();
+		let candidate: CandidateRouteHop = CandidateRouteHop::PublicHop {


Shouldn't need the type declaration.

jkczyz · 2023-10-11T22:43:14Z

lightning/src/routing/scoring.rs

+				liquidity_penalty_multiplier_msat: 1_000,
+				liquidity_penalty_amount_multiplier_msat: 0,
+				..ProbabilisticScoringFeeParameters::zero_penalty()


yea will go through the indentation problems.. I started this pr on a linux machine and now trying to finish it on a mac and the linting is driving me crazy (: cant wait for #2648!

jkczyz · 2023-10-11T22:48:33Z

lightning/src/routing/router.rs

+		/// `BlindedPayInfo` provides information needed about the
+		/// payment while routing through a blinded
+		/// path.


Could you wrap all docs at 100 characters?

max 100 chars in a single line?

Yeah, though I guess now that is defined with tabs may be ambiguous. I just go with the smallest number vim gives.

Note that we aren't really consistent with that currently with long lines. Though, I don't think we should deliberately make the length of comments shorter.

jkczyz · 2023-10-11T22:51:09Z

lightning/src/routing/router.rs

 	},
 	/// The payee's identity is concealed behind blinded paths provided in a BOLT 12 invoice.
 	Blinded {
+		/// Hint provides information about a blinded hop, needed while routing through a blinded
+		/// channel.


s/channel/path

jkczyz · 2023-10-11T22:54:20Z

lightning/src/routing/router.rs

+		/// The node id of current hop in route.
+		///
+		/// This will always equal to `maybe_dummy_payee_node_id` in `find_route` method.
+		target_node_id: NodeId,


We shouldn't reference maybe_dummy_payee_node_id in the docs given it is a local variable. Why do we need a field for this anyway if it could be a constant?

Also, it seems this a routing concern whereas it shouldn't be applicable to scoring. Given we are using the struct in two contexts, I think we need to decide if we want to expose the constant or use an Option. @TheBlueMatt Any thoughts on this?

Yea, we should simply remove it and expose the target as an option. We may want to have a private version that always returns the dummy that is only used in router.rs

ok few questions here because this thing is a kinda big.
add_entry! macro have also target and source inputs as well as Candidate - after adding fn target and fn source to Candidate, we should remove the source and target from add_entry! correct?
regarding maybe_dummy_payee_node_id, should I always run candidate.target().unwrap_or(maybe_dummy_payee_node_id) ?
we have this bit
if ordered_hops.last().unwrap().0.candidate.target().unwrap() == maybe_dummy_payee_node_id
we cant keep it like this as its not correct, should I do if ....candidate.target().is_none().... ?

last question (:
the id function takes a direction as an input which is sourc < target should I keep that input as is or make it optional? because we have some places in the code were we call it with source < target but other places where we just give true/false..

pushed some code with the stuff I mentioned in prev comment^.. please let me know what u think

the question regarding the id function is because we dont need to provide source < target now because Candidate has this info (see code please)

ok few questions here because this thing is a kinda big. add_entry! macro have also target and source inputs as well as Candidate - after adding fn target and fn source to Candidate, we should remove the source and target from add_entry! correct?

Ideally, yes, though there is the blinded case that needs to be considered, as you mention below.

regarding maybe_dummy_payee_node_id, should I always run candidate.target().unwrap_or(maybe_dummy_payee_node_id) ? we have this bit if ordered_hops.last().unwrap().0.candidate.target().unwrap() == maybe_dummy_payee_node_id we cant keep it like this as its not correct, should I do if ....candidate.target().is_none().... ?

maybe_dummy_payee_node_id always seemed a little hacky to me. @TheBlueMatt Is there any reason we can't check against None instead?

last question (: the id function takes a direction as an input which is sourc < target should I keep that input as is or make it optional? because we have some places in the code were we call it with source < target but other places where we just give true/false..

Huh, it's kinda error-prone that it takes a direction in the first place given the source and target relationship is what defines the direction (i.e., a candidate's direction, and thus id, should never change). So someone could mix it up if they need to pass it in. Seems like the direction could be derived from the candidate now, though.

Looks like we pass in true or false when we need a CandidateHopId in both directions, though we could add a method to CandidateHopId that gives the CandidateHopId for the opposite direction. That way, we should never need to pass in a direction to obtain a candidate's id.

Also, we use true unilaterally in some places, which seems wrong? Or maybe it just doesn't matter at that point and we need some id, if not the most accurate one?

maybe_dummy_payee_node_id always seemed a little hacky to me. @TheBlueMatt Is there any reason we can't check against None instead?

I think we largely could. Now that we're moving away from storing a destination in the PathBuildingHop we could drop it and use an option. Its still a bit strange to use an option, though, imo - we're then using None to mean "the blinded path introduction point(s)", which is fine, but that needs to mean only that, if we ever have some other use for a None target that would conflict. Thus I don't really have a strong feeling on changing it, though if we dont we should have an internal-only version of target which returns the dummy rather than doing it at the callsite.

I see... yeah, hard to say what's best. Having an internal target function returning NodeId instead of Option<NodeId> may be better so it's all defined in one place then (opposed to unwrap_or(maybe_dummy_payee_node_id)). But there are places where we expect the candidate to not be blinded, so it's questionable if we should be replacing those with maybe_dummy_payee_node_id. I left some comments where this may be the case or where maybe_dummy_payee_node_id isn't strictly necessary. @jbesraa Let's see what remains after addressing those comments.

jkczyz · 2023-10-11T23:09:28Z

lightning/src/routing/router.rs

+	///  Returns the id of this hop.
+	///  For `Blinded` and `OneHopBlinded` we return `CandidateHopId::Blinded` because we don't know the channel id.
+	///  For any other option we return `CandidateHopId::Clear` because we know the channel id and the direction.
+	pub fn id(&self, channel_direction: bool /* src_node_id < target_node_id */) -> CandidateHopId {


Does this need to be pub? If so, we should document channel_direction in the method docs. I'd rather not expose this detail if we don't need to, though.

If I am not mistaken, @TheBlueMatt wanted this to be pub?

Ah, is that referring to #2551 (comment) ? I'd at least like to rid ourselves of the direction parameter if we want to expose it for some reason.

I think I'd suggested making it pub since I was thinking we just make everything pub. But, I think @jkczyz is right here, there's not a real use for this for scorer users.

jbesraa · 2023-11-20T16:27:46Z

rearranged commits and fixed up some, hopefully its a bit nicer to review

jbesraa · 2023-11-21T13:49:27Z

lightning/src/routing/scoring.rs

-//! # fn find_scored_route(payer: PublicKey, route_params: RouteParameters, network_graph: NetworkGraph<&FakeLogger>) {
-//! # let logger = FakeLogger {};
-//! #
+//!  extern crate bitcoin;


I had to enable this test as it was failing for me locally when running ci checks.. should I move this to a different commit?

The # marks simply hide the code from the rust docs. It shouldn't affect the test result, so it should remain as it was. Do you have the failure message?

jbesraa · 2023-11-21T15:36:50Z

@jkczyz thank for the ongoing review!
I cleared the commits and fixed all the small nits spotted.
It looks like its getting there, please give it another review when you can.

jbesraa · 2023-11-22T12:57:58Z

rebased against main, no changes

jbesraa · 2023-11-22T15:46:16Z

pushed with verified commits, no code changes

jkczyz

Thanks for sticking with this! I think this is shaping up.

jkczyz · 2023-11-27T22:50:17Z

lightning/src/routing/router.rs

@@ -993,44 +993,74 @@ impl cmp::PartialOrd for RouteGraphNode {

 /// A wrapper around the various hop representations.
 ///
-/// Used to construct a [`PathBuildingHop`] and to estimate [`EffectiveCapacity`].
+/// Used to construct a `PathBuildingHop` and to estimate [`EffectiveCapacity`].


Let's updated the docs to not refer to an internal struct and instead talk to how it is used externally.

jkczyz · 2023-11-27T22:51:32Z

lightning/src/routing/router.rs

+	///  Returns the id of this hop.
+	///  For `Blinded` and `OneHopBlinded` we return `CandidateHopId::Blinded` with `hint_idx` because we don't know the channel id.
+	///  For any other option we return `CandidateHopId::Clear` because we know the channel id and the direction.
+	pub fn id(&self) -> CandidateHopId {


Did we settle on keeping this private?

Personally, I don't care, as long as it doesn't have the second argument, which it now doesn't, which is nice. It does seem like it could be useful, but its also kinda confusing that its only valid on a per-routefinding-basis, rather than globally.

jkczyz · 2023-11-27T22:52:18Z

lightning/src/routing/router.rs

+		/// Hint provides information about a blinded hop,
+		/// needed while routing through a blinded path.  `BlindedPayInfo`
+		/// provides information needed about the
+		/// payment while routing through a blinded path.
+		/// `BlindedPath` is the blinded path to the destination.


nit: Here and elsewhere, let's try to wrap comments at 100 characters.

jkczyz · 2023-11-27T22:53:27Z

lightning/src/routing/router.rs

 		hint_idx: usize,
 	},
 	/// Similar to [`Self::Blinded`], but the path here has 1 blinded hop. `BlindedPayInfo` provided
 	/// for 1-hop blinded paths is ignored because it is meant to apply to the hops *between* the
 	/// introduction node and the destination. Useful for tracking that we need to include a blinded
 	/// path at the end of our [`Route`].
 	OneHopBlinded {
+		/// Hint provides information about a single blinded hop,
+		/// needed while routing through a blinded channel. `BlindedPayInfo` is ignored here.


s/blinded channel/one-hop blinded path

jkczyz · 2023-11-27T22:57:52Z

lightning/src/routing/router.rs

+	/// Source node id refers to the hop that is previous
+	/// to the current examined hop in route finding.


Instead of phrasing in terms of route finding -- which actually starts at the recipient and works backwards -- let's phrase in terms of scoring a channel (i.e., in the direction of the payment).

jkczyz · 2023-11-27T23:10:17Z

lightning/src/routing/router.rs

-			if src_node_id != dest_node_id {
+			if Some($candidate.source()) != $candidate.target() {


Why change this in this commit (or at all)?

jkczyz · 2023-11-27T23:10:39Z

lightning/src/routing/router.rs

-								// Update the way of reaching $src_node_id with the given short_channel_id (from $dest_node_id),
+								// Update the way of reaching $candidate.source() with the given short_channel_id (from $candidate.target()),


Likewise, should be in an earlier commit.

lightning/src/routing/router.rs

jkczyz · 2023-11-27T23:14:06Z

lightning/src/routing/scoring.rs

@@ -2548,32 +2553,40 @@ mod tests {
 			inflight_htlc_msat: 0,
 			effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024_000, htlc_maximum_msat: 1_000 },
 		};
-		assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage, &params), 0);
+		let channel = network_graph.read_only().channel(42).unwrap().to_owned();
+		let (info, target) = channel.as_directed_from(&source).unwrap();


Looks like this was resolved in some of the other tests, but not here.

jkczyz · 2023-11-27T23:15:09Z

lightning/src/routing/router.rs

-							// as a way to reach the $dest_node_id.
+							// as a way to reach the target node id.


Please update in an earlier commit.

jbesraa · 2023-12-02T16:16:37Z

@jkczyz pushed updates to nits + fixed the git history

TheBlueMatt · 2023-12-04T23:30:18Z

Needs rebase, sorry about that. We should be able to do any remaining cleanups in followups, though.

If `outband` flag is set to true then `ChannelInfo::node_one` is forwarding a payment to target `ChannelInfo::node_two`. If `outband` flag is set to false then `ChannelInfo::node_two` is forwarding a payment to target `ChannelInfo::node_one`.

`DirectedChannelInfo::source` return the `node_id` of the forwarding hop. `DirectedChannelInfo::target` return the `node_id` of the destination hop.

We add `CandidateRouteHop::source` and `CandidateRouteHop::source` functions to point to current and next hops in route respectively. As we have now `source` and `target` available in `CandidateRouteHop` we also remove `CandidateRouteHop::id` inputs so now they are consumed from `self.target` and `self.source` functions. In the `add_entry` macro we also remove `source` and `target` arguments in favor of `candidate` of type `CandidateRouteHop` that holds the needed info.

Change `short_channel_id`, `cltv_expiry_delta` `fees` and `htlc_minimum_msat` under `CandidateRouteHop` visibility to public and add documentation.

We remove `source`, `target` and `scid` from `channel_penalty_msat` inputs to consume them from `candidate` of type `CandidateRouteHop`

We remove `node_id` from `PathBuildingHop` as we can rely `CandidateRouteHop::target` instead.

TheBlueMatt

Some of the new docs could use a bit more clarity, but I think the code is all correct and we should land it and do more in a followup.

lightning/src/routing/gossip.rs

TheBlueMatt · 2023-12-05T22:05:53Z

lightning/src/util/test_utils.rs

+								details: first_hops[idx],
+								node_id: NodeId::from_pubkey(payer)
+							};
+							scorer.channel_penalty_msat(&candidate, usage, &());


The point here is to ensure that the channel_penalty_msat calls mirror those of normal routing operations (at least if we were to select the first path we find). Here you've changed it so that we only call channel_penalty_msat on first-hops, which I don't think accomplishes that goal.

Good catch here. Fortunately, we only utilize TestScorer in one test currently with announced channels. Below it's probably more accurate to fall back to PrivateHop if the NetworkGraph does not contain the channel, too.

jkczyz

We can leave these comments for a follow-up.

jkczyz · 2023-12-06T00:45:32Z

lightning/src/routing/gossip.rs

+	/// Returns the `node_id` of the source hop.
+	///
+	/// Refers to the `node_id` forwarding the payment to the next hop.


Let's just combine this as:

/// Returns the `node_id` forwarding the payment to the next hop.

jkczyz · 2023-12-06T00:45:50Z

lightning/src/routing/gossip.rs

+	/// Returns the `node_id` of the target hop.
+	///
+	/// Refers to the `node_id` receiving the payment from the previous hop.


jkczyz · 2023-12-06T00:46:46Z

lightning/src/routing/router.rs

+	/// Returns the source node id of current hop.
+	///
+	/// Source node id refers to the hop forwarding the payment.


Likewise here and in target docs.

TheBlueMatt · 2023-12-06T01:20:24Z

Will do a quick followup to address various comments and play with CandidateRouteHop size reductions.

Doc and performance followups to #2551

jbesraa mentioned this pull request Sep 4, 2023

Introduce &RoutingFees to channel_penalty_msat #2510

Closed

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 3 times, most recently from 208e443 to 22f9a75 Compare September 4, 2023 15:49

TheBlueMatt reviewed Sep 5, 2023

View reviewed changes

jkczyz reviewed Sep 8, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 3 times, most recently from 4bd2037 to ffd7463 Compare September 18, 2023 14:33

jbesraa commented Sep 18, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jbesraa commented Sep 18, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch from ffd7463 to 28f7b42 Compare September 19, 2023 11:53

jkczyz reviewed Sep 20, 2023

View reviewed changes

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 4 times, most recently from 262ac35 to 6ca3010 Compare October 2, 2023 16:09

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 3 times, most recently from 8e4aa27 to 1bc8326 Compare October 3, 2023 09:41

jkczyz reviewed Oct 11, 2023

View reviewed changes

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 2 times, most recently from 122ca61 to 1c0c656 Compare October 22, 2023 17:29

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch from 75309a9 to f9fac0b Compare November 20, 2023 16:20

jbesraa commented Nov 21, 2023

View reviewed changes

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 2 times, most recently from 8fc961a to 8383e3f Compare November 21, 2023 15:28

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch from 8383e3f to 38c5592 Compare November 22, 2023 12:56

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch from 38c5592 to 2cfff78 Compare November 22, 2023 15:45

jkczyz reviewed Nov 27, 2023

View reviewed changes

TheBlueMatt mentioned this pull request Nov 28, 2023

Some thoughts on payment splitting heuristics for multi part payments #2276

Open

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch 4 times, most recently from b851626 to 6d7f156 Compare December 2, 2023 13:49

jbesraa added 6 commits December 5, 2023 21:07

Add outbound flag to DirectedChannelInfo

ec95e58

If `outband` flag is set to true then `ChannelInfo::node_one` is forwarding a payment to target `ChannelInfo::node_two`. If `outband` flag is set to false then `ChannelInfo::node_two` is forwarding a payment to target `ChannelInfo::node_one`.

Add source and target to DirectedChannelInfo

366d688

`DirectedChannelInfo::source` return the `node_id` of the forwarding hop. `DirectedChannelInfo::target` return the `node_id` of the destination hop.

Change CandidateRouteHop functions visbility

04e93fc

Change `short_channel_id`, `cltv_expiry_delta` `fees` and `htlc_minimum_msat` under `CandidateRouteHop` visibility to public and add documentation.

Use CandidateRouteHop as input for channel_penalty_msat

f0ecc3e

We remove `source`, `target` and `scid` from `channel_penalty_msat` inputs to consume them from `candidate` of type `CandidateRouteHop`

Remove node_id from PathBuildingHop

cbde4a7

We remove `node_id` from `PathBuildingHop` as we can rely `CandidateRouteHop::target` instead.

jbesraa force-pushed the expose-CandidateRouteHop-to-channel_penalty_msat branch from 6d7f156 to cbde4a7 Compare December 5, 2023 19:08

TheBlueMatt approved these changes Dec 5, 2023

View reviewed changes

jkczyz approved these changes Dec 6, 2023

View reviewed changes

TheBlueMatt merged commit becdf6f into lightningdevkit:main Dec 6, 2023
15 checks passed

tnull added a commit that referenced this pull request Dec 8, 2023

Merge pull request #2774 from TheBlueMatt/2023-12-2551-followups

e94af0c

Doc and performance followups to #2551

		hint: &'a (BlindedPayInfo, BlindedPath),
		/// The index of the hint in the original list of blinded hints.

@@ @@ -1359,4 +1389,4 @@ impl WalletSource for TestWalletSource { @@
               		}
               		Ok(tx)
               	}
-              }
+              }

		/// Source node id refers to the hop that is previous
		/// to the current examined hop in route finding.

		if src_node_id != dest_node_id {
		if Some($candidate.source()) != $candidate.target() {

		// Update the way of reaching $src_node_id with the given short_channel_id (from $dest_node_id),
		// Update the way of reaching $candidate.source() with the given short_channel_id (from $candidate.target()),

		// as a way to reach the $dest_node_id.
		// as a way to reach the target node id.

Add CandidateRouteHop to channel_penalty_msat inputs #2551

Add CandidateRouteHop to channel_penalty_msat inputs #2551

Conversation

jbesraa commented Sep 4, 2023 • edited Loading

codecov-commenter commented Sep 4, 2023 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Sep 13, 2023

TheBlueMatt commented Sep 14, 2023

jbesraa commented Sep 18, 2023

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

jbesraa commented Oct 2, 2023

jkczyz commented Oct 3, 2023

jbesraa commented Oct 3, 2023

jkczyz 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

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

jbesraa Oct 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz Oct 24, 2023 • 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

jbesraa commented Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Nov 21, 2023

jbesraa commented Nov 22, 2023 • edited Loading

jbesraa commented Nov 22, 2023

jkczyz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Dec 2, 2023 • edited Loading

TheBlueMatt commented Dec 4, 2023 • edited Loading

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

jkczyz 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

TheBlueMatt commented Dec 6, 2023

jbesraa commented Sep 4, 2023 •

edited

Loading

codecov-commenter commented Sep 4, 2023 •

edited

Loading

jbesraa Oct 22, 2023 •

edited

Loading

jkczyz Oct 24, 2023 •

edited

Loading

jbesraa commented Nov 20, 2023 •

edited

Loading

jbesraa commented Nov 22, 2023 •

edited

Loading

jbesraa commented Dec 2, 2023 •

edited

Loading

TheBlueMatt commented Dec 4, 2023 •

edited

Loading