Return error when failing onion packet construction

[tnull]

Based on #2277.

Previously, we could panic when failing to construct onion messages in certain circumstances. Here we opt to always rather error out and don't panic if something goes wrong during OM packet construction.

[tnull]

Draft mode for now as I'm a bit dubious why this pass_along_path currently fails. I could swear the same test worked before for 17 hops (before rebasing on current main). Also I'm not sure where the discrepancy comes from: while we construct and send the packet with 17 hops just fine, some node on the path receives:

TRACE node 1 [lightning::onion_message::messenger : lightning/src/onion_message/messenger.rs, 407] Forwarding an onion message to peer 02888f2e3df341d21240f769afd83938fdc1878356769aae64141880d810c61dce
TRACE node 2 [lightning::onion_message::messenger : lightning/src/onion_message/messenger.rs, 412] Errored decoding onion message packet: Malformed { err_msg: "HMAC Check failed", err_code: 49157 }

@valentinewallace Do you have any idea where the HMAC failure could come from and how we could catch it before sending?

[valentinewallace]

Looking into this, will get back this afternoon. I think there’s a bug in pass_along_path where we’ll never assert on the log at the moment :/ not sure if it’s related to the HMAC failure yet.

[valentinewallace]

@valentinewallace Do you have any idea where the HMAC failure could come from and how we could catch it before sending?

So this was a good catch, I don't think we've ever actually supported sending large OMs (i.e. with >1300-byte hop_data). Working on a fix :)

[valentinewallace]

Opened #2277

[valentinewallace]

Previously, we would panic when sending onion messages over paths with an excessive number of hops

I'm confused how we can panic, since in messenger.rs construct_onion_packet, we check that the payloads serialized length is under a certain amount before constructing the packet. Tested in onion_message::functional_tests::too_big_packet_error

[tnull]

I'm confused how we can panic, since in messenger.rs construct_onion_packet, we check that the payloads serialized length is under a certain amount before constructing the packet. Tested in onion_message::functional_tests::too_big_packet_error

Ah, we would hit a 'subtract with overflow' if the filler length is above the hardcoded 1300 bytes:

---- onion_message::functional_tests::many_hops stdout ----
---- onion_message::functional_tests::many_hops stdout ----
thread 'onion_message::functional_tests::many_hops' panicked at 'attempt to subtract with overflow', lightning/src/ln/onion_utils.rs:327:25
stack backtrace:
   0: rust_begin_unwind
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/panicking.rs:100:14
   2: core::panicking::panic
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/panicking.rs:50:5
   3: lightning::ln::onion_utils::construct_onion_packet_with_init_noise
             at ./src/ln/onion_utils.rs:327:16
   4: lightning::ln::onion_utils::construct_onion_message_packet
             at ./src/ln/onion_utils.rs:280:2
   5: lightning::onion_message::messenger::construct_onion_message_packet
             at ./src/onion_message/messenger.rs:570:5
   6: lightning::onion_message::messenger::OnionMessenger<ES,NS,L,CMH>::send_onion_message
             at ./src/onion_message/messenger.rs:251:30

As this panic is fixed in #2277, I'll rebase shortly.

[tnull]

Rebased on #2277.

[codecov-commenter]

Codecov Report

Patch coverage: 91.30% and project coverage change: +0.45 🎉

Comparison is base (71af4a2) 90.92% compared to head (de6649c) 91.37%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2271      +/-   ##
==========================================
+ Coverage   90.92%   91.37%   +0.45%     
==========================================
  Files         104      104              
  Lines       52757    57580    +4823     
  Branches    52757    57580    +4823     
==========================================
+ Hits        47970    52616    +4646     
- Misses       4787     4964     +177     

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	90.18% <50.00%> (+2.97%)	⬆️
lightning/src/ln/onion_utils.rs	90.54% <88.88%> (-0.30%)	⬇️
lightning/src/ln/functional_tests.rs	98.25% <100.00%> (ø)
lightning/src/ln/onion_route_tests.rs	98.26% <100.00%> (ø)
lightning/src/onion_message/messenger.rs	90.85% <100.00%> (ø)

... and 13 files with indirect coverage changes

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

[tnull]

Rebased on main after #2077 got merged.

[TheBlueMatt]

I agree we should return an error, but we should probably also drop route_size_insane and just either optimistically build the path or do that in-line, rather than having the two-step API which is now unnecessary.

[tnull]

I agree we should return an error, but we should probably also drop route_size_insane and just either optimistically build the path or do that in-line, rather than having the two-step API which is now unnecessary.

Alright, now went ahead and dropped route_size_insane.

[valentinewallace]

LGTM after squash

[tnull]

Squashed fixups and included the following changes:

index 27e91116..fb2ff3a7 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2730,10 +2730,6 @@ where
                let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, recipient_onion, cur_height, keysend_preimage)?;

-               let onion_packet = match onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash) {
-                       Ok(packet) => packet,
-                       Err(()) => {
-                               return Err(APIError::InvalidRoute { err: "Route size too large considering onion data".to_owned()});
-                       }
-               };
+               let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash)
+                       .map_err(|_| APIError::InvalidRoute { err: "Route size too large considering onion data".to_owned()})?;

                let err: Result<(), _> = loop {