Skip to content
This repository has been archived by the owner on Sep 18, 2022. It is now read-only.

Add more robust check for locked wallet #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JVillella
Copy link
Contributor

I've been testing the latest LND w/ Umbra and have found it is doesn't try to unlock the wallet due to a faulty is locked check. This should be more robust from looking at the LND code.

Copy link
Contributor

@AaronDewes AaronDewes left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @JVillella! Have you sucessfully tested this?

@JVillella
Copy link
Contributor Author

Yep, been running locally for a few days now.

@lukechilds
Copy link
Member

lukechilds commented Jun 10, 2021

Thanks so much for this @JVillell!

So this isn't an immediate issue but will effect LND v0.13.0 once released, right?

@JVillella
Copy link
Contributor Author

It's a fix needed for LND 0.13, that's right - but it's backwards compatible.

Copy link
Contributor

@AaronDewes AaronDewes left a comment

Choose a reason for hiding this comment

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

  1. We probably don't need compatibility with older LND versions, once we release Umbrel with LND 0.13, this PR can be merged at the same time.

  2. This is not the recommended way of checking if LND is unlocked:

image

@JVillella
Copy link
Contributor Author

  1. I would recommend backwards compatibility. Not everyone is going to update their lnd docker node in their compose file.
  2. Nice to know they have a new service; I was following their approach internally but looks like there are better ways to do this now. I won't have time to make these changes, so feel free to replace this PR or add to it with what's required.

@AaronDewes
Copy link
Contributor

Not everyone is going to update their lnd docker node in their compose file.

To not do that, they would need to fork/modify Umbrel, which Umbrel doesn't need to care about in my opinion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants