Default to BOLT 4 tlv payload format onions #1414

ViktorTigerstrom · 2022-04-09T17:27:29Z

Closes #1393.

Default to creating BOLT 4 tlv payload format onions for nodes for which we haven't received any features through node announcements, and where no other features are known such as invoice features nor features in the init msg for nodes we
have channel(s) to.

If there is a more convenient way in our testing utils to create a network of nodes, where the nodes to not announce their features through NodeAnnouncements, instead of the test config function I created I'll of course change to that :)!

Happy to address any feedback!

ViktorTigerstrom · 2022-04-09T17:32:01Z

lightning/src/routing/router.rs

@@ -1132,7 +1134,7 @@ where L::Target: Logger {
 				let features = if let Some(node_info) = $node.announcement_info.as_ref() {
 					&node_info.features
 				} else {
-					&empty_node_features
+					&variable_length_onion_optional_features


I just want to clarify that I couldn't manage to create a test scenario where changing this specific line created a different result in terms of which payload format we create. Though I think it makes sense to change this line.

codecov-commenter · 2022-04-09T17:55:09Z

Codecov Report

Merging #1414 (697bda8) into main (711bcef) will increase coverage by 0.02%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
+ Coverage   90.82%   90.84%   +0.02%     
==========================================
  Files          73       74       +1     
  Lines       41266    41375     +109     
  Branches    41266    41375     +109     
==========================================
+ Hits        37480    37589     +109     
  Misses       3786     3786

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`95.54% <ø> (ø)`
lightning/src/routing/router.rs	`92.58% <94.73%> (+0.08%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`97.45% <96.20%> (-0.17%)`	⬇️
lightning/src/routing/network_graph.rs	`89.88% <100.00%> (+0.27%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.66% <0.00%> (-0.29%)`	⬇️
lightning-invoice/src/utils.rs	`96.53% <0.00%> (-0.16%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (-0.02%)`	⬇️
lightning/src/ln/mod.rs	`95.00% <0.00%> (ø)`
lightning/src/ln/inbound_payment.rs	`93.49% <0.00%> (ø)`
... and 2 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 711bcef...697bda8. Read the comment docs.

ViktorTigerstrom · 2022-04-10T13:53:50Z

Cleaned up some documentation.

TheBlueMatt

I don't believe this modifies the default for last-hop hints which do not appear in the network graph at all. This is the most common use for this. See the NodeFeatures::empty() call on L1317 in router.rs

ViktorTigerstrom · 2022-04-11T12:27:00Z

I don't believe this modifies the default for last-hop hints which do not appear in the network graph at all. This is the most common use for this. See the NodeFeatures::empty() call on L1317 in router.rs

Ah thanks, I'll update with converge of that case as soon as I can!

vincenzopalazzo · 2022-04-11T23:19:13Z

lightning/src/routing/router.rs

+	let mut variable_length_onion_optional_features = NodeFeatures::empty();
+	variable_length_onion_optional_features.set_variable_length_onion_optional();


Suggested change

let mut variable_length_onion_optional_features = NodeFeatures::empty();

variable_length_onion_optional_features.set_variable_length_onion_optional();

let mut onion_optional_features = NodeFeatures::empty();

onion_optional_features.set_variable_length_onion_optional();

I'm a super fan of the explicative variable name, but I think that here the variable is too long? Mabey onion_optional_features is better?

@vincenzopalazzo True, thanks a lot for the feedback! I opted for onion_format_optional_features in the latest version I pushed, to find some middle ground without losing too much meaning. But I'll change to onion_optional_features if you think that's still too long :)

vincenzopalazzo · 2022-04-11T23:19:24Z

lightning/src/routing/router.rs

@@ -1132,7 +1134,7 @@ where L::Target: Logger {
 				let features = if let Some(node_info) = $node.announcement_info.as_ref() {
 					&node_info.features
 				} else {
-					&empty_node_features
+					&variable_length_onion_optional_features


Suggested change

&variable_length_onion_optional_features

&onion_optional_features

vincenzopalazzo · 2022-04-11T23:19:38Z

lightning/src/routing/router.rs

@@ -1330,7 +1332,7 @@ where L::Target: Logger {
 							if let Some(node_info) = node.announcement_info.as_ref() {
 								ordered_hops.last_mut().unwrap().1 = node_info.features.clone();
 							} else {
-								ordered_hops.last_mut().unwrap().1 = NodeFeatures::empty();
+								ordered_hops.last_mut().unwrap().1 = variable_length_onion_optional_features.clone();


Suggested change

ordered_hops.last_mut().unwrap().1 = variable_length_onion_optional_features.clone();

ordered_hops.last_mut().unwrap().1 = onion_optional_features.clone();

TheBlueMatt

Basically lgtm, need to review the tests still, can you squash the fixup commits?

ViktorTigerstrom · 2022-04-12T18:57:23Z

Fixed some documentation and updated the commit message. Sorry about that.

jkczyz

Largely look good but still need to review the tests while you address the comments.

jkczyz · 2022-04-12T19:16:25Z

lightning/src/routing/router.rs

@@ -2958,7 +2964,7 @@ mod tests {
 		assert_eq!(route.paths[0][3].short_channel_id, last_hops[0].0[1].short_channel_id);
 		assert_eq!(route.paths[0][3].fee_msat, 100);
 		assert_eq!(route.paths[0][3].cltv_expiry_delta, 42);
-		assert_eq!(route.paths[0][3].node_features.le_flags(), &Vec::<u8>::new()); // We dont pass flags in from invoices yet
+		assert_eq!(route.paths[0][3].node_features.le_flags(), &vec![0, 2]); // We dont pass flags in from invoices yet


Use default_onion_format_flags?

jkczyz · 2022-04-12T19:20:32Z

lightning/src/routing/router.rs

+	let mut onion_format_optional_features = NodeFeatures::empty();
+	onion_format_optional_features.set_variable_length_onion_optional();


I'd say use a method for this so it can be reused in the tests and name the variable default_node_features. No need to have the name talk about the feature itself. FWIW, I think we lose some meaning by dropping the "variable length" part of it, but we can avoid that by just using a generic name.

jkczyz · 2022-04-12T19:25:26Z

lightning/src/routing/router.rs

+	fn default_onion_format_flags() -> Vec<u8> {
+		vec![0, 2]
+	}


Could drop this in favor of using the previously suggested method and calling le_flags on the result.

ViktorTigerstrom · 2022-04-12T21:21:55Z

Thanks for the feedback @jkczyz! Addressed it in the latest commit.

I've also realized that I should maybe expand my test to include routing through an unannounced node that isn't the target node, to cover that case as well. I'll add that if you think that it's of value to cover that case in the tests.

ViktorTigerstrom · 2022-04-12T21:24:42Z

lightning/src/routing/router.rs

+///
+/// Default features are:
+/// * variable_length_onion_optional
+fn default_node_features() -> NodeFeatures {


Decided to also name this function to default_node_features, so that it possibly can be extended with more features in the future. I also considered if it would make more sense to add this as part of the NodeFeatures type itself, but figured that it probably belongs to the router context.

I like this refactoring :)

TheBlueMatt

Would be nice to use some of our macros to simplify the tests (I commented with two specific ones, but similar changes apply in test_do_not_use_default_to_tlv_onions_if_other_features_not_supporting_them_exists as well. Otherwise LGTM.

TheBlueMatt · 2022-04-13T14:26:06Z

lightning/src/ln/onion_route_tests.rs

+	let scorer = test_utils::TestScorer::with_penalty(0);
+	let seed = [0u8; 32];
+	let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet);
+	let random_seed_bytes = keys_manager.get_secure_random_bytes();
+	let read_only_network_graph = &network_graph.read_only();
+	let announced_route = get_route(
+		&origin_node.node.get_our_node_id(), &payment_params, read_only_network_graph,
+		Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
+		1000000, TEST_FINAL_CLTV, origin_node.logger, &scorer, &random_seed_bytes).unwrap();


Suggested change

let scorer = test_utils::TestScorer::with_penalty(0);

let seed = [0u8; 32];

let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet);

let random_seed_bytes = keys_manager.get_secure_random_bytes();

let read_only_network_graph = &network_graph.read_only();

let announced_route = get_route(

&origin_node.node.get_our_node_id(), &payment_params, read_only_network_graph,

Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()),

1000000, TEST_FINAL_CLTV, origin_node.logger, &scorer, &random_seed_bytes).unwrap();

let (announced_route, _, _, _) = get_route_and_payment_hash!(origin_node, nodes[3], 1_000_000);

TheBlueMatt · 2022-04-13T14:30:46Z

lightning/src/ln/onion_route_tests.rs

+	let unannounced_chan_params = PaymentParameters::from_node_id(nodes[4].node.get_our_node_id()).with_route_hints(vec![last_hop]);
+	let unannounced_route = get_route(
+		&origin_node.node.get_our_node_id(), &unannounced_chan_params, read_only_network_graph,
+		Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
+		10000, TEST_FINAL_CLTV, origin_node.logger, &scorer, &random_seed_bytes).unwrap();


Suggested change

let unannounced_chan_params = PaymentParameters::from_node_id(nodes[4].node.get_our_node_id()).with_route_hints(vec![last_hop]);

let unannounced_route = get_route(

&origin_node.node.get_our_node_id(), &unannounced_chan_params, read_only_network_graph,

Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()),

10000, TEST_FINAL_CLTV, origin_node.logger, &scorer, &random_seed_bytes).unwrap();

let (unannounced_route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[4], last_hop, 10_000, TEST_FINAL_CLTV);

ViktorTigerstrom · 2022-04-13T23:31:34Z

Would be nice to use some of our macros to simplify the tests (I commented with two specific ones, but similar changes apply in test_do_not_use_default_to_tlv_onions_if_other_features_not_supporting_them_exists as well. Otherwise LGTM.

Hmmm the current version of the get_route_and_payment_hash macro specifies that the PaymentParameters should be constructed using InvoiceFeatures::known(), which will then be used when creating the route for the last hop.
So I had to modify the get_route_and_payment_hash macro in order to be able to use it. However I'm not really sure if my modifications simplified the tests or just complicated things more since the macro became more complicated to understand?

I therefore pushed the changes in a fixup commit, which I'll squash or drop depending on if you think the modifications are ok.

TheBlueMatt · 2022-04-14T15:50:05Z

Oops, right, good point. I do still prefer the fixup change here - less code duplication for stuff that we end up changing kinda often means way less painful changes later on.

jkczyz · 2022-04-14T17:52:59Z

lightning/src/ln/functional_test_utils.rs

 		let payment_params = $crate::routing::router::PaymentParameters::from_node_id($recv_node.node.get_our_node_id())
 			.with_features($crate::ln::features::InvoiceFeatures::known())
 			.with_route_hints($last_hops);
+		$crate::get_route_and_payment_hash!($send_node, $recv_node, payment_params, $recv_value, $cltv, true)
+	}};
+	($send_node: expr, $recv_node: expr, $payment_params: expr, $recv_value: expr, $cltv: expr, true /* with specified payment_params */) => {{


Would it be possible to just have this pattern (without the last true) and have uses of the previous pattern pass in the full PaymentParameters? I think there's only a few places in priv_short_conf_tests.rs.

Ok sure, I'll fix that ASAP!

Default to creating tlv onions for nodes for which we haven't received any features through node announcements or which aren't in the `network_graph`, and where no other features are known such as invoice features nor features in the init msg for nodes we have channels to.

ViktorTigerstrom · 2022-04-14T21:11:25Z

Addressed the latest feedback and squashed the commits!

Note that I pushed the change of the get_route_and_payment_hash macro in a separate commit now as it isn't really related to the defaulting feature I've implemented, and I therefore wanted to avoid mixing the macro change with the feature's tests. Though I can of course squash them if you think that it's more appropriate!

jkczyz

Looks good. Just some nits on the tests.

jkczyz · 2022-04-15T15:37:46Z

lightning/src/ln/onion_route_tests.rs

+	// onions, as `nodes[0]` has no `NodeAnnouncementInfo` `features` for `node[3]`, and no `InvoiceFeatures`
+	// for the `payment_params`, which would otherwise have been used.
+	assert!(hops[2].node_features.supports_variable_length_onion());
+	// Note that we do not asset that `hops[0]` (the channel between `nodes[0]` and `nodes[1]`)


nit: s/asset/assert

jkczyz · 2022-04-15T15:38:55Z

lightning/src/ln/onion_route_tests.rs

+	// for the `payment_params`, which would otherwise have been used.
+	assert!(hops[2].node_features.supports_variable_length_onion());
+	// Note that we do not asset that `hops[0]` (the channel between `nodes[0]` and `nodes[1]`)
+	// supports variable length onions, as the `InitFeatures` exchanged in the init message when


remove "when"

jkczyz · 2022-04-15T15:41:39Z

lightning/src/routing/network_graph.rs

+	/// purposes.
+	#[cfg(test)]
+	pub fn clear_nodes_announcement_info(&self) {
+		for node in self.nodes.write().unwrap().iter_mut(){


nit: add space before brace

jkczyz · 2022-04-15T16:21:05Z

lightning/src/ln/onion_route_tests.rs

+	// Tests that we do not default to creating tlv onions if any of these features exists when
+	// creating the specific hop in the route:
+	// 1. `InitFeatures` to the counterparty node exchanged with the init message to the node, which
+	// doesn't support variable length onions.
+	// 2. `NodeFeatures` in the `NodeAnnouncementInfo` of a node in sender node's the
+	// `network_graph`, which doesn't support variable length onions.
+	// 3. `InvoiceFeatures` specified by the receiving node, which doesn't support variable length
+	// onions, when no `NodeAnnouncementInfo` `features` exists for the receiver in the sender's
+	// `network_graph`.


The language could be simplified here by moving the "doesn't support variable length onions" part out of each item and said once preceding the list.

jkczyz · 2022-04-15T16:21:35Z

lightning/src/ln/onion_route_tests.rs

+	// creating the specific hop in the route:
+	// 1. `InitFeatures` to the counterparty node exchanged with the init message to the node, which
+	// doesn't support variable length onions.
+	// 2. `NodeFeatures` in the `NodeAnnouncementInfo` of a node in sender node's the


drop trailing "the"

jkczyz · 2022-04-15T16:29:57Z

lightning/src/ln/onion_route_tests.rs

+}
+
+#[test]
+fn test_do_not_use_default_to_tlv_onions_if_other_features_not_supporting_them_exists() {


To name somewhat similar to the previous test, how about: test_do_not_default_to_onion_payload_tlv_format_when_unsupported

vincenzopalazzo

LGTM

ViktorTigerstrom · 2022-04-15T23:38:59Z

Thanks for the feedback @jkczyz! Addressed and squashed it with the last commit :)

ViktorTigerstrom commented Apr 9, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-04-default-to-tlv-onions branch from 89b9a42 to 8a59b6b Compare April 10, 2022 13:52

TheBlueMatt reviewed Apr 10, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Apr 10, 2022

Drop support for creating BOLT 4 Legacy onion format payloads #1413

Merged

vincenzopalazzo suggested changes Apr 11, 2022

View reviewed changes

jkczyz self-requested a review April 12, 2022 17:42

TheBlueMatt reviewed Apr 12, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-04-default-to-tlv-onions branch 2 times, most recently from b713196 to 18557ce Compare April 12, 2022 18:55

jkczyz reviewed Apr 12, 2022

View reviewed changes

ViktorTigerstrom commented Apr 12, 2022

View reviewed changes

vincenzopalazzo previously approved these changes Apr 13, 2022

View reviewed changes

TheBlueMatt reviewed Apr 13, 2022

View reviewed changes

ViktorTigerstrom dismissed vincenzopalazzo’s stale review via 697bda8 April 13, 2022 23:25

jkczyz reviewed Apr 14, 2022

View reviewed changes

ViktorTigerstrom added 2 commits April 14, 2022 23:04

Pass PaymentParameters in get_route_and_payment_hash

b28bcfe

ViktorTigerstrom force-pushed the 2022-04-default-to-tlv-onions branch from 697bda8 to a0c4d1b Compare April 14, 2022 21:05

jkczyz reviewed Apr 15, 2022

View reviewed changes

vincenzopalazzo previously approved these changes Apr 15, 2022

View reviewed changes

Add tests for defaulting to creating tlv onions

c72ae67

ViktorTigerstrom dismissed vincenzopalazzo’s stale review via c72ae67 April 15, 2022 23:36

ViktorTigerstrom force-pushed the 2022-04-default-to-tlv-onions branch from a0c4d1b to c72ae67 Compare April 15, 2022 23:36

TheBlueMatt approved these changes Apr 16, 2022

View reviewed changes

TheBlueMatt assigned jkczyz Apr 16, 2022

jkczyz approved these changes Apr 18, 2022

View reviewed changes

jkczyz merged commit e0b9b74 into lightningdevkit:main Apr 18, 2022

		let mut variable_length_onion_optional_features = NodeFeatures::empty();
		variable_length_onion_optional_features.set_variable_length_onion_optional();

	&variable_length_onion_optional_features
	&onion_optional_features

	ordered_hops.last_mut().unwrap().1 = variable_length_onion_optional_features.clone();
	ordered_hops.last_mut().unwrap().1 = onion_optional_features.clone();

		let mut onion_format_optional_features = NodeFeatures::empty();
		onion_format_optional_features.set_variable_length_onion_optional();

Default to BOLT 4 tlv payload format onions #1414

Default to BOLT 4 tlv payload format onions #1414

Conversation

ViktorTigerstrom commented Apr 9, 2022 • edited

Choose a reason for hiding this comment

codecov-commenter commented Apr 9, 2022 • edited

Codecov Report

ViktorTigerstrom commented Apr 10, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Apr 11, 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 left a comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Apr 12, 2022

jkczyz 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

ViktorTigerstrom commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ViktorTigerstrom commented Apr 13, 2022 • edited

TheBlueMatt commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Apr 14, 2022 • edited

jkczyz 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

vincenzopalazzo left a comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Apr 15, 2022

ViktorTigerstrom commented Apr 9, 2022 •

edited

codecov-commenter commented Apr 9, 2022 •

edited

ViktorTigerstrom commented Apr 13, 2022 •

edited

ViktorTigerstrom commented Apr 14, 2022 •

edited