Adds DNS hostname to NetAddress #1553

wvanlint · 2022-06-21T06:26:59Z

Supports Bolt 7 DNS hostnames specified by lightning/bolts#911.

TheBlueMatt · 2022-06-21T16:26:07Z

lightning/src/util/ser.rs

+	type Error = ConversionError;
+
+	fn try_from(s: String) -> Result<Self, Self::Error> {
+		if s.is_ascii() && s.len() <= u8::MAX.into() {


Should also check that all characters are in the printable range (and maybe no spaces? probably no special chars? Dunno what's valid in DNS, we should check). With that in mind, maybe we should name the type something in reference to DNS?

It seems DNS permits any sequence of bytes but has a preferred format. I implemented the preferred format based on RFC 3696 and others linked there.

TheBlueMatt · 2022-06-21T16:26:32Z

lightning/src/ln/msgs.rs

@@ -442,6 +442,14 @@ pub enum NetAddress {
 		/// The port on which the node is listening
 		port: u16,
 	},
+	/// A DNS hostname/port on which the peer is listening.
+	DNSHostname {


Should we just call it hostname? DNS is kinda obvious? Am I missing something?

Adjusted that, I was mainly mirroring the BOLT PR.

TheBlueMatt · 2022-06-21T16:26:48Z

Nice, thanks! Looks like this is failing CI.

TheBlueMatt · 2022-06-23T15:44:50Z

lightning/src/util/ser.rs

+
+	fn try_from(s: String) -> Result<Self, Self::Error> {
+		// Regular expressions can't be used with no-std.
+		let labels: Vec<&str> = s.split(".").collect();


You should be able to avoid the collect here. In general, allocating a Vec via collect is not super trivial, and we should avoid it if at all possible, preferring to just keep and work with the generated iterator directly.

TheBlueMatt · 2022-06-23T15:46:00Z

lightning/src/util/errors.rs

@@ -78,6 +78,10 @@ impl fmt::Debug for APIError {
 	}
 }

+/// Indicates an error on conversions by core::convert traits.
+#[derive(Debug)]
+pub struct ConversionError;


Is this worth it? If its just a dummy type can we not just use () instead? It seems basically as clear either way?

I initially wanted to implement the Error trait for boxing purposes but I wasn't sure if it made sense to do this conditionally only when the standard library is used. Changed to () but let me know if you prefer a type with an Error trait instead.

codecov-commenter · 2022-06-24T06:20:29Z

Codecov Report

Merging #1553 (c30dcf1) into main (abf6564) will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #1553      +/-   ##
==========================================
+ Coverage   90.96%   91.00%   +0.03%     
==========================================
  Files          80       80              
  Lines       43610    44197     +587     
  Branches    43610    44197     +587     
==========================================
+ Hits        39670    40221     +551     
- Misses       3940     3976      +36

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	`86.49% <75.00%> (-0.32%)`	⬇️
lightning/src/util/ser.rs	`90.96% <82.60%> (-0.32%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.73% <100.00%> (+0.34%)`	⬆️
lightning/src/util/chacha20poly1305rfc.rs	`96.25% <0.00%> (-1.71%)`	⬇️
lightning/src/chain/onchaintx.rs	`93.98% <0.00%> (-0.93%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.91% <0.00%> (-0.33%)`	⬇️
lightning/src/routing/scoring.rs	`96.80% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.81% <0.00%> (-0.25%)`	⬇️
lightning-invoice/src/lib.rs	`87.39% <0.00%> (ø)`
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abf6564...c30dcf1. Read the comment docs.

TheBlueMatt · 2022-06-24T16:14:57Z

lightning/src/util/ser.rs

+			// Trailing period for fully-qualified name.
+			if bytes.len() == 0 {
+				if !last_label || label_idx == 0 {
+					break false;


Instead of breaking out of the loop with a validity flag, the structure here would be more readable if we just returned the error directly if something is invalid.

TheBlueMatt · 2022-06-24T16:15:24Z

lightning/src/util/ser.rs

+			let last_label;
+			if let None = labels.peek() {
+				last_label = true;
+			} else {
+				last_label = false;
+			}


Suggested change

let last_label;

if let None = labels.peek() {

last_label = true;

} else {

last_label = false;

}

let last_label = labels.peek().is_none();

TheBlueMatt · 2022-06-24T16:15:53Z

lightning/src/util/ser.rs

+		let valid_labels = loop {
+			let label;
+			match labels.next() {
+				Some(x) => label = x,
+				None => break true,
+			}


Suggested change

let valid_labels = loop {

let label;

match labels.next() {

Some(x) => label = x,

None => break true,

}

while let Some(label) = labels.next() {

TheBlueMatt · 2022-06-24T16:16:17Z

lightning/src/util/ser.rs

+
+	fn try_from(s: String) -> Result<Self, Self::Error> {
+		let mut labels = s.split('.').peekable();
+		let mut label_idx = 0;


Instead of tracking the idx explicitly, maybe just wrap labels with an .enumerate().

TheBlueMatt · 2022-06-24T16:17:18Z

lightning/src/util/ser.rs

+		}
+	}
+}
+impl TryFrom<&str> for Hostname {


This gives the (incorrect) impression that From<&str> is relatively efficient, but its not. Its maybe simpler to force the user to do the String::from() or .to_string() explicitly themselves to communicate what's really going on here.

That's true, removed this implementation.

TheBlueMatt · 2022-06-24T21:48:15Z

It looks like somehow your rebase ended up reverting several recent PRs.

TheBlueMatt · 2022-06-25T02:17:21Z

It looks like somehow your rebase ended up reverting several recent PRs.

Oops, sorry, it looks like the GitHub mobile app just...shows incorrect diffs sometimes?

This basically LGTM, lets get another reviewer on it.

TheBlueMatt

Basically LGTM.

TheBlueMatt · 2022-06-25T02:23:36Z

lightning/src/util/ser.rs

+#[derive(Clone, Debug, PartialEq)]
+pub struct Hostname(String);
+impl Hostname {
+	/// Returns the length of the hostname with an appropriate return type.


s/with an appropriate return type//?

tnull

Thank you for having a go at this! Generally looks good, only a few nits and questions.

lightning/src/util/ser.rs

lightning/src/ln/msgs.rs

lightning/src/util/ser.rs

dunxen · 2022-06-27T20:27:54Z

lightning/src/ln/msgs.rs

@@ -442,6 +442,13 @@ pub enum NetAddress {
 		/// The port on which the node is listening
 		port: u16,
 	},
+	/// A hostname/port on which the peer is listening.
+	Hostname {
+		/// The hostname on which the port is listening.


Think "port" should be "node" or "peer"?

Suggested change

/// The hostname on which the port is listening.

/// The hostname on which the node is listening.

tnull

Generally LGTM.

One note: it's easier for reviewers to follow the changes if you do not immediately squash them, but leave individual commits.

TheBlueMatt · 2022-07-02T01:03:56Z

Sorry for the delay, been a super busy week.

Generally, now that we have all the logic for it, and now that I think about it a bit more, do we really need to do this much verification of a hostname? Like, yea, the DNS won't resolve if the hostname is just one long string, but what happens if users start shoving non-hostname things in the hostname field (ie treating it as a second node alias, listing some policy info, etc) - should we outright drop their gossip, or simply fail to connect to the hostname and try the IP addresses instead? I do think we should ensure the strong is ASCII, is all in the printable range (ie no control chars) but do we need to care about label length?

wvanlint · 2022-07-02T20:23:04Z

@TheBlueMatt I agree, the concern of hostname validation can be left to DNS resolution and separated from the serialization concern here which was my original intent. I reverted back to the first approach with generic serialization prior to #1553 (comment).

Extending this rationale, I would also be open to relaxing validation further and allowing any UTF-8 string within the length constraint (although this is not included in the BOLT). Libraries are able to handle Punycode encoding e.g. "münchen.de:443".to_socket_addrs().unwrap(). I think the field could be misused no matter how strict the validation is.

Out of curiosity, are terminal escape sequence injections the main concern with control characters? I am unsure about their severity for modern terminals.

TheBlueMatt · 2022-07-03T17:05:33Z

Extending this rationale, I would also be open to relaxing validation further and allowing any UTF-8 string within the length constraint (although this is not included in the BOLT)

Hmm, there's two sides to this, though - we have to support reading hostnames in announcements and be kinda liberal there, but also want to be restrictive as to what users include as others may do strict validation. In theory we could split the two but I'm not sure it's really worth it - our users are developers, and can manage to just put a hostname.

Out of curiosity, are terminal escape sequence injections the main concern with control characters? I am unsure about their severity for modern terminals.

Yea, you never know, users of our devlopers' software will grep things and pass them to dig and...boom. With that in mind I'd suggest we also reject chars like $, ", ', backtick, etc. Probably easiest to just accept alphanumeric plus . and -. If its easy its best to be as conservative as possible but not bother doing strict hostname validation so much as just strict character set validation.

TheBlueMatt

Oooooooo-Kkkkkkkk. Thanks for sticking with me on this one, this LGTM as-is. Will let the other reviewers chime in, but feel free to squash down the fixups once they do.

tnull

LGTM. I just restarted the one stuck CI job.

wvanlint · 2022-07-04T17:26:52Z

Thanks for sticking with me on this one, this LGTM as-is. Will let the other reviewers chime in, but feel free to squash down the fixups once they do.

Thanks for the review as well! Squashed down the fixups.

Fixes a deserialization incompatibility introduced with lightningdevkit#1553.

wvanlint force-pushed the dns_hostname branch from 4864773 to 33f8be2 Compare June 21, 2022 06:59

TheBlueMatt reviewed Jun 21, 2022

View reviewed changes

wvanlint force-pushed the dns_hostname branch 4 times, most recently from 9518a45 to 8892a48 Compare June 23, 2022 07:21

tnull linked an issue Jun 23, 2022 that may be closed by this pull request

Support DNS hostnames in gossip #1313

Closed

TheBlueMatt reviewed Jun 23, 2022

View reviewed changes

wvanlint force-pushed the dns_hostname branch from 8892a48 to 4999593 Compare June 24, 2022 06:20

wvanlint marked this pull request as ready for review June 24, 2022 06:25

wvanlint requested a review from TheBlueMatt June 24, 2022 06:26

TheBlueMatt reviewed Jun 24, 2022

View reviewed changes

wvanlint force-pushed the dns_hostname branch from 4999593 to cf6b4aa Compare June 24, 2022 20:11

wvanlint requested a review from TheBlueMatt June 24, 2022 20:16

TheBlueMatt added the Seeking Code Review label Jun 25, 2022

TheBlueMatt previously approved these changes Jun 25, 2022

View reviewed changes

wvanlint dismissed TheBlueMatt’s stale review via e31eff2 June 25, 2022 21:13

wvanlint force-pushed the dns_hostname branch from cf6b4aa to e31eff2 Compare June 25, 2022 21:13

tnull self-assigned this Jun 27, 2022

tnull removed the Seeking Code Review label Jun 27, 2022

tnull removed their assignment Jun 27, 2022

tnull self-requested a review June 27, 2022 07:51

tnull reviewed Jun 27, 2022

View reviewed changes

dunxen reviewed Jun 27, 2022

View reviewed changes

wvanlint force-pushed the dns_hostname branch from e31eff2 to aca0375 Compare July 1, 2022 04:25

wvanlint force-pushed the dns_hostname branch from aca0375 to d6cc751 Compare July 1, 2022 04:53

wvanlint requested a review from tnull July 1, 2022 04:54

wvanlint force-pushed the dns_hostname branch from d6cc751 to ebfe709 Compare July 1, 2022 05:10

tnull reviewed Jul 1, 2022

View reviewed changes

wvanlint requested a review from TheBlueMatt July 1, 2022 17:47

wvanlint force-pushed the dns_hostname branch 2 times, most recently from fc3c757 to 2767a41 Compare July 3, 2022 21:58

TheBlueMatt reviewed Jul 3, 2022

View reviewed changes

wvanlint requested a review from tnull July 4, 2022 04:52

tnull previously approved these changes Jul 4, 2022

View reviewed changes

Adds DNS hostname to NetAddress

c30dcf1

wvanlint dismissed tnull’s stale review via c30dcf1 July 4, 2022 17:22

wvanlint force-pushed the dns_hostname branch from 2767a41 to c30dcf1 Compare July 4, 2022 17:22

TheBlueMatt mentioned this pull request Jul 4, 2022

Explore replacing BTreeSet in IndexedMap with a sorted Vec #1473

Closed

TheBlueMatt approved these changes Jul 4, 2022

View reviewed changes

tnull approved these changes Jul 5, 2022

View reviewed changes

TheBlueMatt merged commit daeb5a6 into lightningdevkit:main Jul 5, 2022

TheBlueMatt mentioned this pull request Jul 5, 2022

Make htlc_maximum_msat a required field. #1519

Merged

wvanlint deleted the dns_hostname branch July 7, 2022 03:59

tnull added a commit to tnull/rust-lightning that referenced this pull request Jul 25, 2022

Don't fail read NodeInfo for inv. NetAddress

8f4c951

Fixes a deserialization incompatibility introduced with lightningdevkit#1553.

m-schmoock mentioned this pull request Nov 30, 2022

Re-experimentalize DNS Bolt7 #911 ? ElementsProject/lightning#5756

Closed

	/// The hostname on which the port is listening.
	/// The hostname on which the node is listening.

Adds DNS hostname to NetAddress #1553

Adds DNS hostname to NetAddress #1553

Conversation

wvanlint commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 24, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 24, 2022

TheBlueMatt commented Jun 25, 2022 • edited

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 2, 2022

wvanlint commented Jul 2, 2022

TheBlueMatt commented Jul 3, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

wvanlint commented Jul 4, 2022

codecov-commenter commented Jun 24, 2022 •

edited

TheBlueMatt commented Jun 25, 2022 •

edited