Light-152 multihop routing#37
Conversation
Add multihop/payment initialisation and integration with routing.
Path are validated by sending each node message requesting if it is able to make HTLC payment. It is implemented in parallel style, not onion like. Add comment to make multihop payment to a given address which integrates routing, path validation and multihop payment.
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Thanks for another PR!
As details within the review, I think many parts of this PR can be omitted due to redundancy compared to the latest version of master. Let's discuss the other components such as the pre-send HTLC lock-up and the placement of the sender/receiver protocol within the LN stack.
| printRespJson(channels) | ||
| } | ||
|
|
||
| var SendMultihopPayment = cli.Command{ |
There was a problem hiding this comment.
I don't think there should be distinct cli nor RPC commands for sending multi-hop payments vs single hop payments. At the time this PR was being put together the onion routing stuff wasn't in place, but it is now so the caller doesn't require any knowledge of the full route, only the final destination.
However, I can see how it might be useful to give the caller full control over the final path which the payment is execute over. So how about we merge this behavior into the existing SendPayment command with an additional argument which provides full "path selection".
| if err != nil { | ||
| return err | ||
| } | ||
| if len(p) != 32 { |
There was a problem hiding this comment.
With the latest version of master, when sending payments compressed public keys are used, so this should check for a length of 33 instead.
|
|
||
| var FindPathCommand = cli.Command{ | ||
| Name: "findpath", | ||
| Description: "finds path from node to some other node", |
There was a problem hiding this comment.
I think the description should be updated to be a bit more explicit. How about something along the lines of:
"attempts to find a path to the target destination node"
| Flags: []cli.Flag{ | ||
| cli.StringFlag{ | ||
| Name: "destination", | ||
| Usage: "lightningID of destination", |
There was a problem hiding this comment.
In order to be in line with the latest version of master, a compressed public key should be expected instead.
| Usage: "lightningID of destination", | ||
| }, | ||
| cli.IntFlag{ | ||
| Name: "maxpath", |
There was a problem hiding this comment.
To make the arg a bit more explicit, it should be extended a bit to be named maxpathlength.
| // proper wire message. This helepr method is provided in order to aide an | ||
| // htlcManager in forwarding packets to the htlcSwitch. | ||
| func (p *peer) logEntryToHtlcPkt(pd *lnwallet.PaymentDescriptor) *htlcPacket { | ||
| func (p *peer) logEntryToHtlcPkt(pd *lnwallet.PaymentDescriptor, chanPoint *wire.OutPoint) *htlcPacket { |
There was a problem hiding this comment.
These changes are unnecessary from the PoV of the latest version of master.
| }, nil | ||
| } | ||
|
|
||
| func (r *rpcServer) SendMultihopPayment(ctx context.Context, in *lnrpc.MultihopPaymentRequest) (* lnrpc.MultihopPaymentResponse, error){ |
There was a problem hiding this comment.
As detailed above, I don't think the addition of the RPC is necessary.
| int(in.NumberOfPaths), | ||
| 1*time.Second, | ||
| ) | ||
| rpcsLog.Info("[FindPath] after FindKSP dest=", hex.EncodeToString([]byte(in.TargetID))) |
There was a problem hiding this comment.
Converting to a byte slice, then using EncodeToString seems like it'll lead to an erroneous string being produced. Why not just encode as hex directly? Or if it's already a hex string, then that can be printed as is.
| } | ||
| validationStatuses := make([]lnwire.AllowHTLCStatus, len(paths)) | ||
| if in.Validate { | ||
| validationStatuses = r.server.routingMgr.ValidatePathMult( |
There was a problem hiding this comment.
What does ValidatePathMult do?
| } else { | ||
| srvrLog.Errorf("Can't find peer to send message %v", receiverID) | ||
| } | ||
| case msg := <- s.paymentManager.chPaymentOut: |
There was a problem hiding this comment.
Similarly, I think this fragment can be removed all together from the PoV of the latest state in master.
|
Also this needs to be rebased on-top of the latest master branch as it diverges significantly. |
|
Closing this PR now as #70 and the state of the current codebase supersedes it. I'll make some issues to salvage certain parts of this PR such as the path querying, since the code has already been written. |
sphinx: return error source as integer
2 participants
A simple implementation of multihop payments.
Integration with routing and path validation.