Allow nodes to be avoided during pathfinding #1550

tnull · 2022-06-18T14:32:34Z

Users may want to - for whatever reasons - prevent payments to be routed over certain nodes.

This PR therefore allows to add NodeIds to a list of banned nodes, which then will be avoided during path finding.

codecov-commenter · 2022-06-18T14:49:42Z

Codecov Report

Merging #1550 (937ad6f) into main (3676a05) will decrease coverage by 0.02%.
The diff coverage is 92.10%.

❗ Current head 937ad6f differs from pull request most recent head 57d8257. Consider uploading reports for the commit 57d8257 to get more accurate results

@@            Coverage Diff             @@
##             main    #1550      +/-   ##
==========================================
- Coverage   91.04%   91.02%   -0.03%     
==========================================
  Files          80       80              
  Lines       44034    44072      +38     
  Branches    44034    44072      +38     
==========================================
+ Hits        40091    40115      +24     
- Misses       3943     3957      +14

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`96.70% <85.71%> (-0.39%)`	⬇️
lightning/src/routing/router.rs	`92.62% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3676a05...57d8257. Read the comment docs.

lightning/src/routing/scoring.rs

jkczyz · 2022-06-18T15:35:36Z

lightning/src/routing/scoring.rs

@@ -1055,7 +1072,8 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Writeable for Probab
 	#[inline]
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
 		write_tlv_fields!(w, {
-			(0, self.channel_liquidities, required)
+			(0, self.channel_liquidities, required),
+			(1, self.banned_nodes, vec_type),


Do we need/want to serialize these? I'd imagine they can be explicitly added after parsing or passed in as an arg instead. Presumably a list of such nodes is maintained elsewhere.

Right, if we want to go that way we'll want to do it via the config argument, but then I guess we'd still keep the methods to edit the set?

Removed serialization now with 1382696.

I'd say keep the methods but leave it out of the config, as you have it, since its more of a dynamic setting. I guess the question would then be if it should be included in the ReadableArgs parameter.

lightning/src/routing/scoring.rs

jkczyz · 2022-06-18T17:53:36Z

lightning/src/routing/scoring.rs

@@ -1055,7 +1072,8 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Writeable for Probab
 	#[inline]
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
 		write_tlv_fields!(w, {
-			(0, self.channel_liquidities, required)
+			(0, self.channel_liquidities, required),
+			(1, self.banned_nodes, vec_type),


I'd say keep the methods but leave it out of the config, as you have it, since its more of a dynamic setting. I guess the question would then be if it should be included in the ReadableArgs parameter.

tnull · 2022-06-19T13:44:48Z

Squashed commits for now. Let me know if I should re-add some serialization of the banned nodes list, or if it's fine as it is.

jkczyz · 2022-06-20T16:22:01Z

Squashed commits for now. Let me know if I should re-add some serialization of the banned nodes list, or if it's fine as it is.

Argument against serializing the banned nodes is that a crash before serialization would result in the list being inaccurate. So I think it should be maintained independently.

Regarding using the config for the list (#1550 (comment)), we couldn't use a HashSet there since it's not supported in bindings. Using a Vec would work but would require converting to a HashMap internally, so would be odd to have two containers where either the nodes are duplicated or one is empty.

For new / read (#1550 (comment)), we should probably be consistent in what they accept. So if we don't allow passing a banned list in new, then we shouldn't in read either.

TheBlueMatt · 2022-06-20T19:51:21Z

Regarding using the config for the list (#1550 (comment)), we couldn't use a HashSet there since it's not supported in bindings. Using a Vec would work but would require converting to a HashMap internally, so would be odd to have two containers where either the nodes are duplicated or one is empty.

We could expose add/remove methods and the bindings would work fine, we just couldn't expose the HashMap directly.

TheBlueMatt

I would really like to see the map in the config struct - I think it can improve ergonomics a good bit especially for rust users, but being able to set the whole thing up before deserialization instead of after is kinda nice. Its not a big deal though, if we feel strongly to not do that I can live with it. Looks basically good either way.

tnull · 2022-06-21T16:18:04Z

I would really like to see the map in the config struct - I think it can improve ergonomics a good bit especially for rust users, but being able to set the whole thing up before deserialization instead of after is kinda nice. Its not a big deal though, if we feel strongly to not do that I can live with it. Looks basically good either way.

Alright, I have no strong opinion on this.
@jkczyz, do you have any strong objection to this? Otherwise I'd just move it to the config? 🤷‍♂️

jkczyz · 2022-06-21T18:09:44Z

I would really like to see the map in the config struct - I think it can improve ergonomics a good bit especially for rust users, but being able to set the whole thing up before deserialization instead of after is kinda nice. Its not a big deal though, if we feel strongly to not do that I can live with it. Looks basically good either way.

Alright, I have no strong opinion on this. @jkczyz, do you have any strong objection to this? Otherwise I'd just move it to the config? 🤷‍♂️

No strong objections. Having consistency between new and read would be most important for me, which would be the case if the set was in the config. But I believe this would require support for HashSet in the bindings, FWIW.

TheBlueMatt · 2022-06-21T19:39:08Z

But I believe this would require support for HashSet in the bindings, FWIW.

Nah, just have to tag it (C-not exported) and have a "add all from vec" method on the config struct in #[cfg(c_bindings)]

tnull · 2022-06-22T16:22:49Z

So, just to be on the same page about the way forward:

I'll move the HashSet as well as the add/remove/clear methods to a field in UserConfig. As the HashSet initialized empty and will only be filled during runtime, read and new will be consistent.
I'll add a add_all method that can make bindings usage easier and will otherwise tag HashSet (C-not exported).

Does that sound about right? Objections/corrections?

TheBlueMatt · 2022-06-22T16:29:55Z

SGTM.

jkczyz · 2022-06-22T16:34:34Z

So, just to be on the same page about the way forward:

I'll move the HashSet as well as the add/remove/clear methods to a field in UserConfig. As the HashSet initialized empty and will only be filled during runtime, read and new will be consistent.

ProbabilisticScoringParameters, not UserConfig, IIUC. Also, the methods need to remain on the scorer since the user won't have access to the params once the scorer is initialized.

tnull · 2022-06-22T16:52:38Z

So, just to be on the same page about the way forward:

I'll move the HashSet as well as the add/remove/clear methods to a field in UserConfig. As the HashSet initialized empty and will only be filled during runtime, read and new will be consistent.

ProbabilisticScoringParameters, not UserConfig, IIUC. Also, the methods need to remain on the scorer since the user won't have access to the params once the scorer is initialized.

lol, thanks, I was starting to wonder what was referred to as 'config' and how I'd access UserConfig. The params struct makes more sense. Will update accordingly.

tnull · 2022-06-23T08:36:29Z

lightning/src/routing/scoring.rs

 }

 /// Parameters for configuring [`ProbabilisticScorer`].
 ///
 /// Used to configure base, liquidity, and amount penalties, the sum of which comprises the channel
 /// penalty (i.e., the amount in msats willing to be paid to avoid routing through the channel).
-#[derive(Clone, Copy)]


Unfortunately, adding the HashSet here makes this only moveable.

We can leave Clone, no? Just have to drop Copy.

Yes, I of course left Clone. Sorry if that was confusing.

TheBlueMatt · 2022-06-23T15:58:21Z

Still needs to address #1550 (comment) ?

tnull · 2022-06-23T16:03:21Z

Still needs to address #1550 (comment) ?

Ah, thanks, I missed that. Will move (most of) the methods back.

lightning/src/routing/scoring.rs

TheBlueMatt · 2022-06-23T16:34:34Z

Looks like this needs rebase?

tnull · 2022-06-23T16:41:07Z

Rebased!

TheBlueMatt · 2022-06-23T18:31:34Z

LGTM, feel free to squash.

Users may want to - for whatever reasons - prevent payments to be routed over certain nodes. This change therefore allows to add `NodeId`s to a list of banned nodes, which then will be avoided during path finding.

tnull · 2022-06-24T06:33:00Z

Squashed.

TheBlueMatt reviewed Jun 18, 2022

View reviewed changes

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

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

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

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

jkczyz self-requested a review June 18, 2022 15:29

jkczyz reviewed Jun 18, 2022

View reviewed changes

tnull force-pushed the 2022-06-scorer-banlist branch from 5055d9b to cc13d2b Compare June 19, 2022 13:42

TheBlueMatt reviewed Jun 21, 2022

View reviewed changes

TheBlueMatt added this to the 0.0.109 milestone Jun 22, 2022

tnull commented Jun 23, 2022

View reviewed changes

jkczyz reviewed Jun 23, 2022

View reviewed changes

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

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

tnull force-pushed the 2022-06-scorer-banlist branch from c69f400 to 937ad6f Compare June 23, 2022 16:40

Allow nodes to be avoided during pathfinding

57d8257

Users may want to - for whatever reasons - prevent payments to be routed over certain nodes. This change therefore allows to add `NodeId`s to a list of banned nodes, which then will be avoided during path finding.

tnull force-pushed the 2022-06-scorer-banlist branch from 937ad6f to 57d8257 Compare June 24, 2022 06:32

jkczyz approved these changes Jun 24, 2022

View reviewed changes

TheBlueMatt approved these changes Jun 24, 2022

View reviewed changes

TheBlueMatt merged commit 79e2af9 into lightningdevkit:main Jun 24, 2022

TheBlueMatt mentioned this pull request Jun 24, 2022

Add anti-probing penalty to ProbabilisticScorer #1555

Merged

This was referenced Jun 28, 2022

Manual node +channel prioritization #1239

Closed

Allow to set manual node penalties #1592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow nodes to be avoided during pathfinding #1550

Allow nodes to be avoided during pathfinding #1550

tnull commented Jun 18, 2022

codecov-commenter commented Jun 18, 2022 •

edited

Loading

jkczyz Jun 18, 2022

TheBlueMatt Jun 18, 2022

tnull Jun 18, 2022

jkczyz Jun 18, 2022

jkczyz Jun 18, 2022

tnull commented Jun 19, 2022 •

edited

Loading

jkczyz commented Jun 20, 2022

TheBlueMatt commented Jun 20, 2022

TheBlueMatt left a comment

tnull commented Jun 21, 2022

jkczyz commented Jun 21, 2022

TheBlueMatt commented Jun 21, 2022

tnull commented Jun 22, 2022

TheBlueMatt commented Jun 22, 2022

jkczyz commented Jun 22, 2022

tnull commented Jun 22, 2022

tnull Jun 23, 2022

TheBlueMatt Jun 23, 2022

tnull Jun 23, 2022

TheBlueMatt commented Jun 23, 2022

tnull commented Jun 23, 2022

TheBlueMatt commented Jun 23, 2022

tnull commented Jun 23, 2022

TheBlueMatt commented Jun 23, 2022

tnull commented Jun 24, 2022

Allow nodes to be avoided during pathfinding #1550

Allow nodes to be avoided during pathfinding #1550

Conversation

tnull commented Jun 18, 2022

codecov-commenter commented Jun 18, 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

tnull commented Jun 19, 2022 • edited Loading

jkczyz commented Jun 20, 2022

TheBlueMatt commented Jun 20, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull commented Jun 21, 2022

jkczyz commented Jun 21, 2022

TheBlueMatt commented Jun 21, 2022

tnull commented Jun 22, 2022

TheBlueMatt commented Jun 22, 2022

jkczyz commented Jun 22, 2022

tnull commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 23, 2022

tnull commented Jun 23, 2022

TheBlueMatt commented Jun 23, 2022

tnull commented Jun 23, 2022

TheBlueMatt commented Jun 23, 2022

tnull commented Jun 24, 2022

codecov-commenter commented Jun 18, 2022 •

edited

Loading

tnull commented Jun 19, 2022 •

edited

Loading