Fix some onion errors and assert their length is correct #1895

TheBlueMatt · 2022-12-01T20:34:52Z

When we're constructing an HTLCFailReason, we should check that we
set the data to at least the correct length for the given failure
code, which we do here.

First we move HTLCFailReason into onion_utils and encapsulate it a bit better, making channelmanager just a bit smaller. Then we can assert the length, but only after fixing a few issues.

Fixes #1879 but I'm frankly a bit confused - I don't see the 14 @valentinewallace claimed was being used incorrectly in phantom failures - maybe it was already fixed?

`impl_writeable_tlv_based_enum` shouldn't be assuming that `DecodeError` is in scope, which we address here.

valentinewallace · 2022-12-01T22:19:17Z

I think this was it, so | 13: https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L2301. That scope will also set chan_update_opt to None, would have to check if any of the expiry checks below are reachable for phantom payments

codecov-commenter · 2022-12-01T22:53:44Z

Codecov Report

Base: 90.69% // Head: 90.63% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (878a41f) compared to base (4dafa43).
Patch coverage: 92.69% of modified lines in pull request are covered.

❗ Current head 878a41f differs from pull request most recent head c9fe69f. Consider uploading reports for the commit c9fe69f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1895      +/-   ##
==========================================
- Coverage   90.69%   90.63%   -0.06%     
==========================================
  Files          91       91              
  Lines       48404    51171    +2767     
  Branches    48404    51171    +2767     
==========================================
+ Hits        43898    46380    +2482     
- Misses       4506     4791     +285

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`89.14% <ø> (+0.29%)`	⬆️
lightning/src/util/ser_macros.rs	`87.19% <0.00%> (ø)`
lightning/src/ln/onion_utils.rs	`93.56% <86.58%> (-1.37%)`	⬇️
lightning/src/ln/channelmanager.rs	`87.15% <97.43%> (+0.88%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`97.65% <100.00%> (+0.16%)`	⬆️
lightning/src/util/persist.rs	`94.44% <0.00%> (-0.80%)`	⬇️
lightning/src/util/events.rs	`24.56% <0.00%> (-0.44%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.45% <0.00%> (-0.28%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.96% <0.00%> (-0.12%)`	⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2022-12-01T23:42:37Z

Ah, thanks, fixed.

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-12-02T20:06:49Z

lightning/src/ln/channelmanager.rs

 use crate::ln::onion_utils;
+use crate::ln::onion_utils::HTLCFailReason;


nit:

Suggested change

use crate::ln::onion_utils;

use crate::ln::onion_utils::HTLCFailReason;

use crate::ln::onion_utils::{self, HTLCFailReason};

I find the original more readable 🤷

lightning/src/ln/onion_utils.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-12-02T20:23:39Z

lightning/src/ln/onion_utils.rs

+				// we get a fail_malformed_htlc from the first hop
+				// TODO: We'd like to generate a NetworkUpdate for temporary
+				// failures here, but that would be insufficient as find_route
+				// generally ignores its view of our own channels as we provide them via
+				// ChannelDetails.
+				// TODO: For non-temporary failures, we really should be closing the
+				// channel here as we apparently can't relay through them anyway.


Do these comments still make sense here, or should they stay in fail_backwards_internal?

The first TODO belongs here I think - it talks about building a NetworkUpdate which this fn is responsible for. The second we could debate but there's already a TODO that is equivalent at the callsite talking about if we blame our own channel.

valentinewallace · 2022-12-02T20:43:24Z

This test hits one of the new debug panics in HTLCFailReason::reason:

#[test]
fn test_phantom_failure_expires_too_soon() {
	// Test that we fail back phantoms if the upstream node fiddled with the CLTV too much with the
	// correct error code.
	let chanmon_cfgs = create_chanmon_cfgs(2);
	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

	let channel = create_announced_chan_between_nodes(&nodes, 0, 1, channelmanager::provided_init_features(), channelmanager::provided_init_features());

	// Get the route.
	let recv_value_msat = 10_000;
	let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat));
	let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel);

	// Route the HTLC through to the destination.
	nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
	check_added_monitors!(nodes[0], 1);
	let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
	let mut update_add = update_0.update_add_htlcs[0].clone();

	// Modify the route to have a too-low cltv.
	// update_add.cltv_expiry += CLTV_FAR_FAR_AWAY;

	connect_blocks(&nodes[1], 72);
	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
	commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);

	let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
	assert!(update_1.update_fail_htlcs.len() == 1);
	let fail_msg = update_1.update_fail_htlcs[0].clone();
	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
	commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);

	// Ensure the payment fails with the expected error.
	let mut fail_conditions = PaymentFailedConditions::new()
		.blamed_scid(phantom_scid)
		.expected_htlc_error_data(0x1000 | 14, &[]);
		expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
}

I think the other phantom 0x1000|14 error would hit the panic as well. I guess the lack of test coverage here is on me 😅

valentinewallace

Really nice cleanup, basically LGTM

lightning/src/ln/channelmanager.rs

lightning/src/ln/onion_route_tests.rs

lightning/src/ln/onion_utils.rs

valentinewallace · 2022-12-05T18:46:30Z

lightning/src/ln/onion_utils.rs

+				// ChannelDetails.
+				if let &HTLCSource::OutboundRoute { ref path, .. } = htlc_source {
+					(None, Some(path.first().unwrap().short_channel_id), true, Some(*failure_code), Some(data.clone()))
+				} else { unreachable!(); }


It'd remove some unreachables to make HTLCSource::OutboundRoute have an inner struct, but I think that'd be a future consideration

Yea, we should, agree it doesnt need to happen here.

lightning/src/ln/onion_utils.rs

valentinewallace

LGTM after squash

Now that it's entirely abstracted, there's no reason for `HTLCFailReason` to be in `channelmanager`, it's really an onion-level abstraction.

Now that `HTLCFailReason` is opaque and in `onion_utils`, we should encapsulate it so that `ChannelManager` can no longer directly access its inner fields.

This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.

The spec mandates that we copy the `sha256_hash_of_onion` field from the `UpdateFailMalformedHTLC` message into the error message we send back to the sender, however we simply ignored it. Here we copy it into the message correctly.

When we're constructing an HTLCFailReason, we should check that we set the data to at least the correct length for the given failure code, which we do here.

When we receive a phantom HTLC with a bogus/modified CLTV, we should fail back with `incorrect_cltv_expiry`, but that requires a `channel_update`, which we cannot generate for a phantom HTLC which has no corresponding channel. Thus, instead, we have to fall back to `incorrect_cltv_expiry`. Fixes lightningdevkit#1879

This ensures we always hit our new debug assertions while building failure packets in the immediately-fail pipeline while processing an inbound HTLC.

If we try to send any onion error with the `UPDATE` flag in response to a phantom receipt, we should always swap it for something generic that doesn't require a `channel_update` in it. Here we use `temporary_node_failure`. Test provided by Valentine Wallace <[email protected]>

TheBlueMatt · 2022-12-06T20:00:54Z

Squashed without further changes.

TheBlueMatt added 4 commits December 1, 2022 18:56

Encode HTLC failure packets in a util method on HTLCFailReason

9a2e26b

Decode HTLCFailReasons in a util method on the enum

6c984bf

Fix impl_writeable_tlv_based_enum to not require DecodeError

fe3cf29

`impl_writeable_tlv_based_enum` shouldn't be assuming that `DecodeError` is in scope, which we address here.

Construct from-message HTLCFailReason via a constructor method

4ba8338

TheBlueMatt added this to the 0.0.113 milestone Dec 1, 2022

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from 7e29b27 to db345a5 Compare December 1, 2022 21:54

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from db345a5 to 99c91bd Compare December 1, 2022 23:42

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from 99c91bd to 22d94a8 Compare December 2, 2022 01:23

valentinewallace reviewed Dec 2, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from 22d94a8 to af89d18 Compare December 2, 2022 20:42

valentinewallace reviewed Dec 5, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from a663ab2 to 35d8eb3 Compare December 5, 2022 19:13

wpaulino reviewed Dec 6, 2022

View reviewed changes

lightning/src/ln/onion_utils.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from 35d8eb3 to 878a41f Compare December 6, 2022 02:35

TheBlueMatt assigned wpaulino and valentinewallace Dec 6, 2022

valentinewallace reviewed Dec 6, 2022

View reviewed changes

TheBlueMatt added 8 commits December 6, 2022 20:00

Move HTLCFailReason to onion_utils

2485ef3

Now that it's entirely abstracted, there's no reason for `HTLCFailReason` to be in `channelmanager`, it's really an onion-level abstraction.

Encapsulate HTLCFailReason to not expose struct variants

4011db5

Now that `HTLCFailReason` is opaque and in `onion_utils`, we should encapsulate it so that `ChannelManager` can no longer directly access its inner fields.

Assert that all onion error messages are correct len in tests

8ec1480

When we're constructing an HTLCFailReason, we should check that we set the data to at least the correct length for the given failure code, which we do here.

Replace build_first_hop_failure_packet with HTLCFailReason

01d299e

This ensures we always hit our new debug assertions while building failure packets in the immediately-fail pipeline while processing an inbound HTLC.

TheBlueMatt force-pushed the 2022-12-fix-missing-data branch from 878a41f to c9fe69f Compare December 6, 2022 20:00

valentinewallace approved these changes Dec 6, 2022

View reviewed changes

wpaulino approved these changes Dec 6, 2022

View reviewed changes

TheBlueMatt merged commit 2390dbc into lightningdevkit:main Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some onion errors and assert their length is correct #1895

Fix some onion errors and assert their length is correct #1895

TheBlueMatt commented Dec 1, 2022

valentinewallace commented Dec 1, 2022

codecov-commenter commented Dec 1, 2022 •

edited

Loading

TheBlueMatt commented Dec 1, 2022

valentinewallace Dec 2, 2022

TheBlueMatt Dec 2, 2022

valentinewallace Dec 2, 2022

TheBlueMatt Dec 2, 2022

valentinewallace commented Dec 2, 2022

valentinewallace left a comment

valentinewallace Dec 5, 2022

TheBlueMatt Dec 5, 2022

valentinewallace left a comment

TheBlueMatt commented Dec 6, 2022

		use crate::ln::onion_utils;
		use crate::ln::onion_utils::HTLCFailReason;

	use crate::ln::onion_utils;
	use crate::ln::onion_utils::HTLCFailReason;
	use crate::ln::onion_utils::{self, HTLCFailReason};

Fix some onion errors and assert their length is correct #1895

Fix some onion errors and assert their length is correct #1895

Conversation

TheBlueMatt commented Dec 1, 2022

valentinewallace commented Dec 1, 2022

codecov-commenter commented Dec 1, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Dec 1, 2022

valentinewallace Dec 2, 2022

Choose a reason for hiding this comment

TheBlueMatt Dec 2, 2022

Choose a reason for hiding this comment

valentinewallace Dec 2, 2022

Choose a reason for hiding this comment

TheBlueMatt Dec 2, 2022

Choose a reason for hiding this comment

valentinewallace commented Dec 2, 2022

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace Dec 5, 2022

Choose a reason for hiding this comment

TheBlueMatt Dec 5, 2022

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Dec 6, 2022

codecov-commenter commented Dec 1, 2022 •

edited

Loading