Implement Flat Features #428

TheBlueMatt · 2019-12-23T22:58:51Z

This merges local and global features into one struct, which is
parameterized by where it appers. The parameterization restricts
which queries can be made and which features can be set, in line
with the latest BOLT 9.

Closes #427.

TheBlueMatt · 2019-12-24T18:53:29Z

Needs clarification on whether we can send to/through a node which has an even feature bit set on its node_announcement. See lightning/bolts#723.

lightning/src/ln/msgs.rs

ariard · 2020-01-04T20:43:31Z

lightning/src/ln/msgs.rs

+	mark: PhantomData<T>,
+}
+
+impl<T: FeatureContext> Clone for Features<T> {


nit: you can use a derive macro on the generic struct Features + concrete struct ones to avoid implementing clone(),eq(), debug()

For some reason the compiler barfs at it - I assume derive macros somehow don't work for templated types.

ariard · 2020-01-04T21:19:49Z

lightning/src/ln/msgs.rs

+				(self.features.flags[i] & ((1 << (14 - 8)) - 1)).write(w)?;
+			}
+		}
+		self.features.write(w)


Not sure if this respect the spec, encoding the whole logical features instead of two bitmaps ? Legacy global features bits are going to be encoded twice, which seems contrary to "SHOULD use the minimum length required to represent the features field."

Hmm. The way I read that is just that you SHOULDn't pad with 0 bytes, not that you should change the meaning of the data you send. I think the spec is pretty mum on the issue here - it doesn't say which bits you can set in globablfeatures, just that you cant send bits higher than 13. In practice, I think this should be fine - if there's some unknown bit the peer will do the right thing and ignore it if its odd or disconnect if its even.

ariard

Seems good to me, I still have a doubt on the right processing of required unknown bits channels by get_route but I don't think it's worse

ariard · 2020-01-05T22:10:49Z

lightning/src/ln/router.rs

-						if first_hops.is_none() || chan.two_to_one.src_node_id != network.our_node_id {
-							if chan.two_to_one.enabled {
-								add_entry!(chan_id, chan.one_to_two.src_node_id, chan.two_to_one, $fee_to_target_msat);
+					if !chan.features.requires_unknown_bits() {


Are you filtering out well first_hop and last_hop channel ? Not sure..

last_hops no cause we call add_entry!() directly. first hops also no but because Features::relevant_init_flags_to_channel currently doesnt convert anything that is unknown (which should maybe be changed, but at least in this case it doesn't matter too much because we certainly wont stay connected if their Init message requires unknown bits).

lightning/src/ln/msgs.rs

jkczyz · 2020-01-06T18:21:32Z

lightning/src/ln/msgs.rs

+pub struct FeatureContextInvoice {}
+
+/// An internal trait capturing the various future context types
+pub trait FeatureContext {}


This trait may not be needed given it has no methods and any methods on Features bounded by this trait could be implemented without the bound.

Gonna leave this one in as otherwise you can construct the Features objects with random types.

jkczyz · 2020-01-06T18:22:41Z

lightning/src/ln/msgs.rs

+	// Used to test encoding of diverse msgs
+	#[cfg(test)]
+	pub flags: Vec<u8>,
+	mark: PhantomData<T>,


Consider naming this context and removing Features trait bound on T as per an earlier comment.

jkczyz · 2020-01-06T18:23:59Z

lightning/src/ln/msgs.rs

+	pub(crate) fn new() -> Features<T> {
+		Features {
 			flags: vec![2 | 1 << 5],
+			mark: PhantomData,
 		}
 	}


The call sites of each new could be less verbose if these were module-level functions (e.g., new_init_features()). The return type could then be the exact specialization desired for the method.

The callsites are much nicer with the type aliases, so I think this is resolved.

lightning/src/ln/msgs.rs

TheBlueMatt · 2020-01-07T00:42:16Z

I think this is correct as of the proposed changes in RFC PR 723, which, after the meeting today, I don't think should have much of an issue, so I'm happy to merge with ACK(s).

lightning/src/ln/features.rs

jkczyz · 2020-01-07T00:51:29Z

lightning/src/ln/features.rs

+	impl FeatureContext for FeatureContextChannel {}
+	//impl FeatureContext for FeatureContextInvoice {}
+
+	pub trait DataLossProtect: FeatureContext {}


I guess the drawback of using the trait bound on Feature<T> is requiring the supertrait. I'm not overly concerned if you want to keep it as is though.

jkczyz · 2020-01-07T00:56:33Z

lightning/src/ln/features.rs

+		let len = cmp::min(2, self.flags.len());
+		w.size_hint(len + 2);
+		(len as u16).write(w)?;
+		for i in (0..len).rev() {
+			if i == 0 {
+				self.flags[i].write(w)?;
+			} else {
+				(self.flags[i] & ((1 << (14 - 8)) - 1)).write(w)?;


Could you make some constants for the magic numbers in this method? As an uninformed reader, I don't know what all of these mean. :)

I just left a comment instead cause its just 13 + 1 and a byte modifier, nothing that really makes sense to create a constant for.

lightning/src/ln/features.rs

TheBlueMatt · 2020-01-07T18:03:05Z

I think I fixed or responded to all your comments @jkczyz, though I was a bit lazy about stuffing the fixes into the last two commits instead of putting them in earlier.

lightning/src/ln/features.rs

jkczyz · 2020-01-07T22:02:37Z

lightning/src/ln/features.rs

+
+	/// Writes all features present up to, and including, 13.
+	pub(crate) fn write_up_to_13<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
+		let len = cmp::min(2, self.flags.len());


Consider replacing flags.len() with byte_count().

Hmm. At least personally I generally prefer direct access when in the struct anyway. It avoids "wait, does that local method really do nothing or does it mangle the value a bit" errors.

jkczyz · 2020-01-07T22:04:01Z

lightning/src/ln/features.rs

+impl InitFeatures {
+	/// Create a Features with the features we support
+	#[cfg(not(feature = "fuzztarget"))]
+	pub(crate) fn our_features() -> InitFeatures {


I have preference to name these default since "our" implies that the result should be used within a certain context, even though no such restriction exists. It would also make the call sites more readable if the word "features" is not repeated.

Hmm. default is strange IMO because the "our" refers to "rust-lightning's set of supported". Agree "our" is maybe less clear, do you have another idea for a better way to say "the set of features supported by rust-lightning, which should be advertised to our peers".

Oh, I thought you were using "our" for "our node" since elsewhere you are using "their" for peer features. :) Or maybe there is some overloading of the word.

I suggested "default" because features are similar to a config/options structs, and it's pretty typical to see code such as:

config.default() .set_option_foo() .set_option_bar();

as a way to set default options and then override some. Otherwise, maybe just call it new.

Hmm, given its just pub(crate), I dont think we need to worry too much about overriding some. new() is a bit too generic, too, cause its not clear that its intended specifically for the "this is what i want to send a peer" use-case. Maybe features_we_implement?

The suggestion of default was mainly to demonstrate this as a convention for specifying typical options. I see how new can be too generic especially given we have the more specific empty.

Using pronouns like we/our/my in identifiers is bit ambiguous for conveying information in my opinion. Don't get me going on identifiers like MyFoo. ;)

I would suggest either known or supported. That is, these features are either known or supported by the implementation. Using known also contrasts with uses of unknown_bits in the code. Optionally, adding either _bits or _features to the end may offer more clarity, though it would be inconsistent with empty unless also updated.

Gonna go with supported(), seems to capture what we want. Note that it is not guaranteed that all non-unknown fetures are supported().

lightning/src/ln/features.rs

jkczyz · 2020-01-07T22:56:31Z

lightning/src/ln/router.rs

+			assert_eq!(route.hops[0].pubkey, node8);
+			assert_eq!(route.hops[0].short_channel_id, 42);
+			assert_eq!(route.hops[0].fee_msat, 200);
+			assert_eq!(route.hops[0].cltv_expiry_delta, (13 << 8) | 1);
+
+			assert_eq!(route.hops[1].pubkey, node3);
+			assert_eq!(route.hops[1].short_channel_id, 13);
+			assert_eq!(route.hops[1].fee_msat, 100);
+			assert_eq!(route.hops[1].cltv_expiry_delta, 42);


Are all these assertions necessary for checking the route?

For the most part, yes. Not only does it give us full test coverage for each field in the RouteHop entry (which I think is useful), but in some cases, eg where we're overriding the route map with local channel or last-hop data, it is used to test that the data came from the correct source (the override) and ignored the original.

I agree that the override should be tested. However, tests should focus on testing individual behaviors. That is, testing the override behavior should not fail if, for example, the fee calculation behavior changes. This seems to be testing multiple behaviors.

I'm willing to concede, however, that the test as written may not be conducive for testing individual behaviors yet.

Right, sadly its super nontrivial to test exact behavior here since override somewhat by definition relies on fee calculation behavior. The fact that this could fail if we add fee randomization sucks, but removing chunks of it means much worse test coverage.

I think this test is a good candidate for refactoring into smaller unit tests but can be done in a different PR. I may be interested in tackling this once I read more on routing.

jkczyz · 2020-01-07T23:06:27Z

lightning/src/ln/router.rs

+		{ // Re-enable nodes 1, 2, and 8
+			let mut network = router.network_map.write().unwrap();
+			network.nodes.get_mut(&node1).unwrap().features.clear_require_unknown_bits();
+			network.nodes.get_mut(&node2).unwrap().features.clear_require_unknown_bits();
+			network.nodes.get_mut(&node8).unwrap().features.clear_require_unknown_bits();
+		}


route_test is tough to comprehend. It looks like in actuality it is many small tests with a common setup and possible dependencies on the preceding tests. Are there any plans to refactor this into smaller standalone tests? That would be more understandable and less fragile.

Hmm, right, maybe a better introductory comment would help. It is one big "create the route map" block followed by a bunch of "calculate routes through it and check them" blocks. We could split it up into a common fn create_routemap() -> Router {} function and a bunch of #[test]s that actually do the work, which is a pattern we use a bunch elsewhere, but lets do that in another PR.

ariard

ACK a682fb4

ariard · 2020-01-09T23:16:40Z

lightning/src/ln/router.rs

-						if first_hops.is_none() || chan.one_to_two.src_node_id != network.our_node_id {
-							if chan.one_to_two.enabled {
-								add_entry!(chan_id, chan.two_to_one.src_node_id, chan.one_to_two, $fee_to_target_msat);
+				if !$node.features.requires_unknown_bits() {


Sadly we're not going to route through a node which requires a local policy feature like data_loss_protect while this doesn't concern payment path at all. That's a spec wrongdoing

jkczyz

81ac36c should be split into two commits since it contains both a bug fix and an unrelated refactor. Also, the message needs to be updated with supported instead of our_features.

jkczyz · 2020-01-10T01:27:00Z

lightning/src/ln/router.rs

+			assert_eq!(route.hops[0].pubkey, node8);
+			assert_eq!(route.hops[0].short_channel_id, 42);
+			assert_eq!(route.hops[0].fee_msat, 200);
+			assert_eq!(route.hops[0].cltv_expiry_delta, (13 << 8) | 1);
+
+			assert_eq!(route.hops[1].pubkey, node3);
+			assert_eq!(route.hops[1].short_channel_id, 13);
+			assert_eq!(route.hops[1].fee_msat, 100);
+			assert_eq!(route.hops[1].cltv_expiry_delta, 42);


I agree that the override should be tested. However, tests should focus on testing individual behaviors. That is, testing the override behavior should not fail if, for example, the fee calculation behavior changes. This seems to be testing multiple behaviors.

I'm willing to concede, however, that the test as written may not be conducive for testing individual behaviors yet.

jkczyz · 2020-01-10T01:41:17Z

lightning/src/ln/router.rs

@@ -1441,6 +1439,98 @@ mod tests {
 			assert_eq!(route.hops[1].cltv_expiry_delta, 42);
 		}

+		{ // Disable 4 and 12 by requiring unknown feature bits


This would be clearer if it said "channels 4 and 12" (likewise on line 1479). Otherwise, it wasn't readily apparent how the following tests differed from the ones that disabled nodes 1, 2, and 8.

jkczyz · 2020-01-10T01:56:23Z

lightning/src/ln/router.rs

+			let mut network = router.network_map.write().unwrap();
+			network.nodes.get_mut(&node1).unwrap().features.set_require_unknown_bits();
+			network.nodes.get_mut(&node2).unwrap().features.set_require_unknown_bits();
+			network.nodes.get_mut(&node8).unwrap().features.set_require_unknown_bits();


More of a question on correct behavior: If you have a channel to node 8 as an override (as you do below) but node 8 is disabled in the network map (as done here), shouldn't get_route fail?

In this case the override is via "first_hops" aka "the list of channels with peers we currently have a tcp connection with, according to our ChannelManager". ISTM that such channels should definitely be taken as the first hop, if possible, especially given if they have unknown features, we probably shouldn't be able to make a connection to them in the first place....that said, the spec is currently a bit awkward around init context. See lightning/bolts#726 but tl;dr: unknown required bits currently both mean "cannot connect to" and "cannot route through", which is...confusing.

I see. Thanks for the explanation and link to discussion!

This merges local and global features into one struct, which is parameterized by where it appers. The parameterization restricts which queries can be made and which features can be set, in line with the latest BOLT 9. Closes lightningdevkit#427.

This change was made in the flat features BOLT PR, as if a channel requires some unknown feature bits we should still rumor it, we just shouldn't route through it.

The spec is a bit mum on feature endianness, so I suppose it falls under the "everything is big endian unless otherwise specified" clause, but we were treating it as little.

The Features::new() method is nonsense and doesn't describe what features were being set - we introduce an empty() and supported() constructors instead.

TheBlueMatt · 2020-01-13T18:53:54Z

Oops, forgot to push with the split commit. I think all your points were addressed.

jkczyz · 2020-01-13T22:44:46Z

ACK 49f88ec

TheBlueMatt force-pushed the 2019-12-flat-features branch 2 times, most recently from 99f5126 to 862b82a Compare December 24, 2019 17:21

TheBlueMatt added the Needs BOLT Clarification label Dec 24, 2019

TheBlueMatt force-pushed the 2019-12-flat-features branch 8 times, most recently from 241cfa9 to cde784d Compare December 29, 2019 21:38

TheBlueMatt mentioned this pull request Dec 29, 2019

Plumb Features through into Routes #433

Merged

ariard reviewed Jan 4, 2020

View reviewed changes

TheBlueMatt force-pushed the 2019-12-flat-features branch from cde784d to 925c73a Compare January 5, 2020 18:18

ariard approved these changes Jan 5, 2020

View reviewed changes

TheBlueMatt force-pushed the 2019-12-flat-features branch from 925c73a to 72c384b Compare January 6, 2020 04:45

jkczyz reviewed Jan 6, 2020

View reviewed changes

TheBlueMatt force-pushed the 2019-12-flat-features branch 3 times, most recently from fb28274 to bc7d64c Compare January 7, 2020 00:37

TheBlueMatt removed the Needs BOLT Clarification label Jan 7, 2020

jkczyz reviewed Jan 7, 2020

View reviewed changes

TheBlueMatt force-pushed the 2019-12-flat-features branch from bc7d64c to 6a0deb6 Compare January 7, 2020 18:02

TheBlueMatt force-pushed the 2019-12-flat-features branch from 6a0deb6 to 90421e5 Compare January 7, 2020 18:06

jkczyz reviewed Jan 7, 2020

View reviewed changes

lightning/src/ln/features.rs Outdated Show resolved Hide resolved

lightning/src/ln/features.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2019-12-flat-features branch from 90421e5 to b7d1ea1 Compare January 7, 2020 21:17

jkczyz reviewed Jan 7, 2020

View reviewed changes

TheBlueMatt force-pushed the 2019-12-flat-features branch 2 times, most recently from 7df6b15 to 8eaa47e Compare January 8, 2020 00:22

jkczyz self-assigned this Jan 8, 2020

TheBlueMatt force-pushed the 2019-12-flat-features branch from 8eaa47e to a682fb4 Compare January 9, 2020 21:56

ariard approved these changes Jan 9, 2020

View reviewed changes

jkczyz reviewed Jan 10, 2020

View reviewed changes

TheBlueMatt added 6 commits January 12, 2020 18:15

Implement Flat Features

31cc243

This merges local and global features into one struct, which is parameterized by where it appers. The parameterization restricts which queries can be made and which features can be set, in line with the latest BOLT 9. Closes lightningdevkit#427.

Relay/store channel/node announces w/ unknown req'd feature bits

0ad8fde

This change was made in the flat features BOLT PR, as if a channel requires some unknown feature bits we should still rumor it, we just shouldn't route through it.

Fix Feature endianness by swapping bytes on read/write.

96d3de5

The spec is a bit mum on feature endianness, so I suppose it falls under the "everything is big endian unless otherwise specified" clause, but we were treating it as little.

Refactor features a bit more to describe what the constructors do

7ec52c6

The Features::new() method is nonsense and doesn't describe what features were being set - we introduce an empty() and supported() constructors instead.

Move features into a separate module out of msgs.

cd5a11f

Seal the features contexts

49f88ec

TheBlueMatt force-pushed the 2019-12-flat-features branch from a682fb4 to 49f88ec Compare January 13, 2020 18:53

jkczyz approved these changes Jan 13, 2020

View reviewed changes

TheBlueMatt merged commit a252f81 into lightningdevkit:master Jan 13, 2020

mattfaltyn mentioned this pull request Jun 2, 2022

Document better optional features #1514

Merged

Implement Flat Features #428

Implement Flat Features #428

Conversation

TheBlueMatt commented Dec 23, 2019

TheBlueMatt commented Dec 24, 2019

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

ariard left a 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 Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 7, 2020

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

jkczyz Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jan 9, 2020 • edited Loading

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

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment • edited Loading

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 Jan 13, 2020

jkczyz commented Jan 13, 2020

TheBlueMatt commented Jan 7, 2020 •

edited

Loading

jkczyz Jan 7, 2020 •

edited

Loading

TheBlueMatt Jan 9, 2020 •

edited

Loading

jkczyz left a comment •

edited

Loading