Skip to content
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

Warn user before doing SCB restore #3698

Merged
merged 1 commit into from Dec 5, 2019

Conversation

@aantonop
Copy link
Contributor

aantonop commented Nov 9, 2019

SCB restore closes all open channels, which may result in significant on-chain fees.

The current implementation of lncli create does not warn users.

We need to warn users that this is what will happen if they try to do an SCB restore, and get their affirmative consent.

@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 9, 2019

Please be gentle. This is my first ever golang work.
I tested it on the commandline (lncli create --multi_file ...) and it works with both "y" and "n" on the warning response

@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 10, 2019

Took 7 commits to conform to the go linter expectations. "Better late than never". I don't yet know how to replicate the travis-ci linter rules on my local system, so that I can preview the warnings, so...

@flack

This comment has been minimized.

Copy link

flack commented Nov 11, 2019

would be nice if the warning would only show if there actually are channels (i.e. a warning about closing channels when none exist could also be confusing)

@halseth halseth requested a review from carlaKC Nov 11, 2019
@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 11, 2019

would be nice if the warning would only show if there actually are channels (i.e. a warning about closing channels when none exist could also be confusing)

I agree, we should suppress this warning if there are no open channels. Furthermore, we should show the number of open channels in the warning.

I am not sure if at that stage of lnd's boot process, lncli can access the current open channels. I'll have to see if that is possible. Let me dig around. I'm new to this codebase

@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 13, 2019

From what I can tell, the channel arbiter is not running during the "create" phase of lnd's boot cycle and therefore I can't check open channels. Let me know if I missed something

Copy link
Collaborator

carlaKC left a comment

Thanks for the PR @aantonop! I think that this is going to be really helpful to people 😄
Works as expected, awesome stuff, I just have a few style nits which the contribution guidelines should help you iron out.

I deeply feel your linter pain, also took me a while to get travis happy 😅 The makefile is super helpful in this case, make lint will run linter as on travis. make check and make unit pgk={package to test} are also very useful! More complete docs for that here.

For a change like this, we'd tend to only have on commit, so would you mind doing an interactive rebase and squashing the correction commits down?

And welcome to the golang way of life 🎈

cmd/lncli/commands.go Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
@aantonop aantonop force-pushed the aantonop:warn_before_SCB_restore branch 2 times, most recently from 61e770f to b9b06a9 Nov 19, 2019
@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 20, 2019

Fixed comment formatting, addressed all your review issues, squashed and rebased!

Copy link
Collaborator

carlaKC left a comment

Awesome change, looks good to me 🙌
Needs another reviewer to go in, so I'm requesting from chanbackups wizard @guggero as well.

@carlaKC carlaKC requested a review from guggero Nov 21, 2019
Copy link
Collaborator

guggero left a comment

Thank you for your contribution, @aantonop!
It's a great addition to the other SCB bug fixes that are coming soon.
I tested the code and it works as expected.

So I'm sorry that I have to be nit picking again, mostly on the formatting of the comments.
I know that not all existing code adheres to the style guidelines but we at least try to get new code to pass the checklist.
I'd be happy if you could fix the one comment I left and the following two points in general:

  • We try to keep the comments as compact as possible so there shouldn't be any extra newlines in there.
  • All code should wrap at 80 characters with the tab character being measured as 8 chars.
cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
SCB restore closes all open channels. We need to warn users that this is what will happen if they try to do an SCB restore.
@aantonop aantonop force-pushed the aantonop:warn_before_SCB_restore branch from b9b06a9 to 651ca47 Nov 21, 2019
@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 21, 2019

Cleaned up comments some more (still getting used to the style conventions of Go and the lnd team). Squashed, rebased and pushed anew.

Copy link
Collaborator

guggero left a comment

Thanks for the quick changes, comments look much better now!
LGTM 💯

restoreSCB = true
break warningLoop
case "n":
restoreSCB = false

This comment has been minimized.

Copy link
@halseth

halseth Nov 26, 2019

Collaborator

If the user provided channel backups, but enters n here, maybe it is more natural to return instead of continuing unlocking the wallet?

@aantonop

This comment has been minimized.

Copy link
Contributor Author

aantonop commented Nov 26, 2019

@Roasbeef Roasbeef added this to the 0.8.2 milestone Nov 27, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 5, 2019

Merging as is so this can make it into 0.8.2, will add on a commit addressing Johan's final comment.

@Roasbeef Roasbeef merged commit 03ee8c0 into lightningnetwork:master Dec 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 62.64%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.