Flat features by t-bast · Pull Request #1253 · ACINQ/eclair

t-bast · 2019-12-12T16:21:10Z

I removed the awkward --allowMultiPart API parameter to instead rely on nodeParams.features.
This feels cleaner and is how we'll integrate Trampoline support too.

Note that when we update our nodes, we'll need to update our reference.conf:

change the features format
update our features to advertise MPP support

Compatibility tests:

With old eclair nodes, advertizing MPP and PaymentSecret: ok
With c-lightning master: ok
With lnd v0.8.2-beta: ok

Not done in this PR (and should probably be done at some point):

real support for turning off features (e.g. if I turn off MPP, I should use a legacy PayFSM)
better typing to indicate which feature bits go in Init vs ChannelAnn vs NodeAnn vs Invoice

codecov-io · 2019-12-12T16:40:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ea7abd9). Click here to learn what that means.
The diff coverage is 80.95%.

@@            Coverage Diff            @@
##             master    #1253   +/-   ##
=========================================
  Coverage          ?   77.11%           
=========================================
  Files             ?      142           
  Lines             ?     9892           
  Branches          ?      394           
=========================================
  Hits              ?     7628           
  Misses            ?     2264           
  Partials          ?        0

Impacted Files	Coverage Δ
...la/fr/acinq/eclair/payment/relay/NodeRelayer.scala	`97.59% <100%> (ø)`
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala	`80% <100%> (ø)`
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	`83.1% <73.07%> (ø)`
...in/scala/fr/acinq/eclair/channel/Commitments.scala	`90.74% <87.5%> (ø)`

eclair-core/src/main/resources/reference.conf

eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala

t-bast · 2019-12-17T17:59:44Z

I took this opportunity to re-work Features.scala to properly type things.
We're now stricter on features dependencies, as described here.

pm47

Nice change, a few nits, didn't review the tests yet.

For Init I disagree with your approach of changing the type instead of the codec. If you figure it out we could probably do the same for LocalParams/NodeParams.

eclair-core/src/main/resources/reference.conf

eclair-core/src/main/scala/fr/acinq/eclair/Features.scala

eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala

eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala

t-bast · 2019-12-18T15:55:49Z

I fixed the nits in aee1472.

For Init I disagree with your approach of changing the type instead of the codec. If you figure it out we could probably do the same for LocalParams/NodeParams.

I'll investigate that.

Implement lightning/bolts#666 We keep the global/local split in Commitments to avoid backwards incompatibility in the codec.

We instead rely on the MPP feature being set in nodeParams. That means MPP-enabled nodes need to update their reference.conf.

Add types to allow cleaner dependency validation. Most of the time we don't care whether a feature is activated as optional or mandatory, which caused duplicate code. This is now handled more cleanly. It also paves the way to annotate features with the places they should be advertised (Init vs NodeAnn vs ChannelAnn vs invoice).

This allows us to drop the global/local fields from the codebase. They are handled directly at the wire level.

pm47 · 2019-12-30T10:27:19Z

eclair-core/src/main/resources/reference.conf

  trampoline-payments-enable = false // TODO: @t-bast: once spec-ed this should use a global feature flag
-  global-features = "0200" // variable_length_onion
-  local-features = "088a" // initial_routing_sync + option_data_loss_protect + option_channel_range_queries + option_channel_range_queries_ex
+  features = "0a8a" // initial_routing_sync + option_data_loss_protect + option_channel_range_queries + option_channel_range_queries_ex + variable_length_onion


I wonder if we should use a completely different method, using rfcName field:

features { option_data_loss_protect = "mandatory" initial_routing_sync = "optional" gossip_queries = "mandatory" ... }

Less error prone and more readable, but also less flexible.

Would also affect the override-features attribute...

I think it's a good idea if we really want to allow users to turn some features on and off.
The problem is there are quite a few features that we can't really turn off right now (some code paths will ignore the fact that a feature is turned off and still use it).
I think it makes sense to do that change once we've worked on making sure the code base really allows completely turning off all features.

eclair-core/src/main/scala/fr/acinq/eclair/Features.scala

eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageCodecs.scala

eclair-core/src/test/scala/fr/acinq/eclair/wire/LightningMessageCodecsSpec.scala

Due to changes in the features system (#1253), feature graph validation would fail with legacy 1.0.1 phoenix wallet. This check should be disabled as long as there are 1.0.1 Phoenix wallet in the wild.

Due to changes in the features system (#1253), feature graph validation would fail with legacy 1.0.1 Phoenix wallet. This check should be disabled as long as there are 1.0.1 Phoenix wallet in the wild.

Due to changes in the features system (#1253), feature graph validation would fail with legacy 1.0.1 Phoenix wallet. This check should be disabled as long as there are 1.0.1 Phoenix wallet in the wild. (cherry picked from commit 9f8f8e4)

t-bast force-pushed the flat-features branch from a7e49ba to e37c413 Compare December 13, 2019 09:54

t-bast marked this pull request as ready for review December 13, 2019 10:13

araspitzu reviewed Dec 13, 2019

View reviewed changes

eclair-core/src/main/resources/reference.conf Show resolved Hide resolved

eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala Outdated Show resolved Hide resolved

t-bast force-pushed the flat-features branch from c7fde83 to a9feba3 Compare December 17, 2019 09:43

t-bast force-pushed the flat-features branch from ecd3f1b to e2f156d Compare December 18, 2019 14:03

t-bast requested review from pm47 and sstone December 18, 2019 14:03

pm47 requested changes Dec 18, 2019

View reviewed changes

t-bast added 5 commits December 19, 2019 11:34

Flat Features

a6da1db

Implement lightning/bolts#666 We keep the global/local split in Commitments to avoid backwards incompatibility in the codec.

Remove allowMultiPart API field

2a50f17

We instead rely on the MPP feature being set in nodeParams. That means MPP-enabled nodes need to update their reference.conf.

Use new feature types in Trampoline code

120f773

Combine feature in codec, not type

9c595e3

This allows us to drop the global/local fields from the codebase. They are handled directly at the wire level.

t-bast force-pushed the flat-features branch from aee1472 to 9c595e3 Compare December 19, 2019 10:41

Fix an unrelated flaky test

1b2daaa

t-bast force-pushed the flat-features branch from e894f17 to 1b2daaa Compare December 19, 2019 13:07

pm47 reviewed Dec 30, 2019

View reviewed changes

dpad85 mentioned this pull request Jan 8, 2020

"No payment found" error ACINQ/phoenix#15

Closed

pm47 reviewed Jan 8, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/Features.scala Outdated Show resolved Hide resolved

pm47 reviewed Jan 8, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/Features.scala Outdated Show resolved Hide resolved

sstone reviewed Jan 8, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageCodecs.scala Outdated Show resolved Hide resolved

pm47 reviewed Jan 8, 2020

View reviewed changes

eclair-core/src/test/scala/fr/acinq/eclair/wire/LightningMessageCodecsSpec.scala Show resolved Hide resolved

t-bast added 2 commits January 9, 2020 09:24

Refactoring from PR comments

ea7abd9

Merge branch 'master' into flat-features

9330c0b

t-bast requested review from pm47 and sstone January 9, 2020 08:27

Remove redundant default argument of None for hasFeature

9bd3059

pm47 approved these changes Jan 9, 2020

View reviewed changes

sstone approved these changes Jan 9, 2020

View reviewed changes

t-bast merged commit 72338ab into master Jan 9, 2020

t-bast deleted the flat-features branch January 9, 2020 12:47

t-bast mentioned this pull request Jan 9, 2020

missing feature dependency: 9 ACINQ/phoenix#18

Closed

Conversation

t-bast commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

t-bast commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pm47 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-bast commented Dec 18, 2019

Uh oh!

pm47 Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

pm47 Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

t-bast Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

t-bast commented Dec 12, 2019 •

edited

Loading

codecov-io commented Dec 12, 2019 •

edited

Loading

t-bast commented Dec 17, 2019 •

edited

Loading