feat: exposes lnduris & connextIdentifier on listpeers grpc call (#2017)

[rsercano]

resolves #2017.

Currently only exposes connext_identifier for connext as @erkarl mentioned in the issue comment, but exposes LND uris.

[kilrau]

In order to resolve #2017, we'd want to have lnd URIs too, I propose to separate the uri part from the pubkey into address, just like we do with xud already, sth like this:

Wdyt? @erkarl @sangaman @raladev

    {
      "address": "127.0.0.1:33870",
      "nodePubKey": "027178f1a99b1b39aeeb53192ad05f617963abba7731d1e7146302e779ed524608",
      "lndUriMap": [
        [
          "BTC",
          "03e2496e71b524a3144aa4897487823803f1cc8a74ff7d912a331f4d5a9e1e6bf0",
          "127.0.0.1:33870"
        ],
        [
          "LTC",
          "03269a47ebba056ba66a6dcee35d5366bbdf845a13f2cef2ef205f6c2fbcfaad83",
          "127.0.0.1:33870"
        ]
      ],
     "connextUriMap": [
        [
          "03e2496e71b524a3144aa4897487823803f1cc8a74ff7d912a331f4d5a9e1e6bf0",
          "127.0.0.1:33870"
        ],
      "inbound": true,
      "pairsList": [
        "ETH/BTC",
        "LTC/BTC",
        "LTC/USDT",
        "BTC/USDT",
        "USDT/DAI"
      ],
      "xudVersion": "1.2.4",
      "secondsConnected": 4127,
      "alias": "BulkSound"
    }
  ]
}

[rsercano]

In order to resolve #2017, we'd want to have lnd URIs too, I propose to separate the uri part from the pubkey into address, just like we do with xud already, sth like this:

Wdyt? @erkarl @sangaman @raladev

    {
      "address": "127.0.0.1:33870",
      "nodePubKey": "027178f1a99b1b39aeeb53192ad05f617963abba7731d1e7146302e779ed524608",
      "lndUriMap": [
        [
          "BTC",
          "03e2496e71b524a3144aa4897487823803f1cc8a74ff7d912a331f4d5a9e1e6bf0",
          "127.0.0.1:33870"
        ],
        [
          "LTC",
          "03269a47ebba056ba66a6dcee35d5366bbdf845a13f2cef2ef205f6c2fbcfaad83",
          "127.0.0.1:33870"
        ]
      ],
     "connextUriMap": [
        [
          "03e2496e71b524a3144aa4897487823803f1cc8a74ff7d912a331f4d5a9e1e6bf0",
          "127.0.0.1:33870"
        ],
      "inbound": true,
      "pairsList": [
        "ETH/BTC",
        "LTC/BTC",
        "LTC/USDT",
        "BTC/USDT",
        "USDT/DAI"
      ],
      "xudVersion": "1.2.4",
      "secondsConnected": 4127,
      "alias": "BulkSound"
    }
  ]
}

I phrased it wrong again, edited my PR, I should think before writing sorry🤔 @kilrau

[kilrau]

I phrased it wrong again, edited my PR, I should think before writing sorrythinking @kilrau

Ok, thx. Let's wait for @sangaman @erkarl to chime in and give an opinion on how the output should look like.

[ghost]

For connext I'd call it for what it is connextIdentifier instead of connextUriMap. The type of it on p2p level can be string since there will only be one per peer.

[raladev]

In order to resolve #2017, we'd want to have lnd URIs too, I propose to separate the uri part from the pubkey into address, just like we do with xud already, sth like this:

Wdyt? @erkarl @sangaman @raladev

    {
      "address": "127.0.0.1:33870",
      "nodePubKey": "027178f1a99b1b39aeeb53192ad05f617963abba7731d1e7146302e779ed524608",
      "lndUriMap": [
        [
          "BTC",
          "03e2496e71b524a3144aa4897487823803f1cc8a74ff7d912a331f4d5a9e1e6bf0",
          "127.0.0.1:33870"
        ],
        [
          "LTC",
          "03269a47ebba056ba66a6dcee35d5366bbdf845a13f2cef2ef205f6c2fbcfaad83",
          "127.0.0.1:33870"
        ]
      ],
     "connextUriMap": [
        [
          "03e2496e71b524a3144aa4897487823803f1cc8a74ff7d912a331f4d5a9e1e6bf0",
          "127.0.0.1:33870"
        ],
      "inbound": true,
      "pairsList": [
        "ETH/BTC",
        "LTC/BTC",
        "LTC/USDT",
        "BTC/USDT",
        "USDT/DAI"
      ],
      "xudVersion": "1.2.4",
      "secondsConnected": 4127,
      "alias": "BulkSound"
    }
  ]
}

not sure why we need divide address and pubkey. I dont see cases when i need only address or only pubkey of the peer. Are they exsist?

[sangaman]

For connext I'd call it for what it is connextIdentifier instead of connextUriMap. The type of it on p2p level can be string since there will only be one per peer.

Agree, connextIdentifier seems like all we need.

not sure why we need divide address and pubkey. I dont see cases when i need only address or only pubkey of the peer. Are they exsist?

I agree as well, I think just one lndUri field (per currency) will do, and I personally think it's a bit excessive to return every lnd listening address. I would just return the first listening address and return it string format, [pubkey]@[ipaddress/hostname]:[port], which in my experience is commonly used when connecting to new nodes/peers in lnd.

As an aside, what is the value in exposing the lnd uri at all? I see that's in the issue but I don't know what the use case is.

[michael1011]

not sure why we need divide address and pubkey. I dont see cases when i need only address or only pubkey of the peer. Are they exsist?

For example you could build an application for the XUD API that queries the public channels of the LND node of the counterparty to check whether you can easily route before taking on order of them. But you could easily split a [pubkey]@[ipaddress/hostname]:[port] by @ so that's not really a dealbreaker.

@sangaman @rsercano which LND URI are we showing when the LND node has more than one?

[rsercano]

not sure why we need divide address and pubkey. I dont see cases when i need only address or only pubkey of the peer. Are they exsist?

For example you could build an application for the XUD API that queries the public channels of the LND node of the counterparty to check whether you can easily route before taking on order of them. But you could easily split a [pubkey]@[ipaddress/hostname]:[port] by @ so that's not really a dealbreaker.

@sangaman @rsercano which LND URI are we showing when the LND node has more than one?

Within this PR we're showing all lnd uris saved in the peer, so not selecting a particular one, but showing all.

[kilrau]

not sure why we need divide address and pubkey. I dont see cases when i need only address or only pubkey of the peer. Are they exsist?

There might be, but even then it will be easy to split the pubkey part since the separator is always a @, I just proposed this for consistency but any way is fine!

Can you give this another spin? @raladev

[rsercano]

@raladev rebased

[ghost]

@erkarl @rsercano Should connextIdentifier be empty for now?

No. It should be the connext public identifier (something like vector123abc...)

[raladev]

@erkarl @rsercano Should connextIdentifier be empty for now?

No. It should be the connext public identifier (something like vector123abc...)

checked rebased version, connextIdentifier is still empty @rsercano

[sangaman]

@sangaman @rsercano which LND URI are we showing when the LND node has more than one?

Within this PR we're showing all lnd uris saved in the peer, so not selecting a particular one, but showing all.

That part still excessive to me. Most nodes only expose one address anyway (my lnd node has 20 peers and none with more than 1 address) and I think in the case where they have more than one, any address should do. But I still don't really get the point of exposing the lnd listening uri through xud, so maybe I'm missing something. What do we expect the user to do with the lnd address(es) for peers? They are already connected so it's not like they can connect again, right?

[michael1011]

My personal, the Boltz ones and many routing nodes have two URIs: clearnet and Tor. So if we show the addresses, we should show all of them. Just in case.

But I don't recall the reason for showing them at all either. When you have the pubkey you can query the URIs from LND directly.

@rsercano sorry for all the back and forth. We should have agreed on that beforehand.

[rsercano]

My personal, the Boltz ones and many routing nodes have two URIs: clearnet and Tor. So if we show the addresses, we should show all of them. Just in case.

But I don't recall the reason for showing them at all either. When you have the pubkey you can query the URIs from LND directly.

@rsercano sorry for all the back and forth. We should have agreed on that beforehand.

Yes of course it would be better and easier for me to continue if we all agree on same thing, no worries

[sangaman]

michael and I both are of the opinion that only exposing the lnd node pubkey, without any uris, makes most sense, which is the current behavior. The nodekey can be used to query lnd for more information about a node if it is needed. @kilrau Did you have a specific use case in mind for exposing the lnd uri(s)? We would presumably already be connected to the peer on the lnd layer.

[ghost]

michael and I both are of the opinion that only exposing the lnd node pubkey, without any uris, makes most sense, which is the current behavior. The nodekey can be used to query lnd for more information about a node if it is needed. @kilrau Did you have a specific use case in mind for exposing the lnd uri(s)? We would presumably already be connected to the peer on the lnd layer.

Think we're currently using the lnd uri for openchannel command (to connect to lnd if not already connected). I'd be fine with switching it to nodekey if we can query the uris on lnd level.

[ghost]

Needs a rebase due to https://github.com/ExchangeUnion/xud/pull/2033 being merged.

[rsercano]

Needs a rebase due to #2033 being merged.

done @erkarl

[ghost]

Needs a rebase due to #2033 being merged.

done @erkarl

The build is failing after rebase.

[rsercano]

Have we implemented prettier already @erkarl 😅 Let me check

[rsercano]

Should be fixed now hopefully 😅 @erkarl

On the other hand, I just exposed connextIdentifier stated in the peer's info, and added LND uris additionally and exposed them too.

[rsercano]

@raladev sorry Roman I managed to overlook same issue twice but should be fixed now. Unfortunately I can't test connextIdentifier myself, my connext somehow keeps throwing 500 🤔

[kilrau]

The nodekey can be used to query lnd for more information about a node if it is needed. @kilrau Did you have a specific use case in mind for exposing the lnd uri(s)? We would presumably already be connected to the peer on the lnd layer.

Sorry that I missed answering here - yes it's for opening a channel to send funds from another node, which will be the setup for our upcoming xuc rewards.

Also, I think this is ready to be reviewed again. @sangaman

[kilrau]

Sry but we are requiring all commits to be signed @rsercano , could you pls force push a signed commit?

PS: git config --global commit.gpgsign true

[rsercano]

Hi @kilrau it took me some time to figure out but I already signed all my commits for this one, github was somehow unverified my email address, I re verified and now seems okay, can you please squash and merge instead of me? I literally suck at it since there're some in between commits, and I don't want to mess with them.

Let me know if you want me to create a new "clean" PR if squashing is not possible on your end. @sangaman @raladev

[ghost]

I literally suck at it since there're some in between commits, and I don't want to mess with them.

Something to try in the future:

git checkout master
// get latest from master
git pull origin master
// go back to your feature branch
git checkout feat/expose-lnd/connext-ids-listpeers
// rebase your branch against master
git rebase master
// go back X commits to rebase
git rebase -i HEAD~X
// choose the commits that you want to squash
// and edit the commit message
// push latest changes with force since the tree has divereged
git push origin feat/expose-lnd/connext-ids-listpeers -f

If you run into super complicated merge conflicts then you could also try to create a new branch based on master and git cherry-pick the commits that you do want to push.

[rsercano]

I literally suck at it since there're some in between commits, and I don't want to mess with them.

Something to try in the future:

git checkout master
// get latest from master
git pull origin master
// go back to your feature branch
git checkout feat/expose-lnd/connext-ids-listpeers
// rebase your branch against master
git rebase master
// go back X commits to rebase
git rebase -i HEAD~X
// choose the commits that you want to squash
// and edit the commit message
// push latest changes with force since the tree has divereged
git push origin feat/expose-lnd/connext-ids-listpeers -f

If you run into super complicated merge conflicts then you could also try to create a new branch based on master and git cherry-pick the commits that you do want to push.

I literally tried this a hundred times but resolving conflicts, not touching other commits, this's literally a mess for me 😢

[ghost]

It's fine we can squash and merge before merging once it is ready to merge.