payalgo: Support bolt11 'r' fields.

[ZmnSCPxj]

Part of spec.

Not sure how to test, though.We need some way to generate r fields in bolt11 invoices.

[rustyrussell]

OK, first this is screaming for a new source file, as your comment points out.

Secondly, you really need to implement 'r' for unannounced routes, at least for ones less than 6 confirms. That's completely independent of this, but does allow you to test this code nicely, too.

[ZmnSCPxj]

This line is wrong. It assumes the previous delay is loaded. But it is not loaded because we are loading route hops backward. Instead, we should load the delay as + from next delay (or final target delay if this is the last hop).

[ZmnSCPxj]

Unfortunately it seems unannounced channels cannot be used for r bolt11 fields. r fields include the fee_base_msat and fee_proportional_millionths. There is no way to know how much the peer will want to charge for the route from that peer to us.

The BOLT spec mentions fee_base_msat and fee_proportional_millionths in only BOLT 7, which is about gossip (and hence means the channel is announced), and BOLT 11, the invoice spec.

My understanding of the invoice route is that for example if it indicates a route peer A going through channel C to us, then the fees should be the fees that A will charge for use of channel C. How do we learn that? Nothing in the spec suggests that we can learn that from a peer except for channel_update which I think is not created unless the channel is announced.

We could try to use our own fee settings, in the hope that nobody will change the defaults and everyone uses the same fee rate, but what if the peer is an lnd node?

[rustyrussell]

You're right. @cdecker may have thoughts on this...

The alternative then is to implement non-public channels, which is what this was designed for.

[ZmnSCPxj]

Even with non-public channels, how does the other side know how much we want to charge for use of our channel? Seems, limitation of BOLT spec?

[cdecker]

We had an alternative code path that'd allow channel_updates for unannounced channels, which was later replaced with one that would send unsigned channel_announcements followed by signed channel_updates, but now we're left without any of these. We probably need to bring this up at the next spec meeting, and fix a canonical way of handling these. I think channel_updates should be exchangeable between endpoints, and the lack of channel_announcement ensures they are local only.

[cdecker]

Just to add some more color to this: we might eventually want to introduce a routing_state_view to be able to temporarily add channels to our view, especially for the non-public channels.

[ZmnSCPxj]

I would argue the opposite; we should retain even non-published channels in our routing map permanently once we learn of them via invoice, and remove it only on permanent channel failure. This makes it harder for route analyzers if we sometimes use non-published channels to pay unrelated nodes; the route analyzer might assume we terminate to the node after the non-published channel, when in fact we are using that node as a hop node, going into one non-published channel and out another non-published channel, and probably helps the privacy of that node too, since this is activity that naive route analysis might assume terminates at them but is actually hopped.

[ZmnSCPxj]

Yet another wrinkle: it seems c-lightning will return WIRE_UNKNOWN_NEXT_PEER for channels in CHANNELD_AWAITING_LOCKIN, meaning they cannot be used for private routes, ha.

It seems to test this at integration test, we need to actually support non-published channels too, sigh.

In any case my attempts to make this work also revealed a new bug in my bolt11-private-route handling:

• If a channel or node failure occurs on the private part of the route, we inform gossipd, but for private routes, then gossipd does not know it, so it does nothing. We then retry with the same "contactpoint" i.e. the private route start point, and gossipd returns the same public route. So probably I need to keep track of the public route length, and if the erring node index exceeds the public part, go to the next contactpoint.

[ZmnSCPxj]

@cdecker has there been some resolution regarding channel_update to get fees for unpublished channels?

[cdecker]

@ZmnSCPxj not really, they only make sense for published nodes since otherwise we are one of the peers (and know what we'd charge) or the other end, in which case we most likely don't want to route in that direction (because we're the receiving endpoint).

We should probably just send a channel_update to our peer upon lockin. That way we could have invoice look up incoming not yet public channels and pass that in as r. Then we could do a 2 hop route through not yet public channels. Would that help?

[ZmnSCPxj]

Yes, that is indeed what I was referring to when I asked about "channel_update for unpublished channels": if the channel is unpublished, we still need to get channel_update (or something equivalent anyway) from the other side, else we cannot really use the channel for receiving. If we are receiving, it is the other side which determines what fees are for forwarding to us, so we need to get fees from them, meaning even for unpublished channels we should give fees to the other side too.

This is approximately OK for interoperability, I think: strict followers of the spec will ignore incoming channel_update for a channel that is not channel_announcement before. but if we have an unpublished channel made to us, we want to be able to tell payers the fees on that hop, so if the other side happens to be a c-lightning also, we should be able to get that information.

But if it is useful for us, we should probably think that other implementations may also find it useful, so we should consider to propose it for standardization.

[ZmnSCPxj]

The contactpoint system does what you describe already. We generate a contactpoint for each r, then prepend the destination as another contactpoint.

The contactpoint list is then iterated over. We compute the target payment and compute how much has to reach the contact point node. If we fail to find a route to the contactpoint, or we get a failure onion past the contactpoint node, we drop the current contactpoint and move on to the next.

I think the current PR is minimal enough as is.

[cdecker]

Just want to make sure, the first contact_point in the list is the destination itself, right? This way we generate an optimal route first, falling back to indirect contact_points if that fails.

[ZmnSCPxj]

Yes. The contactpoint created at the destination is added using list_add, which adds to the front of the list:

https://github.com/ZmnSCPxj/lightning/blob/31f4b259d89098ea5867374da154ebd6b7b36359/lightningd/payalgo.c#L267-L278

Contactpoints created for each r route are added using list_add_tail, which adds to the end of the list:

https://github.com/ZmnSCPxj/lightning/blob/31f4b259d89098ea5867374da154ebd6b7b36359/lightningd/payalgo.c#L279-L298

[cdecker]

Great, I'd say this is good to go, and let's revisit the contact_points layer later on, maybe.

ACK 34f73c2

[cdecker]

Currently needs a rebase, and @rustyrussell's approval :-)

[ZmnSCPxj]

Rebased, s/maxroutes/maxprivchannels/

[cdecker]

Seems we now have a case clash:

lightningd/gossip_control.c: In function ‘gossip_msg’:
lightningd/gossip_control.c:160:2: error: duplicate case value
  case WIRE_GOSSIP_LOCAL_CHANNEL_UPDATE:
  ^
lightningd/gossip_control.c:143:2: error: previously used here
  case WIRE_GOSSIP_GET_PRIVATE_ROUTES:
  ^
<builtin>: recipe for target 'lightningd/gossip_control.o' failed

[ZmnSCPxj]

Ready for review.

Sadly my hacking node will be offline for some time, so I will have some difficulty updating this pull request in the close future, but I will try to respond to reviews anyway.

[cdecker]

ACK 2ad7867

Code is ok, but I'll let @rustyrussell get the final say.

[ghost]

^^

[cdecker]

@ZmnSCPxj I'm sorry I botched the merge edit, and seem to be unable to push to your branch to to a real rebase. Could you rebase and I'll merge it :-)

[ZmnSCPxj]

Sorry for the ridiculously long hiatus, I shall see what I can do (hacking computer is currently unstable due to lower-level issues, am currently stabilizing it).

[cdecker]

This PR has been inactive for a long time (partly my fault), and is probably more work to rebase than it is to rewrite from scratch. Would anybody object if I close this? @ZmnSCPxj @rustyrussell