Repository navigation
[1/2] discovery+lnwire: add support for DNS host name in NodeAnnouncement msg - #9455
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
3e93cdf to
cb36d26
Compare
fb721bc to
4a8adcb
Compare
c3a6a49 to
d67c686
Compare
3efa4b2 to
01c57c8
Compare
|
|
d48d1fd to
98d2f2f
Compare
|
Thank you, @ellemouton, for your feedback on this PR. I have restructured the commits and added unit tests for encoding/decoding the DNS hostname address, as well as for parsing it. Apologies for the significant delay between receiving your review and addressing it—I’ll do my best to minimize such gaps in the future, in line with the Any follow-up feedback would be very much appreciated! PS: |
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
Thanks for the PR :)
I think its a good idea to test/ensure the following:
- make sure the node can handle providing its own DNS addr
- make sure the node can handle receiving a DNS addr from other nodes
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
We have to be quite careful since we always need to think about what our currently persisted data may look like. It is quite involved - so to help, I've put together this draft/rough PR to show you the various changes I think are needed: ellemouton#219
Let me know if that helps & if you have any questions.
perhaps it might be a good idea to split the saga up into 2: part 1 just doing what is covered in that example I shared (ie, just being able to handle network DNS addrs) & then a follow up that adds the new option to add our own DNS addrs.
|
|
||
| // Validate validates that the DNS hostname is not empty and contains only ASCII | ||
| // characters and of max length 255 characters according to BOLT specifications. | ||
| func (d *DNSAddr) Validate() error { |
There was a problem hiding this comment.
unresolving as still not addressed
|
|
||
| // Validate validates that the DNS hostname is not empty and contains only ASCII | ||
| // characters and of max length 255 characters according to BOLT specifications. | ||
| func (d *DNSAddr) Validate() error { |
There was a problem hiding this comment.
what i meant in the comment was that there should not even be an exported Validate method on the type.
rather have a helper like:
func validateDNS(hostname, port) error {
...
}
that you call from within the constructor. Ie, it should not be possible to construct the type without validating it.
| "multiple DNS addresses. See " + | ||
| "Bolt 07") | ||
| } | ||
| dnsAddrIncluded = true |
There was a problem hiding this comment.
ok so i thought about this a bit more and I now agree with YY that this is not the place for the check of "dont add more than 1 DNS addr".
It is a tricky decision since the spec does say that the receiver "should ignore the rest of the data if more than one such addr exists".
BUT what we actually care about here is just being able to parse the wire message: ie, can we properly read the address_descriptor. Then the actual content of the fields can be checked elsewhere such as in netann (see ValidateChannelUpdateFields as an example).
| case *DNSAddr: | ||
| if dnsAddrIncluded { |
There was a problem hiding this comment.
so yeah, just confirming from above: i think let's let the lnwire package just do serialisation & deserialisation. The actual content of the fields & validity of the fields can be checked in netann
There was a problem hiding this comment.
. The actual content of the fields & validity of the fields can be checked in netann
Have validated the DNS fields according to bolt-07 on the netann layer applying this comment. I have made the validate DNS fields utility in locality with the actual DNS schema. If I have other place better to put this utility, I am open to it
commented
Aug 7, 2025
|
ok just spoke to @saubyk and he had a good idea: perhaps we are overcomplicating this & we can potentially get away with not unraveling anything we have already persisted on disk (ie, Opaque Addrs remain Opaque addrs even if they contain DNS addrs). I think this might be an ok solution given that nodes will occasionally send out new announcements & so things should eventually be correct. Im going to start a convo offline just to first see what others think. In the mean time, defs take a look at the draft code i linked before to get an understanding of what that path looks like. |
moawnallah
changed the title
3f1f50b to
a9675a6
Compare
commented
Aug 14, 2025
Thanks @ellemouton for providing this draft/rough PR. It was so helpful moving this PR forward. Will invite for a look after CI tests pass
I will follow-up with a PR regards the new option 👍 |
a9675a6 to
3f15701
Compare
11c8ebc to
6b38f8d
Compare
6b38f8d to
b4a8b24
Compare
8977287 to
035fac4
Compare
commented
Aug 29, 2025
|
@mohamedawnallah, remember to re-request review from reviewers when ready |
| } | ||
|
|
||
| if len(hostname) > 255 { | ||
| return fmt.Errorf("DNS hostname length %d, exceeds limit of "+ |
There was a problem hiding this comment.
nit: think we should define errors above and return fmt.Errorf("%w: ...") here, so it's easier to be tested below, error string matching is a bit fragile.
There was a problem hiding this comment.
nit: think we should define errors above and return
fmt.Errorf("%w: ...")here, so it's easier to be tested below, error string matching is a bit fragile.
Addressed
There was a problem hiding this comment.
cool we should return fmt.Errorf("%w: DNS hostname length %d", ...) - given the goal is to do error matching in the tests, we should also change it to require.ErrorIs instead of error string matching.
There was a problem hiding this comment.
cool we should return
fmt.Errorf("%w: DNS hostname length %d", ...)- given the goal is to do error matching in the tests, we should also change it torequire.ErrorIsinstead of error string matching.
Used wrapped errors with additional details. It seems eventually we need to match against error string i.e that part ("%w: DNS hostname length %d") since require.ErrorIs only checks if the error types match, not the exact error message content
In this commit, we remove `AddrLen` as prepration step before adding DNS address type which will have a var length. Co-authored-by: Elle Mouton <elle.mouton@gmail.com>
Co-authored-by: Elle Mouton <elle.mouton@gmail.com>
| } | ||
|
|
||
| if len(hostname) > 255 { | ||
| return fmt.Errorf("DNS hostname length %d, exceeds limit of "+ |
There was a problem hiding this comment.
cool we should return fmt.Errorf("%w: DNS hostname length %d", ...) - given the goal is to do error matching in the tests, we should also change it to require.ErrorIs instead of error string matching.
Check that the node ann doesnt contain more than 1 DNS addr. This will ensure that we now start rejecting new node announcements with multiple DNS addrs since this check is called in the gossiper before persisting a node ann to our local graph. It also validates the DNS fields according to BOLT #7 specs.
We may have already persisted node announcements that have multiple DNS addresses since we may have received them before updating our code to check for this. So here we just make sure not to send these on to our peers.
The first byte of an opaque addr must be one that we dont understand yet. We do this update in preparation for doing an on-the-fly parse of persisted opaque addrs to see if they contain addrs that we now support. For this to work, the first byte cant be 0x01 since this maps to a known address.
7 participants
Change Description
Towards #6337.
Towards #9126.
Next #10159.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.