Support receiving to multi-hop blinded paths #2688

valentinewallace · 2023-10-26T18:02:08Z

Title. Currently, we only support receiving to 1-hop blinded paths. Diff is largely error handling.

Add test coverage for malformed HTLC ser

~~Based on #2540.~~

codecov-commenter · 2023-10-26T18:11:28Z

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (ec8e0fe) 88.53% compared to head (6b66271) 89.31%.
Report is 35 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	87.56%	13 Missing and 10 partials ⚠️
lightning/src/ln/channel.rs	90.00%	7 Missing and 4 partials ⚠️
lightning/src/ln/blinded_payment_tests.rs	98.31%	3 Missing and 1 partial ⚠️
lightning/src/ln/msgs.rs	90.00%	1 Missing and 1 partial ⚠️
lightning/src/ln/onion_payment.rs	97.77%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2688      +/-   ##
==========================================
+ Coverage   88.53%   89.31%   +0.77%     
==========================================
  Files         115      115              
  Lines       91011    96503    +5492     
  Branches    91011    96503    +5492     
==========================================
+ Hits        80580    86194    +5614     
+ Misses       8004     7947      -57     
+ Partials     2427     2362      -65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2023-12-01T04:33:10Z

Still ironing out some improvements to the code post-rebase of #2540, will get an update out soon.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-12-06T21:02:05Z

Addressed feedback. Planning to add some test coverage for the new malformed HTLC ser changes, added a TODO.

Will be used to read encrypted_tlvs on non-intro-node onion receipt.

Support for receiving to multi-hop blinded payment paths will be completed in the next commit, sans error handling.

The only remaining step is to use the update_add blinding point in decoding inbound onion payloads. Error handling will be completed in upcoming commits.

For use in supporting receiving to multi-hop blinded paths.

valentinewallace · 2023-12-08T21:03:22Z

Rebased.

lightning/src/ln/onion_payment.rs

lightning/src/ln/channel.rs

jkczyz · 2023-12-09T20:44:39Z

lightning/src/ln/channel.rs

+				&HTLCUpdateAwaitingACK::FailMalformedHTLC {
+					htlc_id, failure_code, sha256_of_onion
+				} => {
+					// We don't want to break downgrading by adding a new variant, so write a dummy
+					// `::FailHTLC` variant and write the real malformed error as an optional TLV.
+					malformed_htlcs.push((htlc_id, failure_code, sha256_of_onion));
+
+					let dummy_err_packet = msgs::OnionErrorPacket { data: Vec::new() };
+					2u8.write(writer)?;
 					htlc_id.write(writer)?;
+					dummy_err_packet.write(writer)?;
 				}


When downgrading, would these essentially be left in the holding cell permanently? Or are they dropped as a no-op when freeing the holding cell?

No, downgraded nodes will use this dummy htlc data to fail this htlc backwards. This prevents them from dropping the malformed HTLCs (which are written as optional TLVs) on read post-downgrade, which would cause them to not fail backwards and eventually be FC'd on.

lightning/src/ln/channel.rs

jkczyz · 2023-12-09T21:22:57Z

lightning/src/ln/channel.rs

+/// A wire message that fails an HTLC backwards sometime after the update_add_htlc's corresponding
+/// RAA has been processed, as opposed to failing on initial update_add receipt. Useful for
+/// [`Channel::fail_htlc`] to fail with either [`msgs::UpdateFailMalformedHTLC`] or
+/// [`msgs::UpdateFailHTLC`] as needed.
+trait PostRAAFailHTLCMessage {
+	type FailureReason: Clone;
+	fn new(htlc_id: u64, channel_id: ChannelId, reason: Self::FailureReason) -> Self;
+	fn message_name() -> &'static str;
+	fn inbound_htlc_state(reason: Self::FailureReason) -> InboundHTLCState;
+	fn htlc_update_awaiting_ack(htlc_id: u64, reason: Self::FailureReason) -> HTLCUpdateAwaitingACK;
+}


Given that the err_packet parameter to fail_htlc is what is being generalized, it seems more natural to have those types implement the trait. That way most of the methods would be associated functions and have a &self parameter rather than having them all be named parameters (i.e., reason as phrased here).

Also, since you could parameterize other functions with the error type, it means you don't need to specify a type parameter at the call site. Later this avoid duplicating queue_fail_htlc as you'd be able to generalize its parameter instead.

jkczyz · 2023-12-09T21:42:22Z

lightning/src/ln/channel.rs

+	/// Used for failing back with [`msgs::UpdateFailMalformedHTLC`]. For now, this is used when we
+	/// want to fail blinded HTLCs where we are not the intro node.
+	///
+	/// See [`Self::queue_fail_htlc`] for more info.
+	pub fn queue_fail_malformed_htlc<L: Deref>(
+		&mut self, htlc_id_arg: u64, failure_code: u16, sha256_of_onion: [u8; 32], logger: &L
+	) -> Result<(), ChannelError> where L::Target: Logger {
+		self.fail_htlc::<L, msgs::UpdateFailMalformedHTLC>(
+			htlc_id_arg, (failure_code, sha256_of_onion), true, logger
+		).map(|msg_opt| assert!(msg_opt.is_none(), "We forced holding cell?"))
+	}
+


You shouldn't need to duplicate this. See earlier comment on the aded trait.

Yeah, although see #2688 (comment).

Re-duplicated after offline discussion since it's more readable in ChannelManager this way and helps avoid increasing the visibility of some internal channel structs.

jkczyz · 2023-12-09T21:44:36Z

lightning/src/ln/channelmanager.rs

+								HTLCForwardInfo::FailMalformedHTLC { htlc_id, failure_code, sha256_of_onion } => {
+									log_trace!(self.logger, "Failing malformed HTLC back to channel with short id {} (backward HTLC ID {}) after delay", short_chan_id, htlc_id);
+									if let Err(e) = chan.queue_fail_malformed_htlc(htlc_id, failure_code, sha256_of_onion, &self.logger) {
+										if let ChannelError::Ignore(msg) = e {
+											log_trace!(self.logger, "Failed to fail HTLC with ID {} backwards to short_id {}: {}", htlc_id, short_chan_id, msg);
+										} else {
+											panic!("Stated return value requirements in queue_fail_malformed_htlc() were not met");
+										}
+										// fail-backs are best-effort, we probably already have one
+										// pending, and if not that's OK, if not, the channel is on
+										// the chain and sending the HTLC-Timeout is their problem.
+										continue;
+									}


Might be able to DRY this up, too?

I don't think so, because queue_fail_htlc will have a statically chosen concrete underlying type of FailHTLCContents at compile time, IIUC.

Sure, but the error type and everything downstream of it is identical.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

jkczyz · 2023-12-11T22:21:58Z

lightning/src/ln/channel.rs

+	}
+	fn message_name() -> &'static str { "update_fail_malformed_htlc" }
+	fn inbound_htlc_state(self) -> InboundHTLCState {
+		let (failure_code, sha256_of_onion) = self;


Should we normalize the order so this doesn't need to be destructured?

Got rid of the destructuring. Not sure I got what you meant by "normalize."

By that I meant to keep the order of (failure_code, sha256_of_onion) the same between here and in InboundHTLCRemovalReason::FailMalformed. That way you just need to wrap self here rather than accessing each item in the tuple.

Oh, okay. I'll pass on this for now but I'll do it if there ends up being a follow-up to this PR.

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-12-12T22:17:57Z

Squashed and addressed feedback with 1 new fixup.

TheBlueMatt

LGTM, feel free to squash. A few nits but they're not blocking.

lightning/src/ln/msgs.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2023-12-12T22:23:53Z

lightning/src/ln/channelmanager.rs

+								HTLCForwardInfo::FailMalformedHTLC { htlc_id, failure_code, sha256_of_onion } => {
+									log_trace!(self.logger, "Failing malformed HTLC back to channel with short id {} (backward HTLC ID {}) after delay", short_chan_id, htlc_id);
+									if let Err(e) = chan.queue_fail_malformed_htlc(htlc_id, failure_code, sha256_of_onion, &self.logger) {
+										if let ChannelError::Ignore(msg) = e {
+											log_trace!(self.logger, "Failed to fail HTLC with ID {} backwards to short_id {}: {}", htlc_id, short_chan_id, msg);
+										} else {
+											panic!("Stated return value requirements in queue_fail_malformed_htlc() were not met");
+										}
+										// fail-backs are best-effort, we probably already have one
+										// pending, and if not that's OK, if not, the channel is on
+										// the chain and sending the HTLC-Timeout is their problem.
+										continue;
+									}


Sure, but the error type and everything downstream of it is identical.

jkczyz

LGTM

For context, blinded HTLCs where we are not the intro node must always be failed back with malformed and invalid_onion_blinding error per BOLT 4. Prior to supporting blinded payments, the only way for an update_malformed to be returned from Channel was if an onion was actually found to be malformed during initial update_add processing. This meant that any malformed HTLCs would never live in the holding cell but instead would be returned directly upon initial RAA processing. Now, we need to be able to store these HTLCs in the holding cell because the HTLC failure necessitating an update_malformed may come long after the RAA is initially processed, and we may not be a state to send the update_malformed message at that time. Therefore, add a new holding cell HTLC variant for blinded non-intro node HTLCs, which will signal to Channel to fail with malformed and the correct error code.

Currently it returns only update_fail, but we'll want it to be able to return update_malformed as well in upcoming commits. We'll use this for correctly failing blinded received HTLCs backwards with malformed and invalid_onion_blinding error per BOLT 4.

Useful for failing blinded payments back with malformed, and will also be useful in the future when we move onion decoding into process_pending_htlc_forwards, after which Channel::fail_htlc will be used for all malformed htlcs.

Necessary to tell the Channel how to fail these htlcs.

Makes the next commit adding support for failing blinded HTLCs in said method easier to read.

If an HTLC fails after its RAA is processed, it is failed back with ChannelManager::fail_htlc_backwards_internal. This method will now correctly inform the channel that this HTLC is blinded and to construct an update_malformed message accordingly.

If a blinded recipient to a multihop blinded path needs to fail back a malformed HTLC, they should use error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

And use it in the multihop blinded path receive failure test. Will be used in the next commit to test receiving an invalid blinded final onion payload. We can't use the existing get_route test util here because blinded payments rely on the sender adding a random shadow CLTV offset to the final hop; without this the payment will be failed with cltv-expiry-too-soon.

If a recipient behind a multihop blinded path fails to decode their onion payload, they should fail backwards with error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

If a blinded HTLC does not satisfy the receiver's requirements, e.g. bad CLTV or amount, they should malformed-fail backwards with error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLt 4.

If a blinded HTLC errors when added to a Channel, such as if the recipient has already sent a shutdown message, they should malformed-fail backwards with error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

.. contained within their encrypted payload.

Because we now support receiving to multi-hop blinded paths.

Although this new check is unreachable right now, it helps prevent potential future errors where we incorrectly fail blinded HTLCs with an unblinded error.

in Channel and ChannelManager.

valentinewallace · 2023-12-12T23:45:37Z

Squashed the fixup and added another commit addressing #2688 (comment). Sorry if I'm being dense on DRYing the match arms, but happy to address that and #2688 (comment) in a follow-up.

…followups Follow-ups to #2688

valentinewallace mentioned this pull request Oct 26, 2023

Route blinding: support forwarding as the intro node #2540

Merged

valentinewallace added the blocked on dependent pr label Oct 26, 2023

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch 2 times, most recently from cfec513 to bf84b67 Compare November 9, 2023 22:30

valentinewallace mentioned this pull request Nov 29, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

valentinewallace removed the blocked on dependent pr label Dec 1, 2023

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch from bf84b67 to 69fc9bb Compare December 4, 2023 21:11

valentinewallace added this to the 0.0.119 milestone Dec 4, 2023

valentinewallace marked this pull request as ready for review December 4, 2023 21:12

TheBlueMatt reviewed Dec 5, 2023

View reviewed changes

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

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

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

valentinewallace commented Dec 6, 2023

View reviewed changes

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

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch 2 times, most recently from c6d549e to 9b89b7e Compare December 6, 2023 20:59

jkczyz self-requested a review December 7, 2023 18:18

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch from 9b89b7e to d5dd5ea Compare December 8, 2023 19:32

valentinewallace added 5 commits December 8, 2023 14:33

Pass in update add blinding point on onion decode

e1ed52f

Will be used to read encrypted_tlvs on non-intro-node onion receipt.

Support parsing blinded non-intro onion receive payloads.

87a25c7

Support for receiving to multi-hop blinded payment paths will be completed in the next commit, sans error handling.

Support receiving to multi-hop blinded payment paths.

51f41ce

The only remaining step is to use the update_add blinding point in decoding inbound onion payloads. Error handling will be completed in upcoming commits.

Test successfully receiving to a multihop blinded path.

2a46505

Add variant for non-intro-nodes to BlindedFailure enum

df2d0a4

For use in supporting receiving to multi-hop blinded paths.

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch from d5dd5ea to 91f7361 Compare December 8, 2023 21:02

jkczyz reviewed Dec 9, 2023

View reviewed changes

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch from 91f7361 to d7e40a4 Compare December 11, 2023 19:53

valentinewallace commented Dec 11, 2023

View reviewed changes

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

TheBlueMatt reviewed Dec 11, 2023

View reviewed changes

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

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

jkczyz reviewed Dec 11, 2023

View reviewed changes

TheBlueMatt reviewed Dec 12, 2023

View reviewed changes

jkczyz reviewed Dec 12, 2023

View reviewed changes

valentinewallace added 19 commits December 12, 2023 18:38

ChannelManager: add HTLCForwardInfo variant for blinded non-intro htlcs

7bb4a23

Necessary to tell the Channel how to fail these htlcs.

Tweak initialization of HTLCForwardInfo in fail_htlc_backwards_internal

4198eda

Makes the next commit adding support for failing blinded HTLCs in said method easier to read.

Test recipient failing an HTLC received to a multi-hop blinded path

a2b4813

Fix blinded recipient fail on malformed HTLC

d99089e

If a blinded recipient to a multihop blinded path needs to fail back a malformed HTLC, they should use error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

Fix blinded recipient fail on onion decode failure

52f28e6

If a recipient behind a multihop blinded path fails to decode their onion payload, they should fail backwards with error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

Fix blinded recipient fail on receive reqs violation

eca4dc0

If a blinded HTLC does not satisfy the receiver's requirements, e.g. bad CLTV or amount, they should malformed-fail backwards with error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLt 4.

Fix blinded recipient fail on Channel error

85d3cb8

If a blinded HTLC errors when added to a Channel, such as if the recipient has already sent a shutdown message, they should malformed-fail backwards with error code INVALID_ONION_BLINDING and a zeroed out onion hash per BOLT 4.

Test successful intercept payment to 2-hop blinded path

a351301

Test received blinded HTLC failure in process_pending_htlc_forwards

93ef850

Fail blinded received HTLCs if they violate PaymentConstraints

4180803

.. contained within their encrypted payload.

Make BlindedPath::new_for_payment pub

ae08d0c

Because we now support receiving to multi-hop blinded paths.

Add redundant blinded HTLC failure check for posterity.

11bdcda

Although this new check is unreachable right now, it helps prevent potential future errors where we incorrectly fail blinded HTLCs with an unblinded error.

Add test coverage for serialization of malformed HTLCs.

63ebde1

in Channel and ChannelManager.

Add release note for blinded HTLC serialization.

ecd8238

valentinewallace force-pushed the 2023-10-multihop-blinded-recv branch from 643cf93 to ecd8238 Compare December 12, 2023 23:39

Add missing keysend preimage check on inbound onion read.

6b66271

TheBlueMatt approved these changes Dec 13, 2023

View reviewed changes

jkczyz approved these changes Dec 13, 2023

View reviewed changes

valentinewallace merged commit 9856fb6 into lightningdevkit:main Dec 13, 2023
14 of 15 checks passed

valentinewallace mentioned this pull request Dec 13, 2023

Multi-hop BlindedPath creation interface #2781

Merged

TheBlueMatt added a commit that referenced this pull request Jan 11, 2024

Merge pull request #2791 from valentinewallace/2023-12-multihop-recv-…

db81c65

…followups Follow-ups to #2688

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support receiving to multi-hop blinded paths #2688

Support receiving to multi-hop blinded paths #2688

valentinewallace commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 26, 2023 •

edited

Loading

valentinewallace commented Dec 1, 2023

valentinewallace commented Dec 6, 2023

valentinewallace commented Dec 8, 2023

jkczyz Dec 9, 2023

valentinewallace Dec 11, 2023

jkczyz Dec 9, 2023

jkczyz Dec 9, 2023

valentinewallace Dec 11, 2023

valentinewallace Dec 12, 2023 •

edited

Loading

jkczyz Dec 9, 2023

valentinewallace Dec 11, 2023 •

edited

Loading

TheBlueMatt Dec 12, 2023

jkczyz Dec 11, 2023

valentinewallace Dec 12, 2023

jkczyz Dec 12, 2023

valentinewallace Dec 12, 2023

valentinewallace commented Dec 12, 2023

TheBlueMatt left a comment

TheBlueMatt Dec 12, 2023

jkczyz left a comment

valentinewallace commented Dec 12, 2023

Support receiving to multi-hop blinded paths #2688

Support receiving to multi-hop blinded paths #2688

Conversation

valentinewallace commented Oct 26, 2023 • edited Loading

codecov-commenter commented Oct 26, 2023 • edited Loading

Codecov Report

valentinewallace commented Dec 1, 2023

valentinewallace commented Dec 6, 2023

valentinewallace commented Dec 8, 2023

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

valentinewallace Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Dec 11, 2023 • 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

valentinewallace commented Dec 12, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

valentinewallace commented Dec 12, 2023

valentinewallace commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 26, 2023 •

edited

Loading

valentinewallace Dec 12, 2023 •

edited

Loading

valentinewallace Dec 11, 2023 •

edited

Loading