Skip to content

Start deprecating noencryptwallet#1843

Merged
Roasbeef merged 9 commits into
lightningnetwork:masterfrom
cfromknecht:change-noencryptwallet
Sep 13, 2018
Merged

Start deprecating noencryptwallet#1843
Roasbeef merged 9 commits into
lightningnetwork:masterfrom
cfromknecht:change-noencryptwallet

Conversation

@cfromknecht
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht commented Sep 5, 2018

This PR begins the process of deprecating the confusing noencryptwallet flag, which has initially designed for testing purposes. As people have started to use this flag in the wild, it cannot be fully removed in one fell swoop due to backwards-compatibility requirements, though this will start the process of deprecating its use.

The primary change in this PR is to rename the flag to noseedbackup, as this the more critical information the user should be aware of when using this setting. The help description printed presents a more alarming tone than the prior message, and includes the caveat that no seed will be provided.

References to noencryptwallet have been migrated to noseedbackup in comments throughout the codebase. The section on disabling wallet encryption has also been removed from the install docs and sample-lnd.conf. Lastly, the docker start script has been changed to pass the noseedbackup flag instead.

NOTE: Any users that are actively using noencryptwallet will have to switch any scripts/confs to usenoseedbackup as a result of this PR, though no further modification should be required.

@cfromknecht cfromknecht force-pushed the change-noencryptwallet branch from 6a6c0e2 to fae2052 Compare September 5, 2018 02:11
@cfromknecht cfromknecht added the breaking Non-backwards-compatible changes label Sep 5, 2018
@cfromknecht cfromknecht force-pushed the change-noencryptwallet branch from fae2052 to 1ea0e0a Compare September 5, 2018 03:52
This commit renames the confusing noencryptwallet
flag to noseedbackup, since this highlights the more
crucial information of the flags behavior to the user.
The description has also been capitalized to urge
the user think twice about what they're doing.
We will be slowly phasing this out, though abruptly
discontinuing support would be a more extensive change.
For now, we will ensure that this feature is not
recommended to users setting up a new wallet.
@cfromknecht cfromknecht force-pushed the change-noencryptwallet branch from 1ea0e0a to 9851870 Compare September 5, 2018 03:52
@Roasbeef Roasbeef added documentation Documentation changes that do not affect code behaviour security General label for issues/PRs related to the security of the software P3 might get fixed, nice to have needs review PR needs review by regular contributors labels Sep 9, 2018
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐣

@Roasbeef Roasbeef merged commit c5ece1e into lightningnetwork:master Sep 13, 2018
@cfromknecht
Copy link
Copy Markdown
Contributor Author

Heads up @NicolasDorier, the --noencryptwallet flag will be renamed to --noseedbackup in 0.5

@NicolasDorier
Copy link
Copy Markdown
Contributor

You should add a proper error message in case a user try to use noencryptwallet because of outdated documentation.

@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Sep 13, 2018 via email

@dannypaz
Copy link
Copy Markdown
Contributor

dannypaz commented Sep 14, 2018

@Roasbeef fails to start with something like lnd_btc_1 | /home/lnd/lnd.conf:18: unknown option: noencryptwallet which helps, but I think @NicolasDorier is asking for a deprecation message instead of a generic failure.

@jonasnick
Copy link
Copy Markdown

post-merge utACK. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Non-backwards-compatible changes documentation Documentation changes that do not affect code behaviour needs review PR needs review by regular contributors P3 might get fixed, nice to have security General label for issues/PRs related to the security of the software

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants