chore: Announce coordinator node in local network #209

holzeis · 2023-03-06T13:29:37Z

This change sets all configurations to defaults that will work with our development setup.

p2p_address: changed to 0.0.0.0 so that the coordinator is reachable on localhost as well as local ip. This is needed as the phone communicates on localhost and the lnd node on the local ip.
skip_local_network_announcement: If this flag is set the local network ip announcement is skipped. This should be set in a productive environment. If not set it will add the local ip to the addresses announced for the coordinator. This is needed so that the lnd node is able to reach the coordinator from the docker network.

resolves #203

Note, I decided to go for Option 4, announcing the node on the local_ip, because it seemed to me the most practical solution.

Option 1: Too failure prune, as its pretty hard to know which network interface to use and it will be different on every platform.
Option 2: Binding to the host network feels odd and would require a different docker-compose for the github actions pipeline. We might still want that eventually, but probably for other reasons.
Option 3: Only works for mac and windows. DNS hostname is not supported by lnd yet.

coordinator/src/cli.rs

bonomat · 2023-03-06T21:38:47Z

crates/ln-dlc-node/src/node/mod.rs

+                    tracing::debug!("Announcing node on {:?}", announcements);
+                    let announcements = announcements.clone();
+                    peer_manager.broadcast_node_announcement([0; 3], alias, announcements);


I think this loop should actually be removed and we only announce once.

Well, I thought about it, but left it for the following reasons.

We use the same code for our mobile node and there it is useful to announce the nodes ip address in a loop. (E.g. the network changes)

In case we want to use a domain instead of an ip to announce our node, we could a) change the ip underneath without having to change the config on the coordinator and b) also work with dyndns.

We use the same code for our mobile node and there it is useful to announce the nodes ip address in a loop. (E.g. the network changes)

Do we need to announce the app node?

In case we want to use a domain instead of an ip to announce our node, we could a) change the ip underneath without having to change the config on the coordinator and b) also work with dyndns.

I thought LN does not work with domains?

Do we need to announce the app node?

It looks like it. see also

10101/crates/ln-dlc-node/src/node/mod.rs

Lines 380 to 382 in a045dc9

// TODO: Check why we need to announce the node of the mobile app as otherwise the

// just-in-time channel creation will fail with a `unable to decode own hop data`

// error.

But it shouldn't be required, I guess?

I thought LN does not work with domains?

Well LN Bolt-07 supports it

The following address descriptor types are defined: ... 5: DNS hostname; data = [1:hostname_len][hostname_len:hostname][2:port] (length up to 258) hostname bytes MUST be ASCII characters. Non-ASCII characters MUST be encoded using Punycode: https://en.wikipedia.org/wiki/Punycode

https://github.com/lightning/bolts/blob/master/07-routing-gossip.md

It's just not supported by LND yet - lightningnetwork/lnd#6337

However, we don't have to announce on the hostname, we could still do the announcement on ip and resolve the dns ourselves.

README.md

bonomat · 2023-03-07T00:11:22Z

coordinator/src/cli.rs

+    /// node announcements.
+    pub fn p2p_announcement_addresses(&self) -> Vec<NetAddress> {
+        let mut addresses: Vec<NetAddress> = vec![];
+        if !self.p2p_address.ip().is_unspecified() {


This works for a local setup but not for production where we are either behind a NAT or inside a docker container. For these cases we will need to pass in the announcement address.

yes, I thought so too.. so, lets add it when we need it. 😉

klochowicz

I don't mind this approach, however if you will get into trouble with testing in prod (as @bonomat outlined), then option 2) is likely our next best bet. Then we have just have local-dev specific docker-compose.yml, which is not too bad, especially when we're exposing all the ports anyway.

also: we can also remove the requirement to pass in coordinator host to the mobile app (the other README in /mobile directory)

README.md

holzeis · 2023-03-07T06:02:10Z

I don't mind this approach, however if you will get into trouble with testing in prod (as @bonomat outlined), then option 2) is likely our next best bet. Then we have just have local-dev specific docker-compose.yml, which is not too bad, especially when we're exposing all the ports anyway.

Maybe we shouldn't test in prod then 😅 . However, the whole idea of this PR was to make the defaults work for our local development. You can still change the default configuration to work for any other environment. I just did not add yet, the configuration to add additional announcement addresses.

also: we can also remove the requirement to pass in coordinator host to the mobile app (the other README in /mobile directory)

sorry, I don't find the README.md you are referring to.

klochowicz · 2023-03-07T23:11:10Z

also: we can also remove the requirement to pass in coordinator host to the mobile app (the other README in /mobile directory)

sorry, I don't find the README.md you are referring to.

scratch that, I think I had something on a wrong branch. sorry for making you look for something that wasn't there! 😅

luckysori · 2023-03-08T01:25:30Z

coordinator/src/cli.rs

+        } else {
+            // Announcing the node on an unspecified ip address does not make any sense.
+            tracing::warn!("Skipping node announcement on '0.0.0.0'.");
+        }


🔧 Why don't we just error and crash the application. It's probably pointless to run the app if it's misconfigured.

It does make sense to run the app on an unspecified interface (e.g. listen on any address, local host, local ip, public ip). However, this ip does not make sense to be used when announcing our node - as this address can not be used by a client. Here we need to specify the interface. (localhost, local ip or public ip).

Since I am implying the announcement node from the p2p-address, I wouldn't fail here, as this is a valid setup.

Okay, thanks for the explanation. I think that makes sense.

Could we maybe not print a warning then? This seems like an info or a debug log to me.

@luckysori fyi:
efbd04f

luckysori · 2023-03-08T01:30:07Z

coordinator/src/cli.rs

+        if !self.skip_local_network_announcement {
+            let local_ip = local_ip().expect("to get local ip address");
+            tracing::info!("Adding node announcement within local network {local_ip}. Do not use for production!");
+            addresses.push(build_net_address(local_ip, self.p2p_address.port()));


❓ Won't you end up pushing the same LOCAL_IP:port address twice if skip_local_network_announcement == false and the p2p_address is the default one?

No sure if I follow, the default address of p2p_address is 0.0.0.0. I guess if you set the p2p_address to your local_ip and do not skip_local_network_announcement you will end up pushing the same address twice.

Do suggest to add a handling for that? Looks to me more like a miss configuration.

I misread what we were doing above and forgot that it's pointless to announce on 0.0.0.0. I get it now! Sorry for the confusion.

bonomat · 2023-03-08T09:13:36Z

coordinator/src/cli.rs

@@ -17,6 +20,9 @@ pub struct Opts {
    /// Where to permanently store data, defaults to the current working directory.
    #[clap(long)]
    data_dir: Option<PathBuf>,
+
+    #[clap(long)]


If you add a doc comment here it will be printed when providing an invalid argument/input :)

This change sets all configurations to defaults that will work with our development setup. - p2p_address: changed to `0.0.0.0` so that the coordinator is reachable on localhost as well as local ip. This is needed as the phone communicates on localhost and the lnd node on the local ip. - skip_local_network_announcement: If this flag is set the local network ip announcement is skipped. This should be set in a productive environment. If not set it will add the local ip to the addresses announced for the coordinator. This is needed so that the lnd node is able to reach the coordinator from the docker network.

holzeis · 2023-03-08T09:44:43Z

bors r+

bors · 2023-03-08T09:51:16Z

Build succeeded:

luckysori

Approved after the fact.

luckysori · 2023-03-08T10:01:09Z

coordinator/src/cli.rs

+        } else {
+            // Announcing the node on an unspecified ip address does not make any sense.
+            tracing::warn!("Skipping node announcement on '0.0.0.0'.");
+        }


Okay, thanks for the explanation. I think that makes sense.

Could we maybe not print a warning then? This seems like an info or a debug log to me.

luckysori · 2023-03-08T10:11:04Z

coordinator/src/cli.rs

+        if !self.skip_local_network_announcement {
+            let local_ip = local_ip().expect("to get local ip address");
+            tracing::info!("Adding node announcement within local network {local_ip}. Do not use for production!");
+            addresses.push(build_net_address(local_ip, self.p2p_address.port()));


I misread what we were doing above and forgot that it's pointless to announce on 0.0.0.0. I get it now! Sorry for the confusion.

holzeis requested review from klochowicz and luckysori March 6, 2023 13:29

holzeis self-assigned this Mar 6, 2023

holzeis force-pushed the chore/default-startup-arguments branch 2 times, most recently from 94ad51a to b335e74 Compare March 6, 2023 14:49

bonomat reviewed Mar 6, 2023

View reviewed changes

coordinator/src/cli.rs Outdated Show resolved Hide resolved

bonomat reviewed Mar 6, 2023

View reviewed changes

README.md Show resolved Hide resolved

bonomat reviewed Mar 6, 2023

View reviewed changes

README.md Show resolved Hide resolved

bonomat reviewed Mar 7, 2023

View reviewed changes

klochowicz reviewed Mar 7, 2023

View reviewed changes

README.md Show resolved Hide resolved

holzeis force-pushed the chore/default-startup-arguments branch from b335e74 to 530d7e3 Compare March 7, 2023 13:24

luckysori reviewed Mar 8, 2023

View reviewed changes

bonomat approved these changes Mar 8, 2023

View reviewed changes

holzeis added 2 commits March 8, 2023 10:29

docs: Updated dev setup documentation

b7dad7e

holzeis force-pushed the chore/default-startup-arguments branch from 530d7e3 to b7dad7e Compare March 8, 2023 09:29

bors bot merged commit 3a65617 into main Mar 8, 2023

bors bot deleted the chore/default-startup-arguments branch March 8, 2023 09:51

luckysori reviewed Mar 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Announce coordinator node in local network #209

chore: Announce coordinator node in local network #209

holzeis commented Mar 6, 2023 •

edited

Loading

bonomat Mar 6, 2023

holzeis Mar 7, 2023

bonomat Mar 7, 2023

holzeis Mar 8, 2023

bonomat Mar 7, 2023 •

edited

Loading

holzeis Mar 7, 2023

klochowicz left a comment •

edited

Loading

holzeis commented Mar 7, 2023 •

edited

Loading

klochowicz commented Mar 7, 2023 •

edited

Loading

luckysori Mar 8, 2023

holzeis Mar 8, 2023

luckysori Mar 8, 2023

holzeis Mar 8, 2023

luckysori Mar 8, 2023

holzeis Mar 8, 2023

luckysori Mar 8, 2023

bonomat Mar 8, 2023

holzeis commented Mar 8, 2023

bors bot commented Mar 8, 2023

luckysori left a comment

luckysori Mar 8, 2023

luckysori Mar 8, 2023

	// TODO: Check why we need to announce the node of the mobile app as otherwise the
	// just-in-time channel creation will fail with a `unable to decode own hop data`
	// error.

chore: Announce coordinator node in local network #209

chore: Announce coordinator node in local network #209

Conversation

holzeis commented Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonomat Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz left a comment • edited Loading

Choose a reason for hiding this comment

holzeis commented Mar 7, 2023 • edited Loading

klochowicz commented Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis commented Mar 8, 2023

bors bot commented Mar 8, 2023

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis commented Mar 6, 2023 •

edited

Loading

bonomat Mar 7, 2023 •

edited

Loading

klochowicz left a comment •

edited

Loading

holzeis commented Mar 7, 2023 •

edited

Loading

klochowicz commented Mar 7, 2023 •

edited

Loading