lncli: remote peer alias in `listchannels` #7322

hieblmi · 2023-01-15T09:23:49Z

Implements feature #6994, continuation of PR #7006

This PR introduces a peer_alias field for channels that are retrieved by the ListChannels RPC. By default the peer alias lookup is disabled to avoid degradation of the performance of existing clients. However it can be enabled by passing a new flag peerAliasLookup to ListChannels. For convenience lncli listchannels enables the lookup by default but it can be disabled by a new flag skip_peer_alias_lookup.

rpcserver.go

guggero

Nice feature, thanks a lot!
Looks like there are a couple of failures in the linter and unit tests. But other than that, looks pretty good to me.

lnrpc/lightning.proto

rpcserver.go

lntest/itest/lnd_misc_test.go

lightninglabs-deploy · 2023-02-06T10:11:13Z

@hieblmi, remember to re-request review from reviewers when ready

hieblmi · 2023-02-06T22:51:40Z

Thanks for the review @guggero, I missed the test case errors but fixed them now and also addressed your nits.

guggero

Very nice, LGTM 🎉

rpcserver.go

guggero · 2023-02-10T09:42:28Z

lntemp/harness_assertion.go

@@ -345,12 +345,15 @@ func (h *HarnessTest) AssertChannelExists(hn *node.HarnessNode,
 // findChannel tries to find a target channel in the node using the given
 // channel point.
 func (h *HarnessTest) findChannel(hn *node.HarnessNode,
-	chanPoint *lnrpc.ChannelPoint) (*lnrpc.Channel, error) {
+	chanPoint *lnrpc.ChannelPoint, peerAliasLookup bool) (*lnrpc.Channel,


We could use a variadic argument list with functional options to avoid needing to add the new parameter everywhere. And then just use withPeerLookup() if you want to enable it. Example, in case it's not clear what I mean: https://github.com/lightninglabs/lndclient/blob/master/lightning_client.go#L2786

Thanks for the hint, this looks a little cleaner now.

lntest/itest/lnd_misc_test.go

guggero

Great work, thank you!
One final nit in the itest, otherwise looks good to me 🎉

lntemp/harness.go

ellemouton

LGTM after Oli's comment about options has been addressed :)

Just need to update the release notes entry with the correct link too 🙏

docs/release-notes/release-notes-0.16.0.md

hieblmi · 2023-02-24T06:50:54Z

~~Seems like test framework changes have removed lntemp, have to fix before this is ready to review.~~ @guggero Addressed nits and test framework changes.

guggero

LGTM 🎉

@saubyk given RC1 was just cut, can we still get this in for 0.16?

guggero · 2023-02-24T09:06:58Z

itest/lnd_misc_test.go

+	opts := []lntest.ListChannelOption{lntest.WithPeerAliasLookup()}
+
+	aliceChannelWithAlias := ht.QueryChannelByChanPoint(
+		alice, chanPoint, opts...,


The nice thing about these options is, you can just do alice, chanPoint, lntest.WithPeerAliasLookup() here. But non-blocking, since the diff is now much cleaner already.

Changed it here and below. I thought having the opts field might come in handy in the future to add options to it. But it isn't very likely that we add additional options right here.

saubyk · 2023-02-24T19:57:40Z

LGTM 🎉

@saubyk given RC1 was just cut, can we still get this in for 0.16?

We can, given that it's good to go.

alexbosworth · 2023-03-01T01:07:31Z

If there is an alias lookup problem for a peer it will return unable to lookup peer alias: alias for node not found error

hieblmi · 2023-03-01T06:28:49Z

If there is an alias lookup problem for a peer it will return unable to lookup peer alias: alias for node not found error

Are you pointing to the redundant wording of the error message? Should node be replaced with peer?

guggero · 2023-03-01T07:59:22Z

No, I think it's about returning the error vs. logging the error. As was discussed in #7001 (comment)...
@alexbosworth do you know why your peer doesn't have an alias? Is that something that can happen often or maybe a problem with your node's graph view (which might be fixed with the gossip propagation fix)?
I wonder if we should fully ignore (and not even log) any errors related to fetching the alias.

hieblmi · 2023-03-01T12:14:34Z

No, I think it's about returning the error vs. logging the error. As was discussed in #7001 (comment)...

Oh right, initially I thought of returning an empty alias if an error during the lookup happens so that we don't interrupt it as it is flagged now.

alexbosworth · 2023-03-01T15:18:27Z

No, I think it's about returning the error vs. logging the error. As was discussed in #7001 (comment)... @alexbosworth do you know why your peer doesn't have an alias?

I tried it in a regtest scenario and didn't have no alias on purpose but it maybe can happen while the graph is still syncing?

guggero · 2023-03-01T15:20:55Z

Okay, I guess we should just ignore (and not even log) the errors related to fetching the peer alias. If a user wants to know why an alias isn't showing, they can do lncli getnodeinfo where they would get the same error if something with the graph is not okay. @hieblmi can you create a PR that removes the error return in listchannels and fwdinghistory please?

hieblmi · 2023-03-01T16:08:43Z

@guggero I will submit it in a bit.

hieblmi commented Jan 15, 2023

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

hieblmi force-pushed the cmd-listchannels-peer-alias branch 3 times, most recently from ac91882 to a675f76 Compare January 16, 2023 12:07

hieblmi requested a review from guggero January 29, 2023 13:50

hieblmi commented Jan 29, 2023

View reviewed changes

rpcserver.go Show resolved Hide resolved

hieblmi force-pushed the cmd-listchannels-peer-alias branch from a675f76 to 321177a Compare January 29, 2023 14:03

guggero reviewed Jan 30, 2023

View reviewed changes

lnrpc/lightning.proto Outdated Show resolved Hide resolved

lnrpc/lightning.proto Outdated Show resolved Hide resolved

rpcserver.go Outdated Show resolved Hide resolved

lntest/itest/lnd_misc_test.go Outdated Show resolved Hide resolved

guggero mentioned this pull request Jan 30, 2023

Add peer_scid_alias field to lnrpc.Channel #7366

Merged

7 tasks

hieblmi force-pushed the cmd-listchannels-peer-alias branch from 321177a to 228a617 Compare February 2, 2023 10:42

hieblmi force-pushed the cmd-listchannels-peer-alias branch from 228a617 to a65abe0 Compare February 6, 2023 21:54

hieblmi requested a review from guggero February 6, 2023 22:51

guggero approved these changes Feb 10, 2023

View reviewed changes

hieblmi mentioned this pull request Feb 23, 2023

[feature]: listchannels output should include node alias #6994

Closed

hieblmi force-pushed the cmd-listchannels-peer-alias branch 4 times, most recently from 1b61ae2 to f51aa75 Compare February 23, 2023 08:57

guggero approved these changes Feb 23, 2023

View reviewed changes

lntemp/harness.go Outdated Show resolved Hide resolved

guggero requested a review from ellemouton February 23, 2023 09:09

ellemouton approved these changes Feb 23, 2023

View reviewed changes

docs/release-notes/release-notes-0.16.0.md Outdated Show resolved Hide resolved

hieblmi force-pushed the cmd-listchannels-peer-alias branch 2 times, most recently from 7d4c275 to 66cf6d8 Compare February 23, 2023 20:38

hieblmi requested a review from guggero February 23, 2023 21:20

hieblmi force-pushed the cmd-listchannels-peer-alias branch 2 times, most recently from 1e1f145 to d2c9af5 Compare February 24, 2023 06:07

hieblmi requested review from ellemouton and removed request for guggero February 24, 2023 06:33

hieblmi requested review from guggero and ellemouton and removed request for ellemouton and guggero February 24, 2023 06:33

hieblmi force-pushed the cmd-listchannels-peer-alias branch 2 times, most recently from d9104cc to cdd8b14 Compare February 24, 2023 07:48

hieblmi requested review from guggero and removed request for ellemouton February 24, 2023 08:43

guggero approved these changes Feb 24, 2023

View reviewed changes

hieblmi force-pushed the cmd-listchannels-peer-alias branch 3 times, most recently from 1fa7d78 to 313983f Compare February 24, 2023 09:48

hieblmi added 5 commits February 24, 2023 10:49

lnrpc: peer_alias field for Channel message

edb58a8

lncli: add flag skip_peer_alias_lookup to listchannels

9d048ec

rpcserver: enrich ListChannels with peer alias

3e5943b

itest: check peer alias in ListChannels

4c198bd

docs: update release notes

313983f

guggero merged commit d321182 into lightningnetwork:master Feb 24, 2023

saubyk added this to the v0.16.0 milestone Feb 24, 2023

hieblmi mentioned this pull request Mar 1, 2023

rpc: assign peer alias lookup error string #7471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lncli: remote peer alias in `listchannels` #7322

lncli: remote peer alias in `listchannels` #7322

hieblmi commented Jan 15, 2023 •

edited

Loading

guggero left a comment

lightninglabs-deploy commented Feb 6, 2023

hieblmi commented Feb 6, 2023

guggero left a comment

guggero Feb 10, 2023

hieblmi Feb 23, 2023

guggero left a comment

ellemouton left a comment

hieblmi commented Feb 24, 2023 •

edited

Loading

guggero left a comment

guggero Feb 24, 2023

hieblmi Feb 24, 2023 •

edited

Loading

saubyk commented Feb 24, 2023

alexbosworth commented Mar 1, 2023

hieblmi commented Mar 1, 2023 •

edited

Loading

guggero commented Mar 1, 2023

hieblmi commented Mar 1, 2023

alexbosworth commented Mar 1, 2023

guggero commented Mar 1, 2023

hieblmi commented Mar 1, 2023

lncli: remote peer alias in listchannels #7322

lncli: remote peer alias in listchannels #7322

Conversation

hieblmi commented Jan 15, 2023 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Feb 6, 2023

hieblmi commented Feb 6, 2023

guggero left a comment

Choose a reason for hiding this comment

guggero Feb 10, 2023

Choose a reason for hiding this comment

hieblmi Feb 23, 2023

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

hieblmi commented Feb 24, 2023 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

guggero Feb 24, 2023

Choose a reason for hiding this comment

hieblmi Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

saubyk commented Feb 24, 2023

alexbosworth commented Mar 1, 2023

hieblmi commented Mar 1, 2023 • edited Loading

guggero commented Mar 1, 2023

hieblmi commented Mar 1, 2023

alexbosworth commented Mar 1, 2023

guggero commented Mar 1, 2023

hieblmi commented Mar 1, 2023

lncli: remote peer alias in `listchannels` #7322

lncli: remote peer alias in `listchannels` #7322

hieblmi commented Jan 15, 2023 •

edited

Loading

hieblmi commented Feb 24, 2023 •

edited

Loading

hieblmi Feb 24, 2023 •

edited

Loading

hieblmi commented Mar 1, 2023 •

edited

Loading