Restrict data_loss_protect to init context

[ariard]

A sender should be able to route through a node even if it doesn't understand init features flag advertised by latter one. For e.g, for a Alice -> Bob -> Dave payment path, Alice should be able to route through Dave even if she doesn't support data_loss_protect as this a connection-only feature.

Right now it's not an issue because everyone implements data_loss_protect (and we may let the feature flag as it is given that's a legacy, pre-flat feature one) but should be aware about this issue if we devise further gentleman policy features a la data_loss_protect, than current contexts don't catch faithfully this class of features. Currently, a node new feature will be seen as unknown by non-upgraded nodes without being able to dissociate between connection-only and packet-processing. Though it lets unsolved how you'll learn about peer supporting without trying to connect first..

[t-bast]

It feels useful to me to have data_loss_protect in the node announcement...

Imagine you want to only connect to nodes that offer data_loss_protect because you're afraid you'll lose some state at some point. If that's only offered in the init you need to probe connections to discover which nodes offer the feature you want which is quite impractical, right?

Honestly I'd put everything in node announcement, that ensures peers can find you easily; the only exception of initial_routing_sync is because we really want to deprecate it sooner rather than later.

[TheBlueMatt]

Right, but the point is that we're mixing uses here - node_announce includes flags which restrict when you can route through a node as well as when you can connect. In data_loss_protect we already have an example of something which clearly requires one and not the other. We could redefine node_context, of course, but I don't see very much wrong with probing. Its not like you should be opening channels to random nodes you find in the route graph (without additional restrictions) anyway.

[ariard]

Let's say we introduce some optional_data_loss_protect_enhanced, non-upgraded nodes will see this as an unknown node feature flag and won't be able to route through upgraded nodes though it's a connection-only feature. And if you assume routing node will upgrade faster than edges ones, it just means you may loose suddenly your best payment paths...

Probing isn't great but would work, I'm fine to let dlp as it is, I just want to document this issue to avoid missing in it in the future when we'll need to fix it. At that time we can extend DNS records, split node context into sub-domains, extend node_announcement, ...

[Roasbeef]

Honestly I'd put everything in node announcement, that ensures peers can find you easily; the only exception of initial_routing_sync is because we really want to deprecate it sooner rather than later.

Same, if it can be used for preferential peer selection of channel creation, then it should be in the node announcement.

Let's say we introduce some optional_data_loss_protect_enhanced, non-upgraded nodes will see this as an unknown node feature flag and won't be able to route through upgraded nodes though it's a connection-only feature

Only if the bit is set to required. On the routing level, nodes should be looking at individual channels rather than solely the node announcement. It's also the case that you don't actually need a node's ann in order to route through it, and you may have all the channel details but not the node announcement.

[TheBlueMatt]

nodes should be looking at individual channels rather than solely the node announcement

This highlights an alternative way to solve this: make node_announcement features mirror init features in that they are the things that a node wants their peers to have, but dont set routing-related things in it. This would imply removing eg var_onion_optin from node_announcement context features.

[Roasbeef]

This would imply removing eg var_onion_optin from node_announcement context features.

Onion decoding as is, is a node-level task independent of channels. Therefore it should continue to reside in node anns. In the end, the feature bit unification helped to create a series of more explicit name spaces for features, but certain special cases like TLV in the onion payload still apply.

[ariard]

but certain special cases like TLV in the onion payload still apply.

What do you mean by this ? We have to decide ether if node semantic is connect-to or route-through. Right now it's both.

[TheBlueMatt]

A further question came up for us - if a node announces a channel_announcement with unknown required feature bits in it, but we're directly connected to them, what the hell should we do with that? Clearly if they're requiring something to route through them that we dont understand, we shouldn't keep our channel with them, but we're not guaranteed to even get such a notice if the channel isn't public. It seems init and channel and node context kinda all need to be merged to resolve this :(.

[t-bast]

The discussion helped me understand better the underlying concern (which I had missed initially).

IIUC your point is that node_announcement is used to select the nodes you'll be using in a payment route. That is going to cause issues when nodes upgrade and turn on new features with the required bit; you can't include them in your payment route anymore. This is a valid concern and means we should clarify what message's features we should look at in what context.

I agree with @Roasbeef here: I see node_announcement primarily as the message we look at when we're considering connecting to a node and/or opening channels to that node.
When we are building a payment route, we should not be looking at the nodes of the graph; we should be looking at the edges. IMHO that means we should look at the channel_announcement feature bits. This lets a node update a required feature bit in his node_announcement to tell the network that from now on, it will only accept channels that offer that feature, while still keeping "old" channels open without that feature.

It does make the feature bits analysis more complicated. I think a reasonable way of doing it would be to look at the node_announcement and then if it contains unknown required feature bits, look at this node's channels to see if at least one of them has feature bits we understand; if that's the case we can try routing through this node (and this node will know what channels it can really use when receiving the HTLC and do non-strict forwarding).

Do you think that makes sense? I think you're raising an interesting issue, and it's worth spending some time on clarifying that area of the spec.

[t-bast]

A further question came up for us - if a node announces a channel_announcement with unknown required feature bits in it, but we're directly connected to them, what the hell should we do with that?

That's tricky...I think that should be resolved by adding feature bits to channel_update.
Once the channel is created, I think you should not be allowed to update the channel_announcement's features. Then each side can independently update its channel_update's features - and in case a feature needs to be activated on both sides, that can't be done on an existing channel (or would need a negotiation mechanism).

I think it will be easier to answer that question when we study an actual feature example; a concrete use-case will likely teach us useful lessons.

[TheBlueMatt]

I think you should not be allowed to update the channel_announcement's features. Then each side can independently update its channel_update's features - and in case a feature needs to be activated on both sides, that can't be done on an existing channel (or would need a negotiation mechanism).

Hmm, I dont think we need to be that restrictive. As long as the peers have a good way of negotiating when they're both ready for a feature, they can update it, of course we'd need to build such a negotiation...

Still, I think we've detected enough issues with flat features at this point that we should discuss in the meeting. The "easy" answer is to leave it mostly as is, and just add guidance to the spec discussing these issues and note that there should only very rarely ever be a case where you want to add a feature bit which is not in every context.

[cdecker]

Still, I think we've detected enough issues with flat features at this point that we should discuss in the meeting. The "easy" answer is to leave it mostly as is, and just add guidance to the spec discussing these issues and note that there should only very rarely ever be a case where you want to add a feature bit which is not in every context.

I don't think the issues are about flat features (i.e., the fact that feature bits don't collide), we just found issues with the "just stuff the same bits in all places" part, which might cause some issues when interpreting them 😉

[TheBlueMatt]

To a large extent the issue is that we *don’t* stuff the same bits everywhere anymore. Merging everything into one giant features set would solve most of the issues. In any case I suppose we should discuss in two weeks some today is a ‘murican holiday.

…

On Jan 20, 2020, at 13:28, Christian Decker ***@***.***> wrote:  Still, I think we've detected enough issues with flat features at this point that we should discuss in the meeting. The "easy" answer is to leave it mostly as is, and just add guidance to the spec discussing these issues and note that there should only very rarely ever be a case where you want to add a feature bit which is not in every context. I don't think the issues are about flat features (i.e., the fact that feature bits don't collide), we just found issues with the "just stuff the same bits in all places" part, which might cause some issues when interpreting them 😉 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[rustyrussell]

OK, we should use the same (currently unused) notation as we do for channel announcements: in this case, "N-" meaning "always advertize it in node_announcement as odd, even if it's normally even". That means it doesn't prevent routing, but you still notify others you support it.

If they connect, they'll see it as even in the init msg, and if they don't understand it they'll fail at that stage as designed.

[cdecker]

This issue has been discussed during the 2020/01/20 spec meeting
and, while there is general agreement that we should not blanket-announce all
features in the node_announcement, it was pointed out that signaling this
particular feature data_loss_protect is indeed useful for preferential
peering, e.g., if lnd wants to use the static channel backup which heavily
relies on data_loss_protect being supported.

In particular it was mentioned that we should differentiate between signaling
optional and mandatory support in the node_announcement. If a node signals
mandatory support it might prevent other nodes from even routing through that
node, not just peering. Nevertheless, it's sometimes desirable to also signal
mandatory support:

19:18:37 t-bast: if for example I no longer process legacy onion payloads that'd be an even bit in the node_announcement since otherwise we can't talk
19:18:57 That'd be an example of "if you don't understand this, don't talk to me"

The distinction mentioned by @rustyrussell above allows us to specify whether
we should advertise mandatory flags or not, but in this case the vote was to
keep advertising data_loss_protect in the node_announcement.

Closing this as per spec meeting.

[TheBlueMatt]

Hmm, based on your summary it seems there is quite some misunderstanding at the heart of the issue here. This certainly wasn’t intended as a comment in data_loss_protect, it was only used as an example to demonstrate the lack of flexibility the current system allows, specifically as it relates to unknown features. Let me restate the issue and maybe we can all get on the same page before discussing it further:

As came in both in the discussion you described and in Laolu’s comments on #723, unknown features in node_announcements may have several different meanings: both “I cannot connect to this peer” and “I cannot route through/to this peer “. #723 clarified that they are currently both to keep it consistent with the TLV/MPP features appearing in both, but Laolu’s comment post-merge indicate he thought it was only “connect to” (and channel_announcement features were for routing). Indeed, it seems to me it’s somewhat critical that we have the ability to signal “no connection, but routing is fine” given the conversation.

In any case, the section on what to do with such bits very clearly needs a lot more depth (as well as reasoning so that future new bits Do The Right Thing). We could, for example, specify that node_announcements should contain a superset of what you’ll include in channel_announcements (and init would be a superset and the latest version of that) on the basis that if you can’t route through it, you shouldn’t connect to it (which confuses me why @rustyrussell commented above that we would ever announce something in the node_announcement as odd if its even in the channel announcements that node will want to generate).

This would not imply any restrictions on invoice features as receiving is different than routing, and you may want to connect/route through a node that you cannot send to. However, of course, this poses an issue for spontaneous payments (or any payment without an invoice) given that you’ll need a way to signal features that would appear in an invoice without polluting the namespace in channel/node/init announcements.

[t-bast]

Indeed, it seems to me it’s somewhat critical that we have the ability to signal “no connection, but routing is fine” given the conversation.

The way I understood the conclusion of the spec meeting is that node_announcement is used as some kind of "hey here is everything you need to know about me" so you include mostly every feature you support. However features unrelated to routing through you must be included as N-, ie odd. In init, you can decide to make these features even if they're really required for connecting.

Related to the current PR, we had a NACK for the specific case of removing data_loss_protect from node_announcement, so imho this PR can be closed.

However we agreed that some work was needed to clarify the overall issue of "feature bits to route through vs feature bits to connect", and that a new PR should be opened for that (introducing N- / N+ for example). That new PR would be a good place to discuss this further and figure out the right guidance for future feature bits.

[TheBlueMatt]

Related to the current PR, we had a NACK for the specific case of removing data_loss_protect from node_announcement, so imho this PR can be closed.

Somehow I totally missed that this was a PR not an issue. So my bad on that one...

In any case, the points I made are still valid, and I don't think N- solves even half of them, but I'll just copy that text into whatever PR/issue shows up as a followup :).