Skip to content

Conversation

@araspitzu
Copy link
Contributor

@araspitzu araspitzu commented May 25, 2020

Currently we always merge the features defined by the user in eclair.conf with our reference.conf file, this led to some unexpected behaviors as described in #1434.
The PR corrects this behavior by making sure we never merge our default features with the user defined features, if the user provides some value then we use only that and if it doesn't we fallback to our hard-coded default features. Note that this makes impossible for a user to completely disable all features (i.e setting features { }) but i think it's okay because it doesn't affect any actual use-case.

Because of #1434 we need a way to let the user disable the default features. This PR explicitly allows to disable any feature from the configuration by setting disabled near the feature definition, all features are defined in the reference conf even if not active by default.

@araspitzu araspitzu force-pushed the human_readable_features_no_merge branch from 826716e to bcd385e Compare June 5, 2020 12:57
@pm47 pm47 force-pushed the human_readable_features_no_merge branch from bcd385e to 18b6edc Compare July 27, 2020 13:57
@pm47
Copy link
Member

pm47 commented Jul 27, 2020

Rebased, and added a commit to improve the toString:

features=initial_routing_sync:optional,option_data_loss_protect:optional,var_onion_optin:mandatory
features=gossip_queries_ex:optional,basic_mpp:optional (unknown=5)
features= (unknown=25,28,30)

@pm47 pm47 requested a review from t-bast July 27, 2020 13:59
payment_secret = optional
basic_mpp = optional
option_support_large_channel = optional
trampoline_payment = disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we also missing option_static_remotekey = disabled and keysend = disabled if we need to specify all features?

Copy link
Member

@pm47 pm47 Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shows the ambiguity that "not defined by default" == "defined as disabled" 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it doesn't look like we need to explicitly disable not-defined-by-default features...
So we should either:

  • remove that line and keep option_static_remotekey and keysend absent as well
  • or include them all?

I slightly prefer the second option where the reference.conf lists all supported features, with some of them disabled.
It makes it easier for users to figure out what features they can enable/disable without having to look at the actual code.

Copy link
Member

@pm47 pm47 Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it doesn't look like we need to explicitly disable not-defined-by-default features...

Indeed, I see the disabled one as an example on how to explicitly disable other features that are enabled by default.

I slightly prefer the second option where the reference.conf lists all supported features

I bet new features will be forgotten...

The more I think about it, the more I think we should just rely on the null trick as laid out in the HOCON doc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I see the disabled one as an example on how to explicitly disable other features that are enabled by default.

Got it, that makes sense.

I bet new features will be forgotten...

Clearly :), but it's not an issue! We can add them when we realize we forgot some. I still think it's slightly better for users to have them all listed here, they clearly see what's disabled by default (experimental or obsolete).

The more I think about it, the more I think we should just rely on the null trick

Meh. I prefer explicitly disabling stuff! It would be a problem if we had like 100 features, but given our rate of adding new feature bits we should be ok :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, done in deca797.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@pm47 pm47 merged commit 8cd2644 into master Jul 27, 2020
@pm47 pm47 deleted the human_readable_features_no_merge branch July 27, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants