Skip to content

Add DNSHostname to NetAddress mssgs #1329

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

Closed
wants to merge 1 commit into from

Conversation

a-mpch
Copy link

@a-mpch a-mpch commented Feb 23, 2022

Support Bolt 7 DNS Hostnames Check lightning/bolts#911

I'm missing the test in this one if someone could guide me would be nice

Fixes #1313

@a-mpch a-mpch marked this pull request as draft February 23, 2022 01:27
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks for taking a crack at this!

@@ -434,6 +434,14 @@ pub enum NetAddress {
/// The port on which the node is listening
port: u16,
},
DNSHostname {
/// The hostname length
hostname_len: u8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need an explicit length if its implicit in the Vec below :).

/// The hostname length
hostname_len: u8,
/// The dns hostname on which the node is listening, length hostname len
hostname: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably just make this a String, no?

@@ -454,11 +463,12 @@ impl NetAddress {
&NetAddress::IPv6 { .. } => { 18 },
&NetAddress::OnionV2(_) => { 12 },
&NetAddress::OnionV3 { .. } => { 37 },
&NetAddress::DNSHostname { .. } => { 258 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should return the real serialized length, using the length of the hostname itself.

@a-mpch a-mpch force-pushed the feat/addDNSHostname branch from 73b3a7a to b104ae5 Compare March 22, 2022 00:17
@a-mpch
Copy link
Author

a-mpch commented Mar 22, 2022

@TheBlueMatt sorry the delay, I changed the comments.
Kinda learning rust with these. I hope looks better :)

@@ -434,6 +435,12 @@ pub enum NetAddress {
/// The port on which the node is listening
port: u16,
},
DNSHostname {
/// The dns hostname on which the node is listening, length hostname len
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/, length hostname len //?

@@ -485,6 +496,11 @@ impl Writeable for NetAddress {
version.write(writer)?;
port.write(writer)?;
}
&NetAddress::DNSHostname { ref hostname, ref port } => {
5u8.write(writer)?;
hostname.write(writer)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default String write writes a two-byte length, whereas we need one here. You'll need to manually write the string. That said, you may actually want to use a different type, given we're required to write out hostnames in ASCII, and the tight size length, we'll want to enforce that on users.

5 => {
Readable::read(reader)?;
Ok(Ok(NetAddress::DNSHostname {
hostname: Readable::read(reader)?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should enforce the ASCII-ness of the string here.

@TheBlueMatt
Copy link
Collaborator

Do you intend to revisit this @mauricepoirrier?

@a-mpch
Copy link
Author

a-mpch commented Apr 21, 2022

Do you intend to revisit this @mauricepoirrier?

Yes @TheBlueMatt. I was thinking on this weekend.

@TheBlueMatt
Copy link
Collaborator

Do you still intend to revisit this @mauricepoirrier?

@a-mpch
Copy link
Author

a-mpch commented May 25, 2022

@TheBlueMatt sorry, I haven't found the time. Should I close this?

@TheBlueMatt
Copy link
Collaborator

Up to you, you're welcome to leave it open if you think you'll get back to it, if you don't think you'll find the time feel free to close.

@a-mpch a-mpch closed this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support DNS hostnames in gossip
2 participants