Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregate BlindedPayInfo for blinded routes #2514

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Aug 22, 2023

BOLT 12 invoices encode BlindedPayInfo(s) as part of their hints for routing, whose fields are computed by aggregating fee/cltv/etc values from the hops along the blinded path. Helps address #1970.

Based on #2412.

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Patch coverage: 88.19% and no project coverage change.

Comparison is base (61d896d) 90.59% compared to head (88a0063) 90.59%.
Report is 41 commits behind head on main.

❗ Current head 88a0063 differs from pull request most recent head 134f6f3. Consider uploading reports for the commit 134f6f3 to get more accurate results

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

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2514    +/-   ##
========================================
  Coverage   90.59%   90.59%            
========================================
  Files         110      110            
  Lines       57410    58062   +652     
  Branches    57410    58062   +652     
========================================
+ Hits        52008    52602   +594     
- Misses       5402     5460    +58     
Files Changed Coverage Δ
lightning/src/blinded_path/mod.rs 100.00% <ø> (ø)
lightning/src/blinded_path/payment.rs 78.90% <85.59%> (+78.90%) ⬆️
lightning/src/ln/monitor_tests.rs 98.06% <85.71%> (-0.46%) ⬇️
lightning/src/chain/chainmonitor.rs 94.91% <97.22%> (-0.12%) ⬇️

... and 31 files with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@valentinewallace valentinewallace mentioned this pull request Aug 13, 2023
1 task
@valentinewallace valentinewallace added this to the 0.0.117 milestone Aug 23, 2023
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch from 63025f8 to e87c68e Compare August 23, 2023 21:11
@jkczyz jkczyz self-requested a review August 23, 2023 21:23
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch 2 times, most recently from b159729 to 35b1dc8 Compare August 24, 2023 15:33
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch 3 times, most recently from c6df2b6 to 4b9f000 Compare August 28, 2023 17:17
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch from 4b9f000 to 3ec7789 Compare August 28, 2023 17:23
@valentinewallace
Copy link
Contributor Author

Rebased to fix CI. I added test coverage for the min htlc computation to #2413 so this should be good for review.

lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch 5 times, most recently from f272645 to 5d0a982 Compare August 30, 2023 15:38
Copy link
Contributor

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorta confused by htlc_minimum_msat, everything else looks good

lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch from 5d0a982 to 4777c29 Compare September 6, 2023 18:14
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sorry for all the noise here, only one comment left I think.

@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch from 4777c29 to a6f881b Compare September 6, 2023 22:17
@valentinewallace
Copy link
Contributor Author

I think I addressed all feedback. Thought I saw something about not decrementing htlc_min but I don't see it anymore?

@TheBlueMatt
Copy link
Collaborator

#2514 (comment) :)

@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch from a6f881b to 88a0063 Compare September 7, 2023 17:33
TheBlueMatt
TheBlueMatt previously approved these changes Sep 8, 2023
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch 2 times, most recently from 61658ae to a1b3fbb Compare September 8, 2023 14:37
@valentinewallace valentinewallace force-pushed the 2023-08-compute-blindedpayinfo branch from a1b3fbb to f3616e6 Compare September 8, 2023 14:43
@TheBlueMatt TheBlueMatt merged commit 448b191 into lightningdevkit:main Sep 10, 2023
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants