-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the option on path creator to specify the incoming node or channel on blinded path #9127
base: master
Are you sure you want to change the base?
Conversation
Hello @ellemouton if you think I'm on the right way I can add the option to choose a channel also. |
Maybe change the name to "incoming_node" is a better idea! But I'll wait for others opinion also. |
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
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, | |
) | |
} |
@@ -3894,6 +3894,12 @@ message BlindedPathConfig { | |||
blinded paths. | |||
*/ | |||
repeated bytes node_omission_list = 4; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
squash into first commit because it belongs to the lnrpc change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did just one commit to all lnrpc changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And another one with lnrpc/lightning.pb.go . As it is a file created with make rpc
I'm not sure if I should commit this change.
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. |
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 ... |
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. |
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) |
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. |
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. |
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. |
@@ -35,7 +35,7 @@ require ( | |||
github.com/lightningnetwork/lightning-onion v1.2.1-0.20240712235311-98bd56499dfb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those files were changed when I did go get -u github.com/lightningnetwork/lnd/fn
to update the Fn to use isEmpty function once #9159 is merged
Have a basic question here.
So why are we then giving the user the control of selecting a node in the cli, instead of directly specifying a channel? |
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. |
Good question, I also tend to only support the incoming channel id why:
|
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. |
I think that
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. |
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. |
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:
|
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? |
@ellemouton what's your take on this, we cannot really know whether peers have a universal policy to us?
|
@ziggie1984 - yes that makes sense but my point is that nothing stops the peer from in any case just choosing the channel they want to choose. If you peer has channel X and Y with you, and you tell them to use channel X, nothing stops them from using channel Y |
Correct that's for the perspective of the forwarding node, but let's look at it from the receiving node: You have channel X and Y, both have different Policies, now you create your invoice specifying the last hop via the nodeid, you get 2 blinded paths most likely (or more). The sender will probably select the path with the best constraints, hence the receiver lost a bit of control while specifying the invoice. If he could have specified the channelid he would not put both paths into the invoice. I agree with you statement when policies are equal the sender can choose both channel X or channel Y and specifying the nodeid here makes sense. However when the policies are different it makes sense for the receiver to have more control specifying the channelid not the nodeid. Thats why I proposed only going with the chanid, which is basically valid for both options, rather than going with only nodeid, or both (which increases complexity). |
Do you favour the approach of providing a whole path not just the last channel ? Could not think of a use case where we would like to specify the whole path ? |
Do you mean a list of final hops or all the blinded path? As @ziggie1984 said I can't imagine also a use case for that. 🤔 |
Fixes #8993
Change Description
Add the option on path creator to specify the incoming node setting a list of penultimates nodes
Steps to Test
lncli addinvoice --blind --blinded_path_penultimate_node "node_id" amount
lncli addinvoice --blind --blinded_path_penultimate_node "node_id1" --blinded_path_penultimate_node "node_id2" amount
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.