Add the option on path creator to specify the incoming channel on blinded path #9127

MPins · 2024-09-21T04:16:26Z

Change Description

Add the option on path creator to specify the incoming chained channel list

Steps to Test

lncli addinvoice --blind --blinded_path_incoming_channel_list channel_id1 amount
lncli addinvoice --blind --blinded_path_incoming_channel_list channel_id1,channel_id2 amount

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

MPins · 2024-09-21T04:22:06Z

Hello @ellemouton if you think I'm on the right way I can add the option to choose a channel also.

MPins · 2024-09-22T13:42:18Z

Maybe change the name to "incoming_node" is a better idea! But I'll wait for others opinion also.

ellemouton · 2024-09-23T01:16:34Z

thanks @MPins - in tokyo at the moment for the LN summit so will take a look at this a bit later this week or next week

ziggie1984 · 2024-10-03T20:14:21Z

I think it makes sense to just specify the incoming channel, I don't see a lot of use cases for the whole blinded route, but maybe we already prepare the code so that it will be easy to just upgrade to the whole blinded path if it has benefits.

ziggie1984

Thank you for your first contribution 🎉

Please squash all commits related to the lnrpc change into one.

Moreover while reviewing this PR I found out that we have a bug in your probabiliy calculation. We basically should have the prob. of full certainty for the last hop however we need to change the logic in our probability estimator which still seems to not account for the blinded usecase see here (only when from == Self do we set the full local probability, that needs to now be inverted for the last hop as well):

lnd/routing/missioncontrol.go

Lines 509 to 526 in 2d33317

    
           func (m *MissionControl) GetProbability(fromNode, toNode route.Vertex, 
        
           	amt lnwire.MilliSatoshi, capacity btcutil.Amount) float64 { 
        
           	m.mu.Lock() 
        
           	defer m.mu.Unlock() 
        
           	now := m.cfg.clock.Now() 
        
           	results, _ := m.state.getLastPairResult(fromNode) 
        
           	// Use a distinct probability estimation function for local channels. 
        
           	if fromNode == m.cfg.selfNode { 
        
           		return m.estimator.LocalPairProbability(now, results, toNode) 
        
           	} 
        
           	return m.estimator.PairProbability( 
        
           		now, results, toNode, amt, capacity, 
        
           	) 
        
           }

lnrpc/lightning.proto

routing/pathfind.go

cmd/commands/cmd_invoice.go

MPins · 2024-10-04T02:50:19Z

I think it makes sense to just specify the incoming channel, I don't see a lot of use cases for the whole blinded route, but maybe we already prepare the code so that it will be easy to just upgrade to the whole blinded path if it has benefits.

In most cases it would be sufficient to specify the node, the channel must be specified in cases where there is more than one channel with the same node. Of course, specifying the channel covers both use cases. I'm not sure if we should keep both options for the user, perhaps it would be more intuitive to specify the income node for most users.

ziggie1984 · 2024-10-04T09:57:12Z

I prefer the channel, because than you have more control rather than the pubkey. But at the end not sure if its really worth it to treat different channels to the same peer differently because we have non-strict forwarding. So probably both options are ok ...

MPins · 2024-10-07T12:58:32Z

Thank you for your first contribution 🎉

Please squash all commits related to the lnrpc change into one.

Moreover while reviewing this PR I found out that we have a bug in your probabiliy calculation. We basically should have the prob. of full certainty for the last hop however we need to change the logic in our probability estimator which still seems to not account for the blinded usecase see here (only when from == Self do we set the full local probability, that needs to now be inverted for the last hop as well):

lnd/routing/missioncontrol.go

Lines 509 to 526 in 2d33317

func (m *MissionControl) GetProbability(fromNode, toNode route.Vertex,

amt lnwire.MilliSatoshi, capacity btcutil.Amount) float64 {

m.mu.Lock()

defer m.mu.Unlock()

now := m.cfg.clock.Now()

results, _ := m.state.getLastPairResult(fromNode)

// Use a distinct probability estimation function for local channels.

if fromNode == m.cfg.selfNode {

return m.estimator.LocalPairProbability(now, results, toNode)

}

return m.estimator.PairProbability(

now, results, toNode, amt, capacity,

)

}

Thank you for your carefully revision. I'll be working on it some time on the following days. About the probability, I invest some time trying to understanding it, but I didn't get the bug on it! From my understanding, probability is about sending the payment, right? How the payment node would even know that we are choosing the income channel/node? I'm probably missing something, I would appreciate if you give me directions to expand my understanding.

MPins · 2024-10-07T13:37:56Z

I prefer the channel, because than you have more control rather than the pubkey. But at the end not sure if its really worth it to treat different channels to the same peer differently because we have non-strict forwarding. So probably both options are ok ...

I think we can move forward with both options, but in my opinion they should be exclusive. I mean, if the channel is chosen, the node cannot be chosen. (just to avoid redundant info)

ziggie1984 · 2024-10-08T06:29:07Z

So my initial idea was, that we can know the probability of the last channel in a blinded path with very high accuracy because its our own channel and we know the liquidity distribution when creating the blinded path. However looking into the codebase I think we should keep it as is, we do allow MPP payments for blinded paths so the amount can be splitted among the incoming channels in general. Moreover when we specify the incoming channel to receive on (via this PR) we already made the decision that this is the right channel to receive on the full amount, so maybe we can keep the probability estimation of the last hop/channel as is. Given the fact that we include all routes with a minimum route prob. of 1% we are good as is I think.

MPins · 2024-10-08T09:26:11Z

So my initial idea was, that we can know the probability of the last channel in a blinded path with very high accuracy because its our own channel and we know the liquidity distribution when creating the blinded path. However looking into the codebase I think we should keep it as is, we do allow MPP payments for blinded paths so the amount can be splitted among the incoming channels in general. Moreover when we specify the incoming channel to receive on (via this PR) we already made the decision that this is the right channel to receive on the full amount, so maybe we can keep the probability estimation of the last hop/channel as is. Given the fact that we include all routes with a minimum route prob. of 1% we are good as is I think.

Got it ... besides that, as it is blinded I can't think a way of considering that it is our own node without breaking the main reason for using a blinded path, that is not revealing our own node on a invoice.

ziggie1984 · 2024-10-08T09:54:21Z

Got it ... besides that, as it is blinded I can't think a way of considering that it is our own node without breaking the main reason for using a blinded path, that is not revealing our own node on a invoice.

Hmm not sure what you mean, but I was just referring to the creator of the blinded path taking the incoming channel distribution directly into account without really relying on the MC data. The path would still be blinded for the sender so the sender would just have a sorted list of blinded paths in the bolt11 invoice.

MPins · 2024-10-08T10:00:29Z

Got it ... besides that, as it is blinded I can't think a way of considering that it is our own node without breaking the main reason for using a blinded path, that is not revealing our own node on a invoice.

Hmm not sure what you mean, but I was just referring to the creator of the blinded path taking the incoming channel distribution directly into account without really relying on the MC data. The path would still be blinded for the sender so the sender would just have a sorted list of blinded paths in the bolt11 invoice.

Never mind ... I was thinking that the GetProbability func would be called when the payment is being processed by the sender.

MPins · 2024-10-08T12:10:52Z

So my initial idea was, that we can know the probability of the last channel in a blinded path with very high accuracy because its our own channel and we know the liquidity distribution when creating the blinded path. However looking into the codebase I think we should keep it as is, we do allow MPP payments for blinded paths so the amount can be splitted among the incoming channels in general. Moreover when we specify the incoming channel to receive on (via this PR) we already made the decision that this is the right channel to receive on the full amount, so maybe we can keep the probability estimation of the last hop/channel as is. Given the fact that we include all routes with a minimum route prob. of 1% we are good as is I think.

I just saw that it is called by FindBlindedPaths ... Thank you, I have a better understanding of the whole process now.

I think you right ... maybe it should be changed ... not sure if it should be included in this PR or maybe on a specific one for that.

go.mod

saubyk · 2024-10-13T17:25:55Z

Have a basic question here.
The problem statement in the linked issue is regarding specifying an incoming channel...

to have better control over payment success probability
to better control the liquidity distribution available on the node

So why are we then giving the user the control of selecting a node in the cli, instead of directly specifying a channel?
I believe the user should only specify the channel not the node (which I find confusing)

MPins · 2024-10-13T19:25:37Z

Have a basic question here. The problem statement in the linked issue is regarding specifying an incoming channel...

to have better control over payment success probability

to better control the liquidity distribution available on the node

So why are we then giving the user the control of selecting a node in the cli, instead of directly specifying a channel? I believe the user should only specify the channel not the node (which I find confusing)

The idea to have the option to specify the node instead of the channel is because in the most cases it would be enough and from the node runner perspective it would be more intuitive specifying just the income node.

ziggie1984 · 2024-10-13T20:26:05Z

So why are we then giving the user the control of selecting a node in the cli, instead of directly specifying a channel?
I believe the user should only specify the channel not the node (which I find confusing)

Good question, I also tend to only support the incoming channel id why:

When creating the blinded path we do not create a Unified Edge meaning that we do NOT take all the different policies for all the channels with the same peer into account. Moreover we cannot be sure that all policies are the same, other implementations iirc can have different policies for different channels, therefore I recommend only using the channel ID here.
Although the lightning network supports non-strict forwarding which means the forwarder can basically select another channel if he wants to, but as mentioned before, we are not unifying the edges to create the worst case policy to the last node. So I think we should stick to the incoming channel id here and should not allow the node id.

ziggie1984 · 2024-10-13T20:47:09Z

Imagine your peer has 2 channels with you and different policies, now when creating the blinded path and using the nodeid, you cannot control the particular channel and the sender will probably use the route with the better constraints. We already have something similar when sending a payment where we can select the outgoing channel. We should prob. stick to this form.

MPins · 2024-10-13T21:14:34Z

I think that

Imagine your peer has 2 channels with you and different policies, now when creating the blinded path and using the nodeid, you cannot control the particular channel and the sender will probably use the route with the better constraints. We already have something similar when sending a payment where we can select the outgoing channel. We should prob. stick to this form.

When sending payment you can also choose the last hop (penultimate node in the path) to route through for the payment, but it is other use case. So I think you both are right, I'm going to take the node option out.

ziggie1984 · 2024-10-13T21:20:27Z

Good observation that we also allow the last hop in the payment flow, but LND does unify the edges in the sending flow and selects the most expensive policy that's why it makes sense to have this last hop setting.

MPins · 2024-10-13T21:34:06Z

Good observation that we also allow the last hop in the payment flow, but LND does unify the edges in the sending flow and selects the most expensive policy that's why it makes sense to have this last hop setting.

Perfect ... I'm going to take the node option out. Thank you @saubyk for starting this exchange of ideas.

saubyk · 2024-10-13T21:37:54Z

Good observation that we also allow the last hop in the payment flow, but LND does unify the edges in the sending flow and selects the most expensive policy that's why it makes sense to have this last hop setting.

Perfect ... I'm going to take the node option out. Thank you @saubyk for starting this exchange of ideas.

Thanks. Not to add the confusion, but I do understand the logic of selecting the node from a UX standpoint and especially when you imagine a user doing it in a UI. Selecting a pub key (with an alias) is much easier than selecting a channel ID. But I think that problem can be addressed at the application level, where the UI can establish that node-channel linkage and present a user friendly way for the user to make a selection.

ellemouton · 2024-10-16T09:28:21Z

I prefer the channel, because than you have more control rather than the pubkey.

The problem is that you dont actually every have this control due to non-strict forwarding. At the end of the day, the peer may choose any channel to fwd on as long as the peer on the other end is the same peer.

Regarding the impl here and if we only want to specify final hop or list of hops, I think things should be made general enough for both:

so if user specifies 1 peer, then LND takes that as a fixed final hop and uses its current logic to build on that given the other config values
use can specify a list of hops and LND will just do the same thing as above.

MPins · 2024-10-16T10:37:31Z

use can specify a list of hops and LND will just do the same thing as above.

The way it is done here the user can specify the last hop many times (peers or channels, not both). If I understand you correctly it should be kept this way. Right?

ziggie1984 · 2024-10-16T11:04:41Z

@ellemouton what's your take on this, we cannot really know whether peers have a universal policy to us?

Imagine your peer has 2 channels with you and different policies, now when creating the blinded path and using the nodeid, you cannot control the particular channel and the sender will probably use the route with the better constraints. We already have something similar when sending a payment where we can select the outgoing channel. We should prob. stick to this form.

MPins · 2025-04-24T18:21:51Z

Hello @ziggie1984 , the pending nits have been addressed. Thanks so much for your time and review!

ziggie1984

LGTM 💪🥇

ellemouton

Thanks for the updates here @MPins 🙏

While I think the logic works, the quality of the PR is not up to scratch:

the logic is currently quite spread out and not as simple/contained as it can be.
The commit structure is not great - making the PR difficult to review
Our code formatting styles have not been followed in many places.

I've put together this draft/demo PR for you to take a look at. The main thing I show here is how I would go about 1) implementing the logic change and 2) structuring the commits
(it's obvs a draft and so is missing some things & youd need to still add on the itest commits - but i hope this can help you see what we're looking for here). Feel free to let me know if you have any questions 🙏

lncfg/routing.go

lnrpc/invoicesrpc/invoices.swagger.json

lnrpc/lightning.proto

lntest/rpc/lnd.go

routing/router.go

routing/pathfind.go

routing/blindedpath/blinded_path.go

routing/pathfind_test.go

routing/pathfind.go

MPins · 2025-04-28T03:41:57Z

@ellemouton thank you so much for your time and careful review. You and @ziggie1984 are helping me a lot.

I have addressed all the comments, and I believe the PR is now much easier to review. 🙏

ziggie1984 · 2025-04-28T06:47:18Z

routing/pathfind.go

+	for _, chanID := range restrictions.incomingChainedChannels {
+		visited[nextTarget] = true
+
+		err := g.ForEachNodeDirectedChannel(nextTarget,


Looking at the implementation of func (c *ChannelGraph) FetchChannelEdgesByID(chanID uint64) ( I would appreciate an explanation why we are not implemented the graphCache there cc @ellemouton

also keep in mind there is currently this issue regarding the cache: #9465

Looking at the implementation of func (c *ChannelGraph) FetchChannelEdgesByID(chanID uint64) ( I would appreciate an explanation why we are not implemented the graphCache there cc @ellemouton

we currently only use graph cache for pathfinding calls. Have a look at the implementation of the graph cache - everything in there is indexed by node id & not channel id. we'd need to store a whole new index in there to make this call efficient

routing/pathfind.go

In this commit, the blindedPathRestrictions are expanded to include a list of incoming channels that must be included in any blinded path. The unit tests are expanded to test this new logic.

Including the blinded path incoming channel list argument to the addinvoice command, parsing and verifying that it has one or a comma separeted list of channels id.

Create the ChanIDString function to return a string representation of the route's channel IDs, formatting them in the order they appear in the route (e.g., "chanID1 -> chanID2"). Discarded routes with a success probability lower than the minimum threshold are now logged accordingly when finding a blinded path.

Assert that the BlindedPaths.MaxNumPaths value is non-zero.

Assert that the blindCfg.MaxNumPaths value is non-zero.

lightninglabs-deploy · 2025-05-09T05:07:11Z

@ellemouton: review reminder
@MPins, remember to re-request review from reviewers when ready

ellemouton

so close! Thanks for the updates :) looking much better now 🎉

(just btw - we'll hold off on merging this until 19 has been released. But we can defs get this in merge ready state before then 👍 )

ellemouton · 2025-05-12T08:02:07Z

routing/pathfind_test.go

+		maxNumHops:              3,
+		incomingChainedChannels: []uint64{2, 7, 6, 3},
+	})
+	require.Error(t, err)


please assert the contents of the error as is done in the demo PR 🙏

require.ErrorContains(t, err, "circular route")

ellemouton · 2025-05-12T08:02:58Z

routing/pathfind_test.go

+		nodeOmissionSet:         fn.NewSet(ctx.keyFromAlias("bob")),
+		incomingChainedChannels: []uint64{2},
+	})
+	require.Error(t, err)


i think we should never use require.Error just as is. We should always assert that we get an error we expect to get

ellemouton · 2025-05-12T08:03:09Z

routing/router.go

+			maxNumHops:              restrictions.NumHops,
+			nodeOmissionSet:         restrictions.NodeOmissionSet,
+			incomingChainedChannels: incomingChainedChannels,
+		})


ellemouton · 2025-05-12T08:03:35Z

routing/router.go

+		routeWithProbability := &routeWithProbability{
+			route: &route.Route{
+				SourcePubKey: introNode,
+				Hops:         hops,
+			},
+			probability: totalRouteProbability,
+		}
+
 		// Don't bother adding a route if its success probability less


i dont follow why this move was needed here

ellemouton · 2025-05-12T08:04:48Z

routing/pathfind.go

@@ -1310,7 +1342,7 @@ func processNodeForBlindedPath(g Graph, node route.Vertex,
 	}

 	// At this point, copy the alreadyVisited map.
-	visited := make(map[route.Vertex]bool, len(alreadyVisited))
+	visited := make(map[route.Vertex]bool, len(alreadyVisited)+1)


but then we should add a comment to explain this.

iirc, the demo PR doesnt add this +1. Can we not do what was done there instead @MPins ?

ellemouton · 2025-05-12T08:05:33Z

lnrpc/invoicesrpc/invoices.swagger.json

@@ -533,6 +533,14 @@
            "format": "byte"
          },
          "description": "A list of node IDs of nodes that should not be used in any of our generated\nblinded paths."
+        },


@MPins - you resolved this comment but it hasnt been addressed?

ellemouton · 2025-05-12T08:07:06Z

routing/router.go

+				"probability path(%.3f)",
+				route.ChanIDString(routeWithProbability.route),
+				routeWithProbability.probability,
+			)


re my previous comment: ok cool - but then that code refactor should be done here. In the commit where it is currently done, it is a no-op

ellemouton · 2025-05-12T08:07:51Z

rpcserver.go

@@ -6212,6 +6212,41 @@ func (r *rpcServer) AddInvoice(ctx context.Context,

 			blindingRestrictions.NodeOmissionSet.Add(vertex)
 		}


could use a commit message description

MPins changed the title ~~Add the option on path creator to specify the incoming node~~ Add the option on path creator to specify the incoming node on blinded path Sep 21, 2024

saubyk assigned MPins Oct 3, 2024

ziggie1984 requested changes Oct 3, 2024

View reviewed changes

lnrpc/lightning.proto Show resolved Hide resolved

routing/pathfind.go Outdated Show resolved Hide resolved

routing/pathfind.go Outdated Show resolved Hide resolved

cmd/commands/cmd_invoice.go Outdated Show resolved Hide resolved

cmd/commands/cmd_invoice.go Outdated Show resolved Hide resolved

MPins force-pushed the issue-8993 branch from 65a9c7f to adc5c80 Compare October 12, 2024 00:49

MPins commented Oct 12, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

MPins changed the title ~~Add the option on path creator to specify the incoming node on blinded path~~ Add the option on path creator to specify the incoming node or channel on blinded path Oct 13, 2024

ziggie1984 approved these changes Apr 24, 2025

View reviewed changes

ellemouton mentioned this pull request Apr 25, 2025

[demo] blinded paths: set incoming channel list ellemouton/lnd#198

Open

ellemouton requested changes Apr 25, 2025

View reviewed changes

ziggie1984 reviewed Apr 26, 2025

View reviewed changes

routing/pathfind.go Show resolved Hide resolved

MPins force-pushed the issue-8993 branch 8 times, most recently from 8dcf2cf to 434313d Compare April 28, 2025 03:10

MPins requested a review from ellemouton April 28, 2025 03:42

MPins force-pushed the issue-8993 branch from 434313d to b6ad5f2 Compare April 28, 2025 04:15

ziggie1984 reviewed Apr 28, 2025

View reviewed changes

MPins added 10 commits April 28, 2025 14:45

routing: add incoming channel chain to blinded paths

5b4aa88

In this commit, the blindedPathRestrictions are expanded to include a list of incoming channels that must be included in any blinded path. The unit tests are expanded to test this new logic.

lnrpc: add incoming chan list to BlindedPathConfig

b2c9d50

rpcserver: start reading incoming blinded chan chain

201052d

commands: add option to set the incoming channel on blinded path

53d6797

Including the blinded path incoming channel list argument to the addinvoice command, parsing and verifying that it has one or a comma separeted list of channels id.

lntest: add the function RPC call to asserts errors on AddInvoice

c3c8366

itest: add the test for incoming channel list on blinded invoice

5168ee2

lncfg: validate BlindedPaths.MaxNumPaths

a303387

Assert that the BlindedPaths.MaxNumPaths value is non-zero.

rpcserver: validate blindCfg.MaxNumPaths

6e776e1

Assert that the blindCfg.MaxNumPaths value is non-zero.

doc: release-notes-0.20.0

5517627

MPins force-pushed the issue-8993 branch from b6ad5f2 to 5517627 Compare April 28, 2025 17:46

ellemouton reviewed May 12, 2025

View reviewed changes

	func (m *MissionControl) GetProbability(fromNode, toNode route.Vertex,
	amt lnwire.MilliSatoshi, capacity btcutil.Amount) float64 {

	m.mu.Lock()
	defer m.mu.Unlock()

	now := m.cfg.clock.Now()
	results, _ := m.state.getLastPairResult(fromNode)

	// Use a distinct probability estimation function for local channels.
	if fromNode == m.cfg.selfNode {
	return m.estimator.LocalPairProbability(now, results, toNode)
	}

	return m.estimator.PairProbability(
	now, results, toNode, amt, capacity,
	)
	}

		@@ -6212,6 +6212,41 @@ func (r *rpcServer) AddInvoice(ctx context.Context,

		blindingRestrictions.NodeOmissionSet.Add(vertex)
		}

Add the option on path creator to specify the incoming channel on blinded path #9127

Are you sure you want to change the base?

Add the option on path creator to specify the incoming channel on blinded path #9127

Conversation

MPins commented Sep 21, 2024 • edited Loading

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

MPins commented Sep 21, 2024

MPins commented Sep 22, 2024

ellemouton commented Sep 23, 2024

ziggie1984 commented Oct 3, 2024

ziggie1984 left a comment • edited Loading

Choose a reason for hiding this comment

MPins commented Oct 4, 2024

ziggie1984 commented Oct 4, 2024

MPins commented Oct 7, 2024

MPins commented Oct 7, 2024

ziggie1984 commented Oct 8, 2024

MPins commented Oct 8, 2024

ziggie1984 commented Oct 8, 2024

MPins commented Oct 8, 2024

MPins commented Oct 8, 2024 • edited Loading

saubyk commented Oct 13, 2024

MPins commented Oct 13, 2024

ziggie1984 commented Oct 13, 2024

ziggie1984 commented Oct 13, 2024

MPins commented Oct 13, 2024

ziggie1984 commented Oct 13, 2024

MPins commented Oct 13, 2024

saubyk commented Oct 13, 2024 • edited Loading

ellemouton commented Oct 16, 2024

MPins commented Oct 16, 2024

ziggie1984 commented Oct 16, 2024 • edited Loading

MPins commented Apr 24, 2025

ziggie1984 left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

MPins commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented May 9, 2025

ellemouton 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

MPins commented Sep 21, 2024 •

edited

Loading

ziggie1984 left a comment •

edited

Loading

MPins commented Oct 8, 2024 •

edited

Loading

saubyk commented Oct 13, 2024 •

edited

Loading

ziggie1984 commented Oct 16, 2024 •

edited

Loading