Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support DNS hostnames in node announcements #2234

Merged
merged 16 commits into from Aug 16, 2022
Merged

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Apr 12, 2022

Add support for DNS host names

Notable changes:

  • you can now specify a DNS host name as one of your server.public-ips addresses (see PR #911). Note: you can not specify more than one DNS host name.
  • An IPv4, IPv6 or DNS hostname with port of 0 is invalid as per Bolt #7.
  • DNS host names will not be resolved until Client attempts to connect to a peer.
  • will not rebroadcast a NodeAnnouncement that contains more than one DNS host name
  • the codec does not implement punycode parsing; we should create an issue if we want to support non-ASCII character sets for DNS host names
  • DNS host name addresses must be resolved to determine if they are IPv4 or IPv6 when deciding whether or not to use a proxy.

To do:

See also PR #2202 - Use NodeAddress everywhere
and PR #2296 - drop Tor v2 support

@remyers
Copy link
Contributor Author

remyers commented Apr 29, 2022

Simple interop testing with CLN v0.10.2-252-gc0e3155 configured with:
./configure --enable-experimental-features --enable-developer

Overview of setup:

Alice [Eclair] @ dnstest1.co.fr <---> Bob [CLN] @ dnstest2.co.fr <---> Carol [Eclair] @ dnstest3.co.fr

Added to local /etc/hosts:

127.0.0.1       dnstest1.co.fr
127.0.1.1       dnstest2.co.fr
127.0.2.1       dnstest3.co.fr

Created a script dns_gossip.sh to test that DNS hostnames are gossiped between Eclair and CLN.

dns_gossip.sh

Launch bicoind with ./scripts/start-bitcoin.sh, run nodes: alice-eclair & bob-clightning & carol-eclair & and then run the test script ./scripts/dns_gossip.sh.

Example result:

$ ./scripts/dns_gossip.sh 
Alice is 02ca09b2d0fdab441e7e41019e021db360838478484ff191feaf0e49cc13e7a9ba
Bob is 02a821bf4d075dcb88104ab2784ee87ece8979c1e2492c50cac5cdbcaedf686cfc
Carol is 0384f3fca93a670a854f1b9732965b22920c02745f3d91e8084793bf0c7b69f5b5
Adding some Bitcoin to wallets...
Generating a few blocks to confirm wallet balances...
Opening channels between Alice and Bob...
Opening channels between Bob and Carol...
Generating a few blocks to confirm channels...
Creating invoices...
Awaiting gossip sync...
Gossip sync took 14 seconds
Paying invoices...
1. BOB -> CAROL [10,000]
complete
2. ALICE -> BOB [10,000]
payment-sent
3. CAROl -> BOB -> ALICE [5,000]
payment-sent
4. ALICE -> BOB -> CAROL [20,000]
payment-sent
5. BOB -> ALICE [10,000]
complete
6. CAROL -> BOB [5,000]
payment-sent
All invoices paid
"echo All invoices paid" command filed with exit code 0.

To re-run dns_gossip.sh you first must kill the three nodes, reset their databases with ./scripts/reset_all_nodes.sh and then start the nodes again.

// this actor listens to connection requests and creates connections
system.actorOf(ClientSpawner.props(nodeParams.keyPair, nodeParams.socksProxy_opt, nodeParams.peerConnectionConf, TestProbe().ref, TestProbe().ref))

val invalidDnsHostname_opt = NodeAddress.fromParts("eclair.invalid", 9735).toOption
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the .invalid top level domain will never resolve.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks good to me, I only have small cosmetic/architectural comments, good job!
This does need a rebase though.

@remyers remyers changed the title Support DNS hostnames and deprecate Torv2 addresses in announcements Support DNS hostnames in node announcements Jul 26, 2022
@remyers
Copy link
Contributor Author

remyers commented Aug 1, 2022

As per Roasbeef at today's meeting, LND 0.15 will propagate DNS hostnames, so nodes running a version from about a month ago should not disappear if they add dns host names to their node announcements.

@remyers remyers marked this pull request as ready for review August 12, 2022 10:10
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a couple of nits and a question for @pm47 about the sun.net dependency.
This needs a rebase, which should be trivial.

@codecov-commenter
Copy link

Codecov Report

Merging #2234 (c2df6d2) into master (33e6fac) will decrease coverage by 0.06%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master    #2234      +/-   ##
==========================================
- Coverage   84.95%   84.89%   -0.07%     
==========================================
  Files         198      198              
  Lines       15255    15277      +22     
  Branches      633      640       +7     
==========================================
+ Hits        12960    12969       +9     
- Misses       2295     2308      +13     
Impacted Files Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.11% <87.50%> (-0.17%) ⬇️
...ore/src/main/scala/fr/acinq/eclair/io/Client.scala 56.60% <100.00%> (+0.83%) ⬆️
...in/scala/fr/acinq/eclair/io/ReconnectionTask.scala 98.07% <100.00%> (ø)
...n/scala/fr/acinq/eclair/router/Announcements.scala 100.00% <100.00%> (ø)
...main/scala/fr/acinq/eclair/router/Validation.scala 94.11% <100.00%> (-1.24%) ⬇️
...n/scala/fr/acinq/eclair/tor/Socks5Connection.scala 9.37% <100.00%> (+0.71%) ⬆️
...a/fr/acinq/eclair/wire/protocol/CommonCodecs.scala 97.05% <100.00%> (+0.04%) ⬆️
...q/eclair/wire/protocol/LightningMessageTypes.scala 94.64% <100.00%> (+0.52%) ⬆️
...q/eclair/channel/publish/ReplaceableTxFunder.scala 86.91% <0.00%> (-3.67%) ⬇️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 95.93% <0.00%> (-1.63%) ⬇️
... and 5 more

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good job 💯

@t-bast t-bast merged commit bb6148e into ACINQ:master Aug 16, 2022
@remyers remyers deleted the dnshostnames branch August 16, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants