Add DNSHostname to NetAddress mssgs #1329
Conversation
lightning/src/ln/msgs.rs
Outdated
| @@ -434,6 +434,14 @@ pub enum NetAddress { | |||
| /// The port on which the node is listening | |||
| port: u16, | |||
| }, | |||
| DNSHostname { | |||
| /// The hostname length | |||
| hostname_len: u8, | |||
There was a problem hiding this comment.
You don't need an explicit length if its implicit in the Vec below :).
lightning/src/ln/msgs.rs
Outdated
| /// The hostname length | ||
| hostname_len: u8, | ||
| /// The dns hostname on which the node is listening, length hostname len | ||
| hostname: Vec<u8>, |
There was a problem hiding this comment.
Probably just make this a String, no?
lightning/src/ln/msgs.rs
Outdated
| @@ -454,11 +463,12 @@ impl NetAddress { | |||
| &NetAddress::IPv6 { .. } => { 18 }, | |||
| &NetAddress::OnionV2(_) => { 12 }, | |||
| &NetAddress::OnionV3 { .. } => { 37 }, | |||
| &NetAddress::DNSHostname { .. } => { 258 } | |||
There was a problem hiding this comment.
This should return the real serialized length, using the length of the hostname itself.
mauricepoirrier
force-pushed
the
feat/addDNSHostname
branch
from
73b3a7a
to
b104ae5
Compare
|
@TheBlueMatt sorry the delay, I changed the comments. |
| @@ -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 | |||
There was a problem hiding this comment.
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)?; | |||
There was a problem hiding this comment.
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)?, |
There was a problem hiding this comment.
We should enforce the ASCII-ness of the string here.
|
Do you intend to revisit this @mauricepoirrier? |
Yes @TheBlueMatt. I was thinking on this weekend. |
|
Do you still intend to revisit this @mauricepoirrier? |
|
@TheBlueMatt sorry, I haven't found the time. Should I close this? |
|
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. |
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