Skip to content

Conversation

guggero
Copy link
Contributor

@guggero guggero commented Jan 18, 2021

Depends on lightninglabs/pool#202 and the final release of lnd v0.12.0-beta.

Adds compile time and run time compatibility with lnd v0.12.0-beta.
Also pulls in the latest version of lndclient which allows us to cleanly block on startup until lnd is unlocked.
We can also use a single macaroon now instead of needing to specify all subserver macaroons. Either the admin.macaroon is used if --remote.lnd.macaroondir is specified. Or a custom one (must contain all required permissions!) can be specified with --remote.lnd.custommacaroonpath.

Fixes #155.

@guggero guggero requested review from halseth and carlaKC January 18, 2021 14:48
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

I haven't tested the branch yet, but had a couple comments on the approach taken.

@guggero guggero removed the request for review from halseth January 18, 2021 16:22
@jamaljsr
Copy link
Member

jamaljsr commented Jan 18, 2021

Your latest rebase includes an updated lnd proto and generated JS/TS which breaks the CI build. The new proto requires an update to the frontend Typescript types. I think it's fine to keep the old proto in this PR as I will update it separately.

@guggero
Copy link
Contributor Author

guggero commented Jan 18, 2021

The new RPC compile check prevents us from using an old version. Perhaps we need a way to override that.
Or I can update the testdata, should be straightforward.

The check will fail until we reference the final version of lnd anyway since it tries to download it from an URL without the rc tag.

@jamaljsr
Copy link
Member

I agree that the RPC compile check will fail, but the build is also failing on the "typescript compile" step because the frontend code is not updated for v0.12 protos. This also breaks my local docker build because the frontend won't build successfully.

Can you either remove the proto update or update the sampleData.ts file to fix the frontend build?

@guggero
Copy link
Contributor Author

guggero commented Jan 18, 2021

I fixed the sampleData.ts with a temporary commit. Will remove that once we've tagged the final lnd v0.12.0-beta version.

@jamaljsr jamaljsr self-requested a review January 18, 2021 20:21
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK LGTM 👍

Copy link

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Lndclient changes LGTM!

@@ -304,6 +304,7 @@ func (g *LightningTerminal) startSubservers() error {
MacaroonDir: macDir,
TLSPath: tlsPath,
BlockUntilChainSynced: true,
BlockUntilUnlocked: true,
Copy link

Choose a reason for hiding this comment

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

🎉

One question here in the context of lit. Since we just log that we are waiting for lnd to be unlocked, I imagine there won't be any feedback indicating that the user needs to unlock? Still an improvement over crashing (which was the previous behaviour I think?), just wondering about it longer term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you'd only see this in the log at the moment. This will be fully solved by implementing the unlock in the UI itself.

// all of lnd's macaroons. The specified macaroon MUST have all
// permissions that all the subservers use, otherwise permission errors
// will occur.
MacaroonPath string `long:"macaroonpath" description:"The full path to the single macaroon to use, either the admin.macaroon or a custom baked one. Cannot be specified at the same time as macaroondir. A custom macaroon must contain ALL permissions required for all subservers to work, otherwise permission errors will occur."`
Copy link

Choose a reason for hiding this comment

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

Are we going to add the bake command in the readme for a custom macaroon? I remember we discussed this a while back but don't remember exactly what we came up with. Could be nice to point people there if that's the plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we'll probably do that eventually. But because things with macaroons will probably change again to support the "stateless remote mode", we'll probably look at it in that context again.

With the new lndclient version we can specify a single, custom macaroon.
We use the admin macaroon as the custom macaroon in the remote
connection case which removes the need to copy all subserver macaroons
to the host where LiT is running. Users baking custom non-admin
macaroons can also specify that directly with a new configuration
option.
@guggero guggero merged commit 2380c01 into master Jan 27, 2021
@guggero guggero deleted the lnd-12 branch January 27, 2021 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

litd panic after poold RPC request while lnd is locked
3 participants