Can't use new LND/RTL Channel Backup functionality

maltokyo commented 5 years ago

LND and RTL now allow the backups of all channels, and in BTCPay Server it even looks like it works out of the box in RTL (it tells that backups are on and "verified", a false sense of security).

I asked here whether the mnemonic is required to use channel backups and recover from them, and the answer is yes. https://github.com/lightningnetwork/lnd/issues/3371#issuecomment-518237883

But, as here, we can't use this (without hacks) on BTCPay server - any chances to have backups available by default? https://github.com/btcpayserver/btcpayserver-docker/issues/167

Roasbeef commented 5 years ago

@NicolasDorier you can use REST, no need for gRPC. Also if you're starting nodes /w --noseedbackup, then you'll need to interact with the WalletUnlocker in order to initialize the node. Otherwise, it'll just sit there on start up, waiting for a new seed.

NicolasDorier commented 5 years ago

Cool, I was not seeing the wallet unlocker on https://api.lightning.community/rest/

Otherwise, it'll just sit there on start up, waiting for a new seed.

Yes, except if I modify this part, should not be huge deal to maintain.

maltokyo commented 5 years ago

@NicolasDorier anything I need to test?

NicolasDorier commented 5 years ago

@maltokyo not to test but to code as well. :p

maltokyo commented 5 years ago

I would if I could ;(

NicolasDorier commented 5 years ago

You definitely can... with enough time :p

maltokyo commented 5 years ago

Hehe. Right now I’d cause you more pain than I can help. But if there are other ways to help let me know!

maltokyo commented 5 years ago

@NicolasDorier I have just setup a new server. And temporarily changed the lnd docker compose fragment to noseedbackup=0 directly in the file before first setup. Now node is syncing. Will this be enough to get the seed later? Next step is to work out how I actually will get it.

maltokyo commented 5 years ago

So, the workaround worked well, I got the LND wallet generation dialogue and could save my LND CIPHER SEED.

All that is needed is, on a fresh install (before running for the first time) change the noseedbackup=1 to noseedbackup=0 in file: btcpayserver-docker/docker-compose-generator/docker-fragments/bitcoin-lnd.yml

Then after btc chain finishes syncing run command: ./bitcoin-lncli.sh create

I guess after you do this once, it doesn't matter after that if upgrades change it back to noseedbackup=1 in the yml file above. As this is only relevant if you are creating a wallet anyway. Also you will need to enter a passphrase for LND wallet each time you start, but that is not a big deal. All in all, the workaround is 5min extra work for a new install.

I think you should make all new users do this, even the "hard way" for now. This would make your unbacked-up userbase stop expanding at least.

woutersamaey commented 5 years ago

@maltokyo, what are the steps if you have a previous setup, but never used LND on that install, please?

maltokyo commented 5 years ago

Thats even easier.

change the noseedbackup=1 to noseedbackup=0 in file: btcpayserver-docker/docker-compose-generator/docker-fragments/bitcoin-lnd.yml

then enable LND with these two commands:

export BTCPAYGEN_LIGHTNING="lnd"
. ./btcpay-setup.sh -i

Then create wallet with ./bitcoin-lncli.sh create

woutersamaey commented 5 years ago

@maltokyo Actually I meant that I already had done step 2, but have not enabled lightning in the UI, nor put or received any funds or created channels. So in other words, I’m ok if LND is reset or lost.

maltokyo commented 5 years ago

in that case, first srop the server, and then obliterate the files in /var/lib/docker/volumes/generated_lnd_bitcoin_datadir and /var/lib/docker/volumes/generated_lnd_bitcoin_rtl_datadir

and rerun entire export BTCPAYGEN_LIGHTNING="lnd" and . ./btcpay-setup.sh -i

I have not tested it, but it may work.

NicolasDorier commented 5 years ago

@maltokyo you should not change the fragment. Instead, you should do your own fragment (see the documentation in btcpayserver-docker) as you will be unable to update otherwise.

I can't just change from 0 to 1 for two reasons:

It would require user with 0 to do some SSH hackery to get back their money
It would require work for auto unlocking, so that if the server restart, you don't have to manually unlock.

If you can solve those two problems then I can switch by default.

maltokyo commented 5 years ago

Hi @NicolasDorier Thanks for getting back. I understood its a workaround, and the limitations. I can upgrade, as even if an upgrade now switches noseedbackup to "1", it will not affect me, as I have already created the wallet with a cypher seed.

What I mean is, that you should be able to detect a brand new install on the server (where there is no wallet in existence) and simply then tell the user (in your setup instructions here on github) to open the command line and run a couple of commands.

NicolasDorier commented 5 years ago

simply then tell the user (in your setup instructions here on github) to open the command line and run a couple of commands.

I will never lock the user out of their funds after an update
I will never requires users to run command lines if things are not broken
Detecting a brand new install is not easy thing, PR welcome if you have time to find a way.

maltokyo commented 5 years ago

I get all you’re saying. I’m simply letting people know that the workaround worked. I’ll stop giving further suggestions and ideas for change, as it feels unappreciated / unwanted somehow. I’m sure those of you who are good at coding will work it out one day. Coding part is not possible for me at the moment, as mentioned above.

woutersamaey commented 5 years ago

@NicolasDorier what if we just ask users if it is okay to reset their LND (including a thorough explanation of why and the consequences)? If we can’t detect the need in software, we can ask the user. I guess there will be plenty of installs that have not used LND yet and would not mind a reset. Others could be given the opportunity to switch to the new setup.

What would the auto-unlocking look like in your view? Can you break it down in actionable steps so I can follow and maybe think about (help) building that.

NicolasDorier commented 5 years ago

@woutersamaey I can't break it down in steps, this is difficult (I thought just removing the locking feature by changing LND in our fork is the simplest way to get it done).

what if we just ask users if it is okay to reset their LND (including a thorough explanation of why and the consequences)?

Well if you find a way to do that that don't lock them out of their instance when they update, why not. But I can't tell you how.

NicolasDorier commented 5 years ago

Our fork is here: https://github.com/btcpayserver/lnd/tree/dockerfile2 so feel free to try to do it

NicolasDorier commented 5 years ago

I’ll stop giving further suggestions and ideas for change, as it feels unappreciated / unwanted somehow.

@maltokyo , this is not the case

junderw commented 5 years ago

Re: detecting fresh install / version

Can't you just add a version file in the docker volume?

Then when someone updates / installs:

If no version file the version is < whenever you added it.
If version file, it's the version written in the file.
If no volume (ie. the proper folders are not there) it's a new install.

junderw commented 5 years ago

imo LND's backup method (including seed) is superior to the other lightning implementations. (Though Electrum is also looking promising)

It's a shame that new users of BTCPayServer can't utilize it and the number of users stuck in limbo just keeps growing and growing.

I understand Nicolas's side, and I understand Laolu's side. Both are correct, and it looks like everything is in a stalemate.

Laolu adding xprv export to LND will encourage other projects to do hackey things that are not good for the user. (Though I understand why BTCPay thinks this is the only option for existing users without using command line)

BTCPay adding some sort of versioning that prevents update for existing users of LND but letting new users of LND have the seed and use backup feature is difficult to code, requires time, and is hard to work about with the way BTCPay is currently setup.

This whole situation saddens me quite a bit.

Anything I can do to help fix this issue and allow users going forward to use LND with its full capabilities (aezeed awesomeness and channel backups) I am all ears and willing to help.

NicolasDorier commented 5 years ago

@junderw there is a solution we can add to our fork https://github.com/btcpayserver/btcpayserver-docker/issues/179#issuecomment-520198428 , if you have time to talk and look into it, we can do.

This will allow new user to get the seed, without breaking current users. Letting them upgrade at their own leisure.

junderw commented 5 years ago

@NicolasDorier What does "create a new mode" mean?

Like "run this command to install BTCPay+clightning, this command to install BTCPay+LND_OLD, and this command to install BTCPay+LND_NEW"?

IMO Roasbeefs method of tapping the getinfo HTTP REST API and if it fails, restart LND with the noseedbackup flag... seems much easier... but I would be up for either one.

As long as new users can get their seeds, we can just encourage non-techie users to just close down all channels manually via RTL and restart their BTCPay instance from scratch, get their seed and have automated backups.

There is a huge incentive to do this, so unless they are a combination of lazy / not-doing-enough-business-to-merit-backups-anyways-in-their-mind they will probably switch.

NicolasDorier commented 5 years ago

Like "run this command to install BTCPay+clightning, this command to install BTCPay+LND_OLD, and this command to install BTCPay+LND_NEW"?

I don't want to do this. It involves too much support stress and way to lose money. I can only warn users that does not have the seed in the LND page of BTCPay that they should close channels and move funds, and run a specific command to delete their LND instance. (pointing to a doc which explain step by step)

I don't want to support two modes. New users should have the seed, old user does not. There should have no way to run into "old user mode" via a separate command.

IMO Roasbeefs method of tapping the getinfo HTTP REST API and if it fails, restart LND with the noseedbackup flag... seems much easier... but I would be up for either one.

Maybe. Though you need to do it in bash, through the docker-entrypoint.sh. My guess is that it is easier to code in Go in our fork.

There is a huge incentive to do this, so unless they are a combination of lazy / not-doing-enough-business-to-merit-backups-anyways-in-their-mind they will probably switch.

Sure, I just don't want to force them if they don't care. Personally I have around 1K in my LND node, I don't have the seed, I don't care about the seed so I would not migrate. If I lose 1K, that's fine by me. I prefer taking the risk than the burden of closing my channels.

junderw commented 5 years ago

I don't want to do this. It involves too much support stress and way to lose money. I can only warn users that does not have the seed in the LND page of BTCPay that they should close channels and move funds, and run a specific command to delete their LND instance. (pointing to a doc which explain step by step)

I don't want to support two modes. New users should have the seed, old user does not. There should have no way to run into "old user mode" via a separate command.

ok, my original question was "What does "create a new mode" mean?" and I gave an idea of what I thought you meant.

I understand my idea of what you mean was wrong, but could you explain what a "we will do a mode" is referring to? (from your comment that you gave me the link to)

NicolasDorier commented 5 years ago

@junderw yes basically the following:

Let's call this new mode noseedbackup=2. We would set all users to noseedbackup=2.

If starting with noseedbackup=2:

No wallet is detected => Create the wallet, with a automatically generated seed, and write the seed in a clear text file (seed.txt) next to the wallet.db.
Wallet is detected and no seed.txt => Assume same behavior as noseedbackup=1
- If that fail, fallback in same behavior as noseedbackup=0
Wallet is detected and seed.txt is present => Unlock the wallet automatically using seed.txt and hello password.
- If that fail, fallback in same behavior as noseedbackup=0

By doing this we make sure that old user are not disrupted, new user get their seed, and we don't need to code plumbing in bash or btcpay to auto unlock things. This is easier to maintain for us.

Users wanting to use official LND (ie. they don't use btcpayserver-docker) will have to code their own auto unlocker or manually unlock if their server reboot.

junderw commented 5 years ago

@NicolasDorier

noseedbackup is a bool in the code currently. 2 is not a valid value.
Maintaining the LND fork with a complete rewrite of the init code (which is what you are planning) is much harder to maintain than maybe 5-10 lines of bash code that will work as long as the init REST interface doesn't change. (@Roasbeef I'm guessing can chime in on the stable-ness of these interfaces)
Regardless of this feature or other features, adding a version file to the docker volume seems like a better way to manage versioning rather than "if x file exists, this, if y file exists, that"

NicolasDorier commented 5 years ago

noseedbackup is a bool in the code currently. 2 is not a valid value.

This can be changed in our fork. (or just using a different config flag)

Maintaining the LND fork with a complete rewrite of the init code (which is what you are planning) is much harder to maintain than maybe 5-10 lines of bash code that will work as long as the init REST interface doesn't change.

I am not sure about that, but if you find a better way to do it in the docker entrypoint, try and let me know. But in anyway, we will need to change the LND code so having changes in two different places is not super good either.

Regardless of this feature or other features, adding a version file to the docker volume seems like a better way to manage versioning rather than "if x file exists, this, if y file exists, that"

We can't assume such versioning file exists, as right now it does not exists and we don't know from which version users are upgrading from. So even with a versioning file, we need to have a check if x file exists.

If you can make such mode without modifying LND only with some bash smartness, then I am all for it. Unsure it will be simpler.

junderw commented 5 years ago

But in anyway, we will need to change the LND code

I looked at your changes (here's a github link): https://github.com/btcpayserver/lnd/compare/add905d..3f62988?w=1

Ignoring the extra dockerfiles (which really isn't a patch of lnd) you have maybe changed about 10-20 lines and the patch boils down to "disable TLS" and "disable strict RPC user/pass checks" and the changes are isolated into areas not likely to have conflicts moving forward.

To fix the startup methods moving from a two step method to a single step with auto-generating seed.txt you will easily need to change a couple 100s of lines... these lines will be scattered through files that are changed often.

I think using bash logic is much quicker... I think you will realize this once someone actually sits down and tries to change this part of LND, it is a lot more work than adding a config param or adding an if statement... whereas I think bash will be just a couple of if statements and maybe some sleep + while combos in the case of retry.

I think I might have some time next week

NicolasDorier commented 5 years ago

Sure, you can try. I am not convinced it is that hard in Go. I don't have to keep the noseedbackup=0 and =1 to work at all. If it is too complicated to fallback, I can just crash LND instead for example.

rockstardev commented 4 years ago

OK, migration script is almost ready... I'm taking final round of feedback here: https://github.com/btcpayserver/lnd/pull/4

Once deployed new script will maintain existing LND deployments and user will be warned in UI to migrate... here is preview of how it will look:

New LND deployments will be auto initialized with seed and then on subsequent starts LND wallet will be auto unlocked so it can be used by BTCPayServer. User will be able to obtain his seed by simply reading plain text file placed next to wallet.db. If they want they can then remove the seed as it's not needed for future wallet unlocks.

rockstardev commented 4 years ago

Final call for review of that autoinit bash script... we are moving to testing soon. @Roasbeef @cfromknecht @maltokyo any feedback?

maltokyo commented 4 years ago

Looks great for me. I already run my node with noseedbackup=0 from when I set it up by creating a custom lnd.yaml file. I will have to work out how to migrate back over to standard once this PR is incorporated.

rockstardev commented 4 years ago

@maltokyo 👍

I'm working on UI to expose seed now per @NicolasDorier directions... will update this issue with link to PR once it's ready... we are going for different look than what I posed 12 days ago.

btcpayserver / btcpayserver-docker

Can't use new LND/RTL Channel Backup functionality #179