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.

Sign up for GitHub

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

Jump to bottom

bolt7: dns support #4829

Merged
merged 9 commits into from Nov 29, 2021
Merged

bolt7: dns support #4829

merged 9 commits into from Nov 29, 2021

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Sep 29, 2021 •
edited

This implements RFC lightning/bolts#911 to allow annoucements of DNS hostnames.
The feature can only be used with EXPERIMENTAL.
The bold quotes are disabled in the topmost commit to please CI.

Adds:

  • related fixes and changes (i.e. announce up to two addresses of the same type)
  • feature itself
  • and tests

@m-schmoock m-schmoock force-pushed the bolt7/dns branch 7 times, most recently from 30e9c00 to 38d9ea0 Compare October 7, 2021 11:01
@m-schmoock m-schmoock force-pushed the bolt7/dns branch 4 times, most recently from 1193fae to 6e1c37a Compare October 8, 2021 13:39
m-schmoock
m-schmoock commented Oct 8, 2021
View reviewed changes
connectd/connectd.c Outdated Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the bolt7/dns branch 2 times, most recently from 63b35de to b7d098f Compare October 9, 2021 08:40
@m-schmoock m-schmoock marked this pull request as ready for review October 9, 2021 08:41
@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Oct 9, 2021 •
edited

I think this is ready for review now.
Still not 100% happy with the way iffy way I changed opt_add_addr_withtype in lightningd/options.c, maybe you see a better way.

@m-schmoock m-schmoock force-pushed the bolt7/dns branch 3 times, most recently from 964179d to d2ccfef Compare October 9, 2021 09:03
@cdecker
Copy link
Member

cdecker commented Oct 9, 2021

Note: the BOLT checker complains about my not yet merged changes on BOLT7. Not sure how we solve this chicken and egg when implementing upcoming protocol changes.

* `5`: DNS hostname; data = `[byte:len][len*byte:hostname][u16:port]` (length up to 258)

Yeah, there is a special syntax that specifies a separate branch of the lightning-rfc repo to check against, though I'm not remembering how that worked rn

@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Oct 9, 2021 •
edited

Thx for the tip, looks like we can set the BOLTVERSION in the Makefile for that.
Update: I cant create branches in the RFC repo and had other issues with it so I went back to removing the BOLT quotes for now just to get past CI.

@m-schmoock m-schmoock force-pushed the bolt7/dns branch 6 times, most recently from bd48566 to 0b68251 Compare October 13, 2021 14:21
rustyrussell
rustyrussell requested changes Nov 17, 2021
View reviewed changes
connectd/connectd.c Show resolved Hide resolved
connectd/connectd.c Outdated Show resolved Hide resolved
connectd/connectd.c Show resolved Hide resolved
connectd/connectd.c Outdated Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the bolt7/dns branch 3 times, most recently from 9598e74 to 1edb314 Compare November 17, 2021 10:13
@m-schmoock
Copy link
Collaborator Author

Currently CI fails on wallet/test/run-db.c:209 which is very likely not related to this PR. I can even reproduce this on my local machine. So I guess there is some condition in master branch that causes it...

@ZmnSCPxj
Copy link
Collaborator

Currently CI fails on wallet/test/run-db.c:209 which is very likely not related to this PR.

Kicked CI, just in case this is just test flakiness.

@m-schmoock
Copy link
Collaborator Author

Kicked CI, just in case this is just test flakiness.

@ZmnSCPxj The problem remains in wallet/test/run-db.c:209

@m-schmoock m-schmoock force-pushed the bolt7/dns branch 2 times, most recently from 195cbc6 to 1c83cfd Compare November 23, 2021 14:50
@m-schmoock @rustyrussell
options: fix ld proposed_wireaddr proposed_listen_announce get out of…
cb2a3fa 
… sync

Incase the error handling happening after the quted line is non-critical:
```
    return tal_fmt(NULL, "Unable to parse address '%s': %s", arg, err_msg);
```

we should not expand the proposed_listen_announce array without adding
a proposed_wireaddr. So we move the expand of proposed_listen_announce
to the location where we also expand the proposed_wireaddr.

Changelog-None
@m-schmoock @rustyrussell
options: make --disable-dns an early arg
185f28c
@m-schmoock @rustyrussell
chore: gitignore gdb_history
4bd6573
@m-schmoock @rustyrussell
options: fix respect --disable-dns when parsing wireaddress
dbfee2b 
Changelog-Fixed: Options: Respect --always-use-proxy AND --disable-dns when parsing wireaddresses to listen on.
@m-schmoock @rustyrussell
wireaddr: adds helper is_ipaddr, is_toraddr and is_dnsaddr
f99aa1d
@m-schmoock @rustyrussell
bolt7: allow announcement of ADDR_TYPE_DNS
55825cb
@m-schmoock @rustyrussell
connectd: resolve ADDR_TYPE_DNS
5cfcc6c 
This will resolve ADDR_TYPE_DNS wireaddr by expanding connect->addrs with one
new wireaddr ADDR_INTERNAL_WIREADDR per DNS result and calling recursion
@m-schmoock @rustyrussell
pytest: test connecting to a DNS only announced node
6bc3f57
@m-schmoock @rustyrussell
chore: use EXPERIMENTAL for BOLT7 DNS ElementsProject#911
930d25b 
Changelog-EXPERIMENTAL: Ability to announce DNS addresses
@rustyrussell
Copy link
Contributor

Ack 930d25b

Rebased onto master, fixed trivial conflicts.

@rustyrussell rustyrussell merged commit a3ea9fd into ElementsProject:master Nov 29, 2021
@m-schmoock m-schmoock mentioned this pull request Nov 30, 2021
Merged
@m-schmoock m-schmoock deleted the bolt7/dns branch November 30, 2021 17:50
@m-schmoock
Copy link
Collaborator Author

Seems like rust-lightning is going to add support for this as the first other implementation: lightningdevkit/rust-lightning#1329

@remyers remyers mentioned this pull request Apr 12, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZmnSCPxj ZmnSCPxj ZmnSCPxj left review comments

@rustyrussell rustyrussell rustyrussell approved these changes

@cdecker cdecker Awaiting requested review from cdecker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@m-schmoock @cdecker @ZmnSCPxj @rustyrussell