Skip to content
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

Flat features: far simpler solution for feature bits. #666

Open
wants to merge 4 commits into
base: master
from

Conversation

@rustyrussell
Copy link
Collaborator

commented Sep 4, 2019

We simply specify how they're presented in the three different contexts.

Consider these theoretical future features:

opt_dlog_chan: a new channel type which uses a new discrete log HTLC
type, but can't support traditional HTLC:

  • init: presents as odd (optional) or even (if traditional channels
    not supported)
  • node_announcement: the same as above, so you can seek suitable peers.
  • channel_announcement: presents as even (compulsory), since users need
    to use the new HTLCs.

opt_wumbochan: a node which allows channels > 2^24 satoshis:

  • init: presents as odd (optional), or maybe even (if you only want
    giant channels)
  • node_announcement: the same as above, so you can seek suitable peers.
  • channel_announcement: not present, since size of channel indicates
    capacity.

opt_wumbohtlc: a channel which allows HTLCs > 2^32 millisatoshis:

  • init: presents as odd (optional), or even (compulsory)
  • node_announcement: the same as above, so you can seek suitable peers.
  • channel_announcement: odd (optional) since you can use the channel
    without understanding what this option means.

Closes: #571


#### Requirements

The sending node:
- MUST send `init` as the first Lightning message for any connection.
- MUST set `mbz` to zero.

This comment has been minimized.

Copy link
@t-bast

t-bast Sep 4, 2019

Collaborator

nit: should we mention that it's for backwards-compatibility?

Crazy idea (but could be done later): why not:

  • [u16:mbz]
  • [u16:mbz]
  • [tlvs]

And define a first tlv type (2?) to hold feature bits?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 5, 2019

Author Collaborator

Not backward compatible. unfortunately. Currently we have no deployed global bits, so we can do the mbz trick.

This comment has been minimized.

Copy link
@t-bast

t-bast Sep 5, 2019

Collaborator

We do have a global feature bit spec-ed (variable-length onion).
For eclair we haven't made a release that advertises it yet, but if people deployed nodes from master then they will be sending something in the global features field.
So our code would need to handle both the previous format and the new with mbz (at least for a while)...this isn't too painful to implement though (just branch on mbz being 0 or not).

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 16, 2019

Author Collaborator

Yes, since it's not deployed I was hoping we could sidestep this... but perhaps not.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 17, 2019

Member

What does mbz mean here? "Must be zero"?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 17, 2019

Member

We haven't cut a release either using the new global feature bits, but are very close to doing so....

As a result, would be nice to have all this clarified as otherwise with our target release schedule, we may cut a version that still uses the existing global feature bits.

Stepping back for a second, this mbz change doesn't really strike me as being backwards compatible. We're potentially fracturing the network here (let's say people are using a fork of some implementation in the wild and are using global feature bits to gate their protocol). Instead we can simply rename the field in the spec: global isn't bound to anything (don't set a name), and local becomes the features as is here. For some period of time, we should still also merge the two fields together.

01-messaging.md Outdated Show resolved Hide resolved
| 3 | `initial_routing_sync` | Sending node needs a complete routing information dump | I | [BOLT #7][bolt07-sync] |
| 4/5 | `option_upfront_shutdown_script` | Commits to a shutdown scriptpubkey when opening channel | IN | [BOLT #2][bolt02-open] |
| 6/7 | `gossip_queries` | More sophisticated gossip control | IN | [BOLT #7][bolt07-query] |
| 8/9 | `var_onion_optin` | Requires/supports variable-length routing onion payloads | IN | [Routing Onion Specification][bolt04] |

This comment has been minimized.

Copy link
@t-bast

t-bast Sep 4, 2019

Collaborator

Shouldn't this be INC?
This could allow nodes to force some of their channels to use variable-length onions (and explicitly disallow the legacy fixed-frames) by setting bit 8 in the channel_update of such channels.
Not sure whether it's really useful or not, but maybe worth discussing?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 5, 2019

Author Collaborator

It's tempting, since node_announcement propagation is currently less reliable than channel_announcements. But channel_announcements can't be updated, so current channels would never set this bit, even though they'd be perfectly fine.

So I think it needs to be a Node bit.

07-routing-gossip.md Show resolved Hide resolved
@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Why aren't the three scenarios in this PR body expressible using the current set of feature bit fields and names? We can just leave the existing local feature bits as is, but then start to define all new bits as a global feature bit. The node and channel announcement fields would then only inherit the name space from the global feature bits. Even in this latest iteration, I still have a nagging feeling that this isn't really fixing anything.

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

(also this PR is evil, just saying)

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 17, 2019

Why aren't the three scenarios in this PR body expressible using the current set of feature bit fields and names? We can just leave the existing local feature bits as is, but then start to define all new bits as a global feature bit. The node and channel announcement fields would then only inherit the name space from the global feature bits. Even in this latest iteration, I still have a nagging feeling that this isn't really fixing anything.

Because everyone got confused about the two schemes, yet a single scheme also sucks, because optional and compulsory are different in different contexts:

  1. My new node insists a new crypto peer comms (EVEN bit in init and node_announcement). But it doesn't matter for channels, so why would I set this to channel_announce? That would stop old peers from routing through them.

  2. My new node allows funky log-based channels (ODD bit in init and node_announcement). You do too, and we negotiate it. For that channel, it has to be EVEN in channel_announce, since you can't use the channel unless you understand the log-based scheme.

  3. My node only allows log-based channels except for existing channels (EVEN bit in node_announcment, odd bit in init messages to legacy peers).

TLDR: much easier to think about what each feature needs, and specify it at that level.

BOLT 9: flatten feature fields.
We simply specify, in each case, where they will appear ("Context").
We replace the `globalfeatures` field in init with a zero.

Note also: we REQUIRE minimal `features` field in
channel_announcement, since otherwise both sides of channel will not
agree and not be able to create their signatures!

Consider these theoretical future features:

`opt_dlog_chan`: a new channel type which uses a new discrete log HTLC
type, but can't support traditional HTLC:

* `init`: presents as odd (optional) or even (if traditional channels
  not supported)
* `node_announcement`: the same as above, so you can seek suitable peers.
* `channel_announcement`: presents as even (compulsory), since users need
  to use the new HTLCs.

`opt_wumbochan`: a node which allows channels > 2^24 satoshis:

* `init`: presents as odd (optional), or maybe even (if you only want
  giant channels)
* `node_announcement`: the same as above, so you can seek suitable peers.
* `channel_announcement`: not present, since size of channel indicates
  capacity.

`opt_wumbohtlc`: a channel which allows HTLCs > 2^32 millisatoshis:

* `init`: presents as odd (optional), or even (compulsory)
* `node_announcement`: the same as above, so you can seek suitable peers.
* `channel_announcement`: odd (optional) since you can use the channel
  without understanding what this option means.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
fixup! BOLT 9: flatten feature fields.
Restore gflen field, as (unreleased) implementations are using it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 7: always propagate announcements with unknown features.
The feature fields refer to the properties of the channel/node, not the
message itself, so we can still propagate them (and should, to avoid
splitting the network).

If we want to make an incompatible announcement message, we'll use a
different type, or insert an even TLV type.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/flat-features branch from 926e960 to 2f5176e Sep 17, 2019

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 17, 2019

Rebased, restored gflen field.

01-messaging.md Outdated Show resolved Hide resolved
@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Sep 17, 2019

It would be nice to make quick progress there; this way we're not slowing down releases.
ACK 2f5176e, as long as no-one made an explicit release advertizing var_onion_optin in the global flags we can safely start ignoring those (otherwise if lnd does release before we merge this PR, we'll need our code to temporarily support both schemes which is a bit awkward).

@t-bast
t-bast approved these changes Sep 17, 2019
The Context column decodes as follows:
* `I`: presented in the `init` message.
* `N`: presented in the `node_announcement` messages
* `C`: presented in the `channel_announcement` message.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 17, 2019

Member

What about the channel update message? We also need possibly extend the existing hop hints to also encompass this information. One could say the current set of feature bits handles that, however it's possible to provide multiple hop hints, and each of those channels could themselves advertise/require distinct sets of hop hints.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 18, 2019

Author Collaborator

We don't have a general feature field in channel_update (yet?). But we can add a field if we need to.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 18, 2019

Member

I think the main thing here is the inability to express distinct feature sets for each channel with the current BOLT 11 feature bit vector extension.

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

It would be nice to make quick progress there; this way we're not slowing down releases.

Our release is late as is, and it's now being held up by this PR as it creates a dependency with the static remote key PR. We're now schedule to tag next week. As for the current state of the global feature bits: the static key PR is more or less finalized/ready but is now blocked on the inclusion of this PR, the onion tlv feature has itself long been included.

| 3 | `initial_routing_sync` | Sending node needs a complete routing information dump | I | [BOLT #7][bolt07-sync] |
| 4/5 | `option_upfront_shutdown_script` | Commits to a shutdown scriptpubkey when opening channel | IN | [BOLT #2][bolt02-open] |
| 6/7 | `gossip_queries` | More sophisticated gossip control | IN | [BOLT #7][bolt07-query] |
| 8/9 | `var_onion_optin` | Requires/supports variable-length routing onion payloads | IN | [Routing Onion Specification][bolt04] |

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 17, 2019

Member

Why should a node level feature gate if two peers can/will-continue-to communicate on the connection level? As is, if I went to open a channel with a peer that doesn't support the new onion payload, but they required this bit, then the connection would fail upon feature bit comparison.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 18, 2019

Author Collaborator

Yes, it's overbroad: this is the tradeoff. Previously you could in theory gossip with a node which had an even global feature you didn't like, too.

Putting it in the channel_announce would also be overbroad (since both nodes might not need it). We could add a feature bitfield (TLV-style) to channel_update, but it's marginally more efficient to use node_announce, since it's really per-node not per channel in any sane implementation.

Did we want to go there?

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

At this point, we may need to just accept the existing global feature bit namespace, then once this is ready and tested for interop all around, copy over the existing global feature bit namespace to what is currently known as the local feature bit namespace. However, re the combination of the namespaces, see my latest comment re cross-layer feature bit awareness comparisons.

Fix-up!
Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

To remedy that comparison issue, we may need to add additional feature bit vectors maybe? Not sure this is really doable though, since the innit message can be padded like the rest of the messages, so it's possible that a larger preceding feature vectors renders another unable to be coded in the message size limit.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Sep 18, 2019

Our release is late as is, and it's now being held up by this PR as it creates a dependency with the static remote key PR.

Yeah, I might have been too optimistic by hoping we could merge this soon to unblock your release from further delay :).

I quite like the current proposal. Updated writers will only write to the currently-named localfeatures field all of their feature bits.

Legacy readers will not be impacted because right now the only global feature bit (variable length onion) will only be set as optional, not mandatory (they aren't missing out on anything since we don't have real features like AMP that actually leverage the variable length onion).

We can have our implementations handle both the legacy case (if we see that glen isn't 0) and the updated case (when glen is 0) when reading the other peer's features so that we play nicely with legacy nodes. Does that sound like a reasonable compromise?

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2019

There's definitely no harm in advertizing var_onion_optin in local features, and I'd definitely recommend it.

The other questions are:

  1. Adding features to channel_update.
  2. Adding features to bolt11 routing hints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.