feat: make lightning nodes p2p port reachable on LAN

[hsjoberg]

Closes #298

Description

This commit changes lightning nodes to accept p2p connections via LAN instead of only via the private network (172.x.x.x).

As with #296, this is also a breaking change that needs migration.

Steps to Test

1. Add a new network with a lnd node and a c-lightning node
2. Add a lnd and a c-lightning node from the Designer
3. Check the connectivity tab for each lightning node
4. Try to connect to the lightning nodes from within the LAN

Screenshots

(IP address for the P2P URI has changed from 172.x.x.x to 127.0.0.1)

[jamaljsr]

Hey @hsjoberg, thanks for this PR. After the last review I'm pretty confident that this all will work. I just took a quick look through the changes via GitHub and only had the one concern below. I will pull this branch locally and test it out later today.

[jamaljsr]

I haven't tested but I think having both the lnd and c-lightning ports start at 9735 will cause host port conflicts. This is why I made the REST ports 100 numbers apart.

[hsjoberg]

Very good point. They collide. I'll change to 100 apart.
I don't see why though, shouldn't the id+1 in createLndNode and createCLightningNode set them apart?

polar/src/utils/network.ts

Line 90 in 44b9fb4

const id = lightning.length ? Math.max(...lightning.map(n => n.id)) + 1 : 0;

[hsjoberg]

Fixed.

[jamaljsr]

I don't see why though, shouldn't the id+1 in createLndNode and createCLightningNode set them apart?

They will initially start out correct when the nodes are created. So if you had a network with 4 LN nodes [LND, c-lightning, LND, c-lightning], they will start out with the correct ports [9735, 9736, 9737, 9738]. But when you start the network and getOpenPorts is called, the port detection is run in groups of the implementations independently. So imagine if port 9737 is unavailable. For LND, getOpenPortRange will be called with [9735, 9737] and return [9735, 9738]. Then getOpenPortRange is called for c-lightning nodes with [9736, 9738] and will return [9736, 9738] since both of those ports are currently open. We now have a conflict on port 9738 when docker tries to start the containers. This likely has a very rare chance of occurring, but if we can easily avoid it by adding the gap of 100, why not?

[hsjoberg]

Ah right, makes sense! Thank you for the explanation.

[jamaljsr]

After these discussions, I am thinking it may be a good idea in the future to remove the + id when creating nodes. This isn't really doing anything meaningful other than avoiding a call to dockerService.saveComposeFile if all of the initially set ports are available on startup. I now see that it causes some confusion when reading the code. It should work fine by just setting these values to the BasePorts then let getOpenPorts reassign all the values when the node is started.

I am not suggesting that you should change anything in this PR, I'm just mentioning it as a thought I have.

[hsjoberg]

@jamaljsr Yes I have been kind of thinking in the same lines, the + id is kind of confusing, it's better to make it more clear that getOpenPorts is ultimately responsible for fixing any conflicts. Not sure how much calling dockerService.saveComposeFile costs, but perhaps it is worth it.
But yes this could be addressed at a later point.

[hsjoberg]

Btw, the old rpcUrl is still retrieved for the nodes, but not used in ConnectTab.tsx anymore.

polar/src/lib/lightning/types.ts

Line 14 in 5a53de5

rpcUrl: string;

[jamaljsr]

Btw, the old rpcUrl is still retrieved for the nodes, but not used in ConnectTab.tsx anymore.

I just had a thought to why we should display both P2P urls in the UI. The external url works for users that are connecting LN nodes outside of Polar to nodes inside. But I would assume most users are using Polar to develop an app that only uses the internal nodes.

If I connect my app to alice (via RPC) and I want to open a channel to bob from my app's UI, I need to use the internal P2P url 033...c6a@172.20.0.3:9735, not the external one 033...c6a@127.0.0.1:9737. Connecting from an internal node to 127.0.0.1 (localhost) will always fail.

I suggest displaying them both in the sidebar with the labels "P2P Internal" and "P2P External". You may need to decrease the number of characters in the url to prevent text wrapping.

[hsjoberg]

@jamaljsr Oh yeah that's reasonable!

I updated the PR. I think it worked well without decreasing the number of characters.

[jamaljsr]

I tested this branch locally and it works as advertised. The code looks good as well.

Thanks for the contribution