-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
cfg: move experimental options to main protocol cfg #8744
cfg: move experimental options to main protocol cfg #8744
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
cc @saubyk |
70c3636
to
ef3740a
Compare
9fbb281
to
f22d926
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand the need for this push given we are so close to wrapping up 0.18. |
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:
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.
That is a problem. The |
Tagged for 0.18. Gated on @carlaKC 's review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK, simple code move ✅
2d65baf
to
c6293ff
Compare
@@ -3,49 +3,7 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think the experiment on and off files can also be removed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
c6293ff
to
e8196c6
Compare
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.