Route blinding groundwork #2128

valentinewallace · 2023-03-24T19:29:43Z

This lays some groundwork for supporting sending and receiving over blinded payment paths.

To support route blinding, we want to split OnionHopData into two separate
structs, one for inbound onions and one for outbound onions. This is because
blinded payloads change the fields present in the onion hop data struct based
on whether we're sending vs receiving (outbound onions include encrypted blobs,
inbound onions can decrypt those blobs and contain the decrypted fields
themselves).

In upcoming work, we'll add variants for blinded payloads to the new enums.

Also includes a few small related refactors and code moves.

re-add fuzz test coverage for onion hop data

TheBlueMatt · 2023-03-27T17:19:55Z

Do have the followup commits somewhere? I'm a bit confused by the last two here on their own - we end up just renaming some structs so that we have two identical structs for onions, would be nice to see something that makes sense on its own.

valentinewallace · 2023-03-27T17:55:13Z

Do have the followup commits somewhere? I'm a bit confused by the last two here on their own - we end up just renaming some structs so that we have two identical structs for onions, would be nice to see something that makes sense on its own.

Here's the WIP commit that I'm slowly translating into PRs: valentinewallace@3460250#diff-350147204a45ffef3f58ea74ece349fe18296fbbf5a1e0a55acde6547f785a3dR1147 Link goes to the completed new In/OutboundPayload structs.

It's fairly messy, so I'm also happy to close this PR until the follow-up(s) are open if that makes more sense.

TheBlueMatt · 2023-03-31T20:12:02Z

Yea, I guess I'd prefer to review something that's logically complete, or is at least split into two PRs that are ready to go.

In upcoming blinded paths work, this method will grow to handle blinded forwards.

codecov-commenter · 2023-07-13T21:34:37Z

Codecov Report

Patch coverage: 94.51% and project coverage change: +1.36% 🎉

Comparison is base (df237ba) 90.23% compared to head (b0b549c) 91.59%.
Report is 99 commits behind head on main.

❗ 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    #2128      +/-   ##
==========================================
+ Coverage   90.23%   91.59%   +1.36%     
==========================================
  Files         106      106              
  Lines       55213    70622   +15409     
  Branches    55213    70622   +15409     
==========================================
+ Hits        49819    64688   +14869     
- Misses       5394     5934     +540

Files Changed	Coverage Δ
lightning/src/ln/channel.rs	`92.38% <84.61%> (+2.93%)`	⬆️
lightning/src/ln/onion_utils.rs	`91.06% <91.66%> (+0.38%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.45% <92.50%> (+3.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`99.00% <100.00%> (+0.73%)`	⬆️
lightning/src/ln/msgs.rs	`86.08% <100.00%> (+1.48%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`98.50% <100.00%> (+0.23%)`	⬆️

... and 68 files with indirect coverage changes

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

TheBlueMatt

LGTM, one question.

fuzz/src/bin/gen_target.sh

jkczyz · 2023-07-31T22:14:53Z

lightning/src/ln/channelmanager.rs

+		let (payment_data, keysend_preimage, payment_metadata) = match hop_data.format {
+			msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } =>
+				(payment_data, keysend_preimage, payment_metadata),
+			_ =>
+				return Err(InboundOnionErr {
+					err_code: 0x4000|22,
+					err_data: Vec::new(),
+					msg: "Got non final data with an HMAC of 0",
+				}),


Does it matter that this check happens before other checks now?

I don't think so because these errors won't be processed until process_pending_htlc_forwards is eventually called, so I don't think it changes any sort of timing from our peer's perspective.

What I meant was, could we return a different error now if the order of the error checks have changed? Or does something preclude that? (i.e., the conditions leading to them do not overlap)

Yeah, we might return a different error now if multiple errors apply. I don't think it matters, though. The one error that's in a different order now is the Got non final data with HMAC of 0, which should be really rare/indicate a very buggy sender.

lightning/src/ln/channelmanager.rs

lightning/src/ln/msgs.rs

jkczyz

LGTM assuming the fuzz discussions are resolved. Feel free to squash.

This also set us up for supporting receiving blinded onion payloads.

To support route blinding, we want to split OnionHopData into two separate structs, one for inbound onions and one for outbound onions. This is because blinded payloads change the fields present in the onion hop data struct based on whether we're sending vs receiving (outbound onions include encrypted blobs, inbound onions can decrypt those blobs and contain the decrypted fields themselves). In upcoming commits, we'll add variants for blinded payloads to the new InboundPayload enum.

Follows on from the previous commit, see its message

valentinewallace · 2023-08-02T19:58:44Z

Squashed. @TheBlueMatt let me know on the fuzzing stuff.

alecchendev

Don't have much to add, LGTM

TheBlueMatt · 2023-08-08T19:16:08Z

lightning/src/ln/channelmanager.rs

@@ -341,7 +341,7 @@ impl HTLCSource {
 	}
 }

-struct ReceiveError {
+struct InboundOnionErr {


nit: the error contents themselves here are generic failure codes for any error during HTLC receipt, I'm not sure why we need to rename the struct to refer to the onion decode error.

TheBlueMatt · 2023-08-08T19:42:53Z

lightning/src/ln/msgs.rs

 			payment_data: Option<FinalOnionHopData>,
 			payment_metadata: Option<Vec<u8>>,
 			keysend_preimage: Option<PaymentPreimage>,
+			amt_msat: u64,
+			outgoing_cltv_value: u32,


Since this is now about the receiving end, do we want to rename this and the amount to be more descriptive? Same on the outbound edge.

TheBlueMatt · 2023-08-08T19:56:46Z

lightning/src/ln/channel.rs

@@ -3077,9 +3077,9 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

 			let mut htlc_updates = Vec::new();
 			mem::swap(&mut htlc_updates, &mut self.context.holding_cell_htlc_updates);
-			let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len());
-			let mut update_fulfill_htlcs = Vec::with_capacity(htlc_updates.len());
-			let mut update_fail_htlcs = Vec::with_capacity(htlc_updates.len());


…followups #2128 follow-ups

valentinewallace marked this pull request as draft March 31, 2023 20:13

valentinewallace mentioned this pull request May 12, 2023

BOLT12 Offers Tracking Issue #1970

Open

57 tasks

Move pending forward htlc construction into method

9c5acf1

In upcoming blinded paths work, this method will grow to handle blinded forwards.

valentinewallace force-pushed the 2023-03-route-blinding-groundwork branch from 059bfe4 to eb3af58 Compare July 13, 2023 19:25

valentinewallace mentioned this pull request Jul 13, 2023

Route blinding MVP #2413

Merged

1 task

valentinewallace added this to the 0.0.117 milestone Jul 13, 2023

valentinewallace force-pushed the 2023-03-route-blinding-groundwork branch from eb3af58 to e9de303 Compare July 14, 2023 19:04

valentinewallace marked this pull request as ready for review July 14, 2023 19:12

jkczyz self-requested a review July 14, 2023 21:42

valentinewallace force-pushed the 2023-03-route-blinding-groundwork branch 2 times, most recently from 53202ea to e76025a Compare July 17, 2023 21:01

TheBlueMatt previously approved these changes Jul 30, 2023

View reviewed changes

fuzz/src/bin/gen_target.sh Show resolved Hide resolved

TheBlueMatt added the Seeking Code Review label Jul 30, 2023

jkczyz reviewed Jul 31, 2023

View reviewed changes

valentinewallace dismissed TheBlueMatt’s stale review via 0fa51de August 1, 2023 21:12

valentinewallace force-pushed the 2023-03-route-blinding-groundwork branch from e76025a to 0fa51de Compare August 1, 2023 21:12

valentinewallace assigned TheBlueMatt and jkczyz Aug 1, 2023

valentinewallace removed the Seeking Code Review label Aug 1, 2023

valentinewallace force-pushed the 2023-03-route-blinding-groundwork branch from 0fa51de to b0b549c Compare August 1, 2023 23:23

jkczyz reviewed Aug 2, 2023

View reviewed changes

valentinewallace added 5 commits August 2, 2023 12:54

Remove indentation in payment receive util

9473f1c

This also set us up for supporting receiving blinded onion payloads.

Remove outdated documentation of a panic

0c37488

Replace OnionHopData with OutboundPayload for outbound onions

67868ae

Follows on from the previous commit, see its message

Remove unnecessary vecs in channel.rs

c9d3544

valentinewallace force-pushed the 2023-03-route-blinding-groundwork branch from b0b549c to c9d3544 Compare August 2, 2023 19:54

alecchendev reviewed Aug 8, 2023

View reviewed changes

jkczyz approved these changes Aug 8, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 8, 2023

View reviewed changes

TheBlueMatt merged commit 4b24135 into lightningdevkit:main Aug 8, 2023
14 checks passed

valentinewallace mentioned this pull request Dec 18, 2023

#2128 follow-ups #2801

Merged

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

Merge pull request #2801 from valentinewallace/2023-12-rb-groundwork-…

62d52c6

…followups #2128 follow-ups

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route blinding groundwork #2128

Route blinding groundwork #2128

valentinewallace commented Mar 24, 2023 •

edited

TheBlueMatt commented Mar 27, 2023

valentinewallace commented Mar 27, 2023

TheBlueMatt commented Mar 31, 2023

codecov-commenter commented Jul 13, 2023 •

edited

TheBlueMatt left a comment

jkczyz Jul 31, 2023

valentinewallace Aug 1, 2023

jkczyz Aug 1, 2023

valentinewallace Aug 1, 2023

jkczyz left a comment

valentinewallace commented Aug 2, 2023

alecchendev left a comment

TheBlueMatt Aug 8, 2023

TheBlueMatt Aug 8, 2023

TheBlueMatt Aug 8, 2023

Route blinding groundwork #2128

Route blinding groundwork #2128

Conversation

valentinewallace commented Mar 24, 2023 • edited

TheBlueMatt commented Mar 27, 2023

valentinewallace commented Mar 27, 2023

TheBlueMatt commented Mar 31, 2023

codecov-commenter commented Jul 13, 2023 • edited

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

jkczyz Jul 31, 2023

Choose a reason for hiding this comment

valentinewallace Aug 1, 2023

Choose a reason for hiding this comment

jkczyz Aug 1, 2023

Choose a reason for hiding this comment

valentinewallace Aug 1, 2023

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

valentinewallace commented Aug 2, 2023

alecchendev left a comment

Choose a reason for hiding this comment

TheBlueMatt Aug 8, 2023

Choose a reason for hiding this comment

TheBlueMatt Aug 8, 2023

Choose a reason for hiding this comment

TheBlueMatt Aug 8, 2023

Choose a reason for hiding this comment

valentinewallace commented Mar 24, 2023 •

edited

codecov-commenter commented Jul 13, 2023 •

edited