Add DNSHostname to NetAddress mssgs#1329
Conversation
TheBlueMatt
left a comment
There was a problem hiding this comment.
Thanks for taking a crack at this!
lightning/src/ln/msgs.rs
Outdated
| }, | ||
| 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
| &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.
73b3a7a to
b104ae5
Compare
|
@TheBlueMatt sorry the delay, I changed the comments. |
| 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 //?
| } | ||
| &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