Typed features

[thomash-acinq]

Give different types to init features / node announcement features / invoice features.

This also fixes the lack of unknown features in the invoice (users can now add their custom unknown features) and we now accept more invoices (we would previously reject invoices if they required a non invoice feature that we don't support).

[codecov-commenter]

Codecov Report

Merging #2164 (96d99fa) into master (fa31d81) will decrease coverage by 0.06%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master    #2164      +/-   ##
==========================================
- Coverage   83.89%   83.83%   -0.07%     
==========================================
  Files         185      185              
  Lines       13898    13900       +2     
  Branches      562      542      -20     
==========================================
- Hits        11660    11653       -7     
- Misses       2238     2247       +9     

Impacted Files	Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	50.56% <ø> (ø)
...in/scala/fr/acinq/eclair/channel/ChannelData.scala	100.00% <ø> (ø)
...main/scala/fr/acinq/eclair/io/PeerConnection.scala	85.60% <ø> (ø)
...c/main/scala/fr/acinq/eclair/payment/Invoice.scala	100.00% <ø> (ø)
...la/fr/acinq/eclair/payment/send/PaymentError.scala	20.00% <ø> (ø)
...ire/internal/channel/version0/ChannelCodecs0.scala	96.77% <ø> (ø)
...ire/internal/channel/version1/ChannelCodecs1.scala	98.94% <ø> (ø)
...ire/internal/channel/version2/ChannelCodecs2.scala	100.00% <ø> (ø)
...ire/internal/channel/version3/ChannelCodecs3.scala	100.00% <ø> (ø)
...q/eclair/wire/protocol/LightningMessageTypes.scala	94.59% <ø> (ø)
... and 19 more

[t-bast]

we now accept more invoices (we would previously reject invoices if they required a non invoice feature that we don't support).

But that's not what we want though, we want exactly the opposite behavior.
If the invoice feature bits contain a mandatory feature that we don't know, we must reject that invoice.

I really like having better types, but as I pointed out the first time we explored it, this creates a tension with dynamic data we receive from the external world that might not pass these static checks (and that we still want to allow because we need to be a bit lenient here in case other implementations aren't strict enough with how they use feature bits).

Properly typing this is useful only if we can actually use these types for:

• invoices: but we can't because we don't want to automatically reject invoices that contain non-invoice feature bits
• node announcements: same reason, we actually don't want to enforce it
• init: here we can enforce it, but mostly because all feature bits except option_payment_metadata are init features, so it doesn't bring much

If we were the only implementation in the network, we'd know that external data would follow these rules and could enforce them, but in a decentralized network such as lightning I don't think we can be that strict.

[thomash-acinq]

we now accept more invoices (we would previously reject invoices if they required a non invoice feature that we don't support).

But that's not what we want though, we want exactly the opposite behavior. If the invoice feature bits contain a mandatory feature that we don't know, we must reject that invoice.

I don't think you understand what it does. If the invoice contains a mandatory feature that we don't know, we will reject the invoice, the case when we would accept a previously rejected invoice is it contains a mandatory feature we don't support but that we know is not for invoices.

we don't want to automatically reject invoices that contain non-invoice feature bits

And we won't, we'll just ignore these non-invoice feature bits.

[t-bast]

Ok, then your description was very misleading!

If the invoice contains a mandatory feature that we don't know, we will reject the invoice, the case when we would accept a previously rejected invoice is it contains a mandatory feature we don't support but that we know is not for invoices.

I'm still not sure how that sentence should be interpreted.
I'll go look at the code, it's probably easier than english to verify that it has the right behavior.

And we won't, we'll just ignore these non-invoice feature bits.

But we don't want to ignore them completely, if they are mandatory, we do want to verify that we also support them.
But again, I may be misinterpreting your sentence.

[thomash-acinq]

And we won't, we'll just ignore these non-invoice feature bits.

But we don't want to ignore them completely, if they are mandatory, we do want to verify that we also support them.

Why do we care about non-invoice features ? If we receive an invoice that says "option_anchor_outputs is mandatory" and we don't support anchor outputs, why should we reject the invoice, it means we probably don't have a direct channel with that node as our channel features are incompatible but that doesn't prevent us from paying this invoice.

[t-bast]

Why do we care about non-invoice features ?

Because if it's an unknown feature, it could be an invoice feature, we simply don't know, right? If you see feature bit 64 for example, we don't know what it is, but it may be a new invoice feature that has been introduced and we don't know about. I completely agree that if we see a known, non-invoice feature, then we probably don't care.

I haven't read the code yet, so maybe this is properly handled already?

[thomash-acinq]

Unknown features are processed separately, I'm talking here about features that we know are not invoice features, these are features that we know, or features above 5120 because they don't fit in invoices anyway.

[t-bast]

Great, then that should work!
I'll make comments directly on the code.

[t-bast]

I think we can make that work, and it would be a nice improvement.
We need to test and fix the subtle signature issues, but that shouldn't be impossible to handle.

[thomash-acinq]

It should be ready now.

[t-bast]

Nice, looks good to me! A couple of test improvements and this should be good to go 👍

[t-bast]

LGTM, thanks!
It's good to get that refactoring done!