lightninglabs / lightning-terminal

Lightning Terminal: Your Home for Lightning Liquidity
MIT License
488 stars 82 forks source link

Bump litd to version `v0.12.3-alpha` #719

Closed ViktorTigerstrom closed 4 months ago

ViktorTigerstrom commented 4 months ago

This PR bumps the litd version to v0.12.3-alpha.

This PR also bumps lnd to v0.17.4-beta, loop to v0.27.0-beta and lightning-node-connect to v0.3.0-alpha and adds the new included protos.

I've tested its compatibility with:

ViktorTigerstrom commented 4 months ago

A question for reviewers: Do you think it'd make sense to add the gbn Subsystem to to the logger, i.e. here? https://github.com/lightninglabs/lightning-terminal/blob/abf231b379c04ee3788ba9165779eee04b4ede8a/log.go#L70

That way users could provide us with more detailed info when the LNC connection fails. The downside being though, that the gbn subsystem has pretty verbose logging in debug + trace log level.

ViktorTigerstrom commented 4 months ago

Rebased to resolve merge conflict.

I don't have a strong opinion on this. It would be a bit of an annoyance seeing log messages every second in debug, but that can be avoided with a custom debuglevel. To me this isn't a huge deal.

Thanks! Would also be great to get @guggero & @ellemouton's opinions regarding this as well :)

ellemouton commented 4 months ago

The downside being though, that the gbn subsystem has pretty verbose logging in debug + trace log level.

Maybe there are logs in that area that should be downgraded a level before we add the logs here?

guggero commented 4 months ago

We should also add taproot-assets v0.3.3 that was just pushed (the release isn't out yet but the tag exists).

I think we should look into the log spamminess, but perhaps in its own PR. For now I think it's okay to just adjust the log level manually if the verbosity is too high for one's taste.

ViktorTigerstrom commented 4 months ago

Thanks for the reviews everyone!

We should also add taproot-assets v0.3.3 that was just pushed (the release isn't out yet but the tag exists).

Great, thanks! Added with the latest push.

Maybe there are logs in that area that should be downgraded a level before we add the logs here? & I think we should look into the log spamminess, but perhaps in its own PR. For now I think it's okay to just adjust the log level manually if the verbosity is too high for one's taste.

Ok agree that we should probably look over the log level spamminess and consider downgrading the log level, but I take it that you both agree that it makes sense to at least add the gbn subsystem to the logger before the release then? I've added it now with the latest push :). Then we can look into the spamminess in separate PRs, potentially also on the LNC repos side, as I for example think the handshake & resending of packets + setting of timeout values is probably more important info than packets being sent back and forth.

ellemouton commented 4 months ago

@ViktorTigerstrom - just how bad is the current default level (or even debug) level of spam from the GBN side?

ViktorTigerstrom commented 4 months ago

@ViktorTigerstrom - just how bad is the current default level (or even debug) level of spam from the GBN side?

So with the default level (info), there's actually no logging in the gbn package as it only has debug & trace log level logging currently.

In debug, there's lot's of useful information that's included, such as all the required info to debug the handshake. Though few seconds basically (3-5), there will be 2 lines added in the log that states the current resend timeout, and sometimes the handshake timeout, which in hindsight is a bit too excessive under debug level. With debug level, all the info about packets being sent back and forth is filtered out also as that's under trace. There's unfortunately no info that packet's have been resent, which we probably should have had a line for under debug level in hindsight.

With the trace level, the log is very verbose, but that's to be expected.

ellemouton commented 4 months ago

current resend timeout

Yeah should probably be trace level. Often it's nice that users run with Debug in case things go wrong...

what do you guys think about a small LNC pr just to change the levels of those logs & then we just point to the commit hash here? (ie, no new LNC version needed)

cc @guggero

If y'all think that majority of users run in info anyways then all good 👍

Gonna add my LGTM so long as this is non-blocking but nice to have

guggero commented 4 months ago

what do you guys think about a small LNC pr just to change the levels of those logs & then we just point to the commit hash here? (ie, no new LNC version needed)

Sure, we can do that. I do normally run everything on debug so I would probably see those logs. But can still customize my log level, so no big deal either way. Depends on how much else is on people's plate currently and how urgent this release is?

ViktorTigerstrom commented 4 months ago

Added an updated LNC dependency which has less verbose logging under debug log level.