Fix some onion errors and assert their length is correct by TheBlueMatt · Pull Request #1895 · lightningdevkit/rust-lightning

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.

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

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


nit:
Suggested change
usecrate::ln::onion_utils;
usecrate::ln::onion_utils::HTLCFailReason;
usecrate::ln::onion_utils::{self,HTLCFailReason};

I find the original more readable 🤷

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

+ // 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

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

+ // 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.

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`. Fixeslightningdevkit#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 <vwallace@protonmail.com>

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

Squashed without further changes.

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