cfg: move experimental options to main protocol cfg #8744

orbitalturtle · 2024-05-09T13:53:02Z

Change Description

This PR simply moves the experimental custom message config options to the main protocol config, so that the dev build flag doesn't need to be set to use them. Closes issue #8536.

We've gotten multiple requests for this at LNDK -- people don't want to be running LND in dev mode in production. There hasn't been any discussion on the #8536 issue, so hoping to at least spur discussion on what needs to happen to make this change feasible. :)

Tested this with LNDK to confirm it works properly.

Steps to Test

The config options set in sample-lnd.conf should be possible to set without the dev flag.

coderabbitai · 2024-05-09T13:53:07Z

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

ellemouton

Nice - just left comment about implementation.

Also just want to check offline with some folks to check if perhaps a better move would be to add an experimental build tag instead. Will get back to you here about the response.

lncfg/protocol_integration.go

mrfelton · 2024-05-10T08:49:54Z

Also just want to check offline with some folks to check if perhaps a better move would be to add an experimental build tag instead. Will get back to you here about the response.

Ideally, these should be enabled in the default build to make these more accessible.

It would be amazing to get this into 0.18 to enable people to leverage these features without needing to create custom builds with special tags enabled. That would help further adoption of bolt 12 through lndk.

guggero · 2024-05-10T08:59:34Z

cc @saubyk

lncfg/protocol_experimental_off.go

ellemouton · 2024-05-15T09:12:23Z

sample-lnd.conf

+; Set to handle messages of a particular type that falls outside of the
+; custom message number range (i.e. 513 is onion messages)
+; protocol.custom-message=
+
+; Specifies feature bits to advertise in the node's init message.
+; protocol.custom-init=
+
+; Specifies custom feature bits to advertise in the node's announcement message.
+; protocol.custom-nodeann=
+
+; Specifies custom feature bits to advertise in the node's invoices.
+; protocol.custom-invoice=


should we maybe just note that a user can repeat the flags to set multiple bits? Else I think it comes across like they should specify the encoded bit vector

+1, generally we set values in here to help illustrate so maybe fill in the value and note it's the bit position?

Could also improve the cli doc to clarify that while we're here.

That makes sense, just improved those docs.

~~Oh d'oh, just fixing that sample-conf error~~

ellemouton

Code LGTM! one suggestion re the comments in the sample-conf file.

Just need a product-ack from @saubyk on this one too.

Also might need to move the release note entry depending on if this is going in 0.18 or 0.18.1 - again up to @saubyk

saubyk · 2024-05-15T20:10:56Z

Trying to understand the need for this push given we are so close to wrapping up 0.18.
Assuming we have sophisticated users for LNDK, why can’t they do custom build with make install and the tag? Is it because it's a "dev" tag that it appears problematic?

mrfelton · 2024-05-15T20:51:18Z

Trying to understand the need for this push given we are so close to wrapping up 0.18. Assuming we have sophisticated users for LNDK, why can’t they do custom build with make install and the tag?

I understand your concern about the timing of this push for 0.18. While LNDK users may tend to be technically proficient right now (partially because they need to be due to the current barriers to entry, like this one), Interest in Bolt 12 extends beyond advanced users, and LNDK is currently the primary path for enabling partial adoption for the vast majority of Lightning Network users who rely on LND.

I'd summarise as:

Reduced Barrier to Entry: Currently, using these features requires compiling LND with a specific tag, a hurdle for anyone new to the system. By including it in the main configuration, users of all technical backgrounds can simply download the latest LND release (or use the official Lightning Labs Docker image, or spin up a setup using Polar) and install and connect LNDK. Removing the need for custom builds, custom docker images, and enabling more easy integration with tools like Polar would lower the barrier to entry and encourages wider adoption.
Potential Future Integration: Including these options in the main configuration lays the groundwork for future feature integration. This could lead to a more streamlined process for utilizing advanced functionalities in the future, benefiting both new and experienced users.
Early Feedback and Informed Development: Including these features now allows us to gather valuable user feedback and inform future development decisions related to Bolt 12 and LNDK integration. This can help ensure these features meet the needs of the broader user base.

While the 0.18 release cycle is nearing completion, the benefits of broader accessibility, smoother user experience, and early user feedback may make including this change now make sense. It could significantly help to boost adoption and encourage more people to explore these features.

Is it because that it's a "dev" tag that it appears problematic?

That is a problem. The dev tag is intended to be used for development builds. Many people would not want to run development builds in production, but would want to use this specific feature.

saubyk · 2024-05-16T02:51:21Z

Tagged for 0.18. Gated on @carlaKC 's review

carlaKC

tACK, simple code move ✅

carlaKC · 2024-05-16T12:24:43Z

sample-lnd.conf

+; Set to handle messages of a particular type that falls outside of the
+; custom message number range (i.e. 513 is onion messages)
+; protocol.custom-message=
+
+; Specifies feature bits to advertise in the node's init message.
+; protocol.custom-init=
+
+; Specifies custom feature bits to advertise in the node's announcement message.
+; protocol.custom-nodeann=
+
+; Specifies custom feature bits to advertise in the node's invoices.
+; protocol.custom-invoice=


+1, generally we set values in here to help illustrate so maybe fill in the value and note it's the bit position?

Could also improve the cli doc to clarify that while we're here.

lncfg/protocol.go

yyforyongyu · 2024-05-16T17:27:25Z

lncfg/protocol_experimental_on.go

@@ -3,49 +3,7 @@



think the experiment on and off files can also be removed now?

@ellemouton and I had a little convo about this up here: #8744 (comment)

Leaving it in sounds like the right move to me because looking at the commit history, the experimental protocol struct has housed other protocol features in the past, so it could be useful for future experimental flags in the future.

ellemouton reviewed May 10, 2024

View reviewed changes

lncfg/protocol_integration.go Outdated Show resolved Hide resolved

orbitalturtle force-pushed the custom-protocol-cfg branch from 70c3636 to ef3740a Compare May 12, 2024 20:54

ellemouton reviewed May 13, 2024

View reviewed changes

lncfg/protocol_experimental_off.go Show resolved Hide resolved

orbitalturtle force-pushed the custom-protocol-cfg branch 3 times, most recently from 9fbb281 to f22d926 Compare May 14, 2024 14:02

orbitalturtle requested a review from ellemouton May 14, 2024 14:23

ellemouton reviewed May 15, 2024

View reviewed changes

ellemouton approved these changes May 15, 2024

View reviewed changes

orbitalturtle mentioned this pull request May 15, 2024

Does lndk still need a custom branch of LND? lndk-org/lndk#94

Open

saubyk requested a review from carlaKC May 15, 2024 19:19

saubyk assigned orbitalturtle May 16, 2024

saubyk added the config Parameters/arguments/config file related issues/PRs label May 16, 2024

saubyk added this to the v0.18.0 milestone May 16, 2024

carlaKC approved these changes May 16, 2024

View reviewed changes

orbitalturtle force-pushed the custom-protocol-cfg branch 2 times, most recently from 2d65baf to c6293ff Compare May 16, 2024 16:48

yyforyongyu reviewed May 16, 2024

View reviewed changes

cfg: move experimental options to main protocol cfg

e8196c6

orbitalturtle force-pushed the custom-protocol-cfg branch from c6293ff to e8196c6 Compare May 16, 2024 22:17

Roasbeef enabled auto-merge May 16, 2024 23:17

guggero disabled auto-merge May 17, 2024 06:45

guggero merged commit 87d5170 into lightningnetwork:master May 17, 2024
28 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cfg: move experimental options to main protocol cfg #8744

cfg: move experimental options to main protocol cfg #8744

orbitalturtle commented May 9, 2024

coderabbitai bot commented May 9, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

ellemouton left a comment

mrfelton commented May 10, 2024

guggero commented May 10, 2024

ellemouton May 15, 2024

carlaKC May 16, 2024

orbitalturtle May 16, 2024

orbitalturtle May 16, 2024 •

edited

ellemouton left a comment

saubyk commented May 15, 2024 •

edited

mrfelton commented May 15, 2024 •

edited

saubyk commented May 16, 2024

carlaKC left a comment

carlaKC May 16, 2024

yyforyongyu May 16, 2024

orbitalturtle May 16, 2024

cfg: move experimental options to main protocol cfg #8744

cfg: move experimental options to main protocol cfg #8744

Conversation

orbitalturtle commented May 9, 2024

Change Description

Steps to Test

coderabbitai bot commented May 9, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

ellemouton left a comment

Choose a reason for hiding this comment

mrfelton commented May 10, 2024

guggero commented May 10, 2024

ellemouton May 15, 2024

Choose a reason for hiding this comment

carlaKC May 16, 2024

Choose a reason for hiding this comment

orbitalturtle May 16, 2024

Choose a reason for hiding this comment

orbitalturtle May 16, 2024 • edited

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

saubyk commented May 15, 2024 • edited

mrfelton commented May 15, 2024 • edited

saubyk commented May 16, 2024

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC May 16, 2024

Choose a reason for hiding this comment

yyforyongyu May 16, 2024

Choose a reason for hiding this comment

orbitalturtle May 16, 2024

Choose a reason for hiding this comment

CodeRabbit Configration File (`.coderabbit.yaml`)

orbitalturtle May 16, 2024 •

edited

saubyk commented May 15, 2024 •

edited

mrfelton commented May 15, 2024 •

edited