[2/2] discovery+lnwire: add support for DNS host name in NodeAnnouncement msg using a new external-dns-address config field
#10159
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mohamedawnallah, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces the capability for LND nodes to announce their presence on the network using a DNS hostname, in addition to existing IP and Tor addresses. This is achieved by adding a new external-dns-address configuration field and a dedicated DNSAddress type. The changes ensure that node announcements adhere to network protocol specifications, allowing only a single DNS address per node. Furthermore, the pull request includes substantial improvements to the graph database's migration process, enhancing its efficiency and robustness through the implementation of batch validation for various data types.
Highlights
- DNS Hostname Support: Introduced a new
lnwire.DNSAddresstype and a correspondingexternal-dns-addressconfiguration option, allowing LND nodes to advertise themselves using a domain name inNodeAnnouncementmessages. - Node Announcement Validation: Implemented strict validation for
NodeAnnouncementmessages to ensure compliance with BOLT 07, specifically limiting nodes to advertising at most one DNS address. - Graph Database Integration: Updated the graph database schema and migration logic to properly store, retrieve, and manage the new DNS address type, ensuring data consistency and compatibility.
- Database Migration Optimizations: Significantly improved the performance and reliability of graph database migrations by refactoring them to use batch processing for validation steps, reducing individual database calls and enhancing overall efficiency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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>
There was a problem hiding this comment.
Code Review
This pull request introduces support for DNS hostnames in NodeAnnouncement messages and refactors the database migration process for better performance. The changes for DNS support are well-integrated across the configuration, wire protocol, and database layers. The migration refactoring to use batching is a significant performance improvement. I've found a couple of issues: one is a duplicated block of code in the configuration parsing, and another is a minor redundancy in the new DNS address validation logic. Overall, this is a solid pull request with valuable additions.
mohamedawnallah
force-pushed
the
addDNSField
branch
4 times, most recently
from
6eb1f37 to
5390853
Compare
Co-authored-by: Elle Mouton <elle.mouton@gmail.com>
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.
mohamedawnallah
force-pushed
the
addDNSField
branch
from
5390853 to
4559257
Compare
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.
cause we will need to be able to deserialise any persisted OpaqueAddrs which will have a Payload that we should be able to decode with this lnwire method to extract any new addresses that we now support. Co-authored-by: Elle Mouton <elle.mouton@gmail.com>
In this commit, we test the central `parseAddr` with existing net addresses before support parsing DNS address
mohamedawnallah
force-pushed
the
addDNSField
branch
from
4559257 to
c326857
Compare
|
@mohamedawnallah, remember to re-request review from reviewers when ready |
|
converting this to draft until the dependent PR is in |
mohamedawnallah
changed the title
NodeAnnouncement msg using a new external-dns-address config field
Change Description
With this PR, we expose the support for DNS in Node Announcement message through
external-dns-addressconfig field.Depends on #9455.
Closes #6337.
Closes #9126.
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.