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

transient keys for signing invoice requests #133

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Jul 11, 2024

We should use a new key to sign each invoice request to improve sender privacy. Closes #116

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (9c5727f) to head (b1047ed).

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #133   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines         105     105           
======================================
  Misses        105     105           

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

@orbitalturtle orbitalturtle requested a review from dunxen August 9, 2024 20:41
@orbitalturtle orbitalturtle added this to the 0.2.0 milestone Aug 9, 2024
Copy link
Collaborator

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Generally LGTM! Just some nits and questions. Also needs rebase now :)

@@ -590,7 +590,7 @@ impl InvoicePayer for Client {

let blinded_payment_paths = tonic_lnd::lnrpc::BlindedPaymentPath {
blinded_path,
total_cltv_delta: u32::from(cltv_expiry_delta) + 120,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the context for this extra delta before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC in earlier versions of our fork of ldk-sample, payments wouldn't work without this addition. I think I got the exact number from the notes in Carla's PR here: lightningnetwork/lnd#7267 But I think CLN doesn't like this additional delta & newer version of LDK don't need it. I'll add a comment to the commit for more clarification.

src/lndk_offers.rs Outdated Show resolved Hide resolved
src/lnd.rs Outdated Show resolved Hide resolved
We should use a new key to sign each invoice request to improve sender privacy
and to ensure we can pay a particular CLN offer more than once.
In earlier versions of ldk-sample we couldn't make a payment without this
additional delta. But CLN doesn't like this additional delta and it's no longer
necessary for payments to ldk-sample.
@orbitalturtle
Copy link
Collaborator Author

@dunxen Thanks for the review as always! Just pushed up those changes.

@orbitalturtle orbitalturtle requested a review from dunxen August 13, 2024 06:12
Copy link
Collaborator

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@orbitalturtle orbitalturtle merged commit 259de8b into lndk-org:master Aug 13, 2024
10 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.

Bug: second payment to CLN offer fails
2 participants