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

public create_payment_onion in onion_utils #2677

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

Evanfeenstra
Copy link
Contributor

Ok, one last PR from me on this subject :)

Adds create_payment_onion and peel_payment_onion functions in ln::onion_utils. I added a new PeeledPayment struct instead of using the existing (very similar) Hop struct, to simplify the returned value and avoid leaking the fuzzy_internal_msgs types.

This PR is only additive, the only lines changed are just making things public

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

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

Comparison is base (d2242f6) 88.78% compared to head (0c7b647) 88.77%.
Report is 31 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2677      +/-   ##
==========================================
- Coverage   88.78%   88.77%   -0.02%     
==========================================
  Files         112      113       +1     
  Lines       88474    89143     +669     
  Branches    88474    89143     +669     
==========================================
+ Hits        78553    79136     +583     
- Misses       7686     7751      +65     
- Partials     2235     2256      +21     
Files Coverage Δ
lightning/src/ln/channelmanager.rs 78.73% <100.00%> (-0.09%) ⬇️
lightning/src/ln/mod.rs 82.60% <ø> (ø)
lightning/src/ln/msgs.rs 77.23% <ø> (ø)
lightning/src/ln/onion_utils.rs 91.00% <66.66%> (-0.51%) ⬇️

... and 20 files with indirect coverage changes

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

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Needs a CI fix and rebase.

lightning/src/util/scid_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator

What's the usecase for peeling a payment onion? Generally that should only happen when actually forwarding a payment, no?

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

What's the usecase for peeling a payment onion? Generally that should only happen when actually forwarding a payment, no?

Not sure about OPs motivation, but want to note that the LSP client might eventually also need access to onion peeling when it comes to supporting LSPS4.

From what I heard, even other non-LDK based implementations would be interested in utilizing our onion tools to avoid having to reinvent the wheel.

@@ -39,6 +39,7 @@ pub(crate) mod onion_utils;
mod outbound_payment;
pub mod wire;

pub use onion_utils::{ForwardedPayment, PeeledPayment, ReceivedPayment, create_payment_onion, peel_payment_onion};
Copy link
Contributor

@tnull tnull Oct 30, 2023

Choose a reason for hiding this comment

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

Rather than exposing these directly under lightning::ln, could we rather make onion_utils pub and expose these types under lightning::ln::onion_utils?

@Evanfeenstra
Copy link
Contributor Author

What's the usecase for peeling a payment onion? Generally that should only happen when actually forwarding a payment, no?

Although we will be supporting self custody in Sphinx Chat app (via VLS), we also have a user group who cares less about self custody and more about instant onboarding and UX. Our app spins up many LN node id keys behind virtual (trustful) "private channels" from a real node. These clients can send payments privately with create_onion_payment with the server pubkey as the first hop. The server unwraps one layer with peel_payment_onion but can't know the recipient. This also gives our users total privacy in group chats as long as the client chooses a route with >2 hops (and randomizes timing, and all service providers aren't colluding). Clients can further obfuscate by routing through other clients (like our VLS hardware or even a friend's phone)

Of course its not the core use case that LDK is built for, but its pretty unprecedented for privacy (server can't track group membership, even for users on the same server!). And these users are interoperable with actual LN via route hints.

FYI most chat messages in this design are actually bolt12 onions (not payments). But we will NOT be spamming LN p2p with bolt12 just for chat (non-payment onions are out of band)

@Evanfeenstra Evanfeenstra mentioned this pull request Oct 30, 2023
@TheBlueMatt
Copy link
Collaborator

Not sure about OPs motivation, but want to note that the LSP client might eventually also need access to onion peeling when it comes to supporting LSPS4.

I don't think it makes sense to use "raw" onion peeling for this. In LSP4 we need to answer the question "would I accept this HTLC", which has a lot more checks than just looking at the onion. IMO that logic should live in LDK.

Our app spins up many LN node id keys behind virtual (trustful) "private channels" from a real node.

You mean 0conf channels or non-bitcoin-backed channels (ie 0conf where the funding just never confirms) or channels which are expected to carry no payments?

These clients can send payments privately with create_onion_payment with the server pubkey as the first hop. The server unwraps one layer with peel_payment_onion but can't know the recipient.

Why is the server unwraping the onion? Why is it not just forwarding a payment? I'm still quite confused.

@TheBlueMatt
Copy link
Collaborator

Oh, I guess you're saying like you want to forward payments but not have channels? ie the server receives an HTLC with a next-hop but that next-hop simply doesn't exist (and is intended to indicate updating a custodial balance)? We need way more than just exposing some stuff here, maybe let's chat about high-level design to get there first (and, also, why not just use a 0conf channel that never confirms?)

@tnull
Copy link
Contributor

tnull commented Oct 30, 2023

I don't think it makes sense to use "raw" onion peeling for this. In LSP4 we need to answer the question "would I accept this HTLC", which has a lot more checks than just looking at the onion. IMO that logic should live in LDK.

Fair, we should def. discuss where the corresponding code should live when we get around to implementing it. So far I assumed it would live in the ldk-lsp-client/lightning-liquidity crate and would only reuse utils exposed by the lightning crate. This would also have the benefit that we could keep the implementation relatively independent from LDK itself, making it reusable by other projects, similar to how the lightning-invoice crate is useful even if you don't use "proper" LDK itself.
We're currently considering how to make the crate more modular and how to expose more of the inner workings in a good way to make the parts reusable in different implementations. Granted, doing so for LSPS4 might be a bit more involved, but I still like to try.

@Evanfeenstra
Copy link
Contributor Author

Oh, I guess you're saying like you want to forward payments but not have channels? ie the server receives an HTLC with a next-hop but that next-hop simply doesn't exist (and is intended to indicate updating a custodial balance)? We need way more than just exposing some stuff here, maybe let's chat about high-level design to get there first (and, also, why not just use a 0conf channel that never confirms?)

Exactly. The purpose is privacy, not custody. By peeling payments like this, the client can obfuscate activity various ways, like routing through other clients (the server doesn't know if an onion delivered to a client is actually destined for that client). Curious what else is needed besides the changes in this PR... its already all working on my end, using the CLN sendonion "oblivious payments" RPC for interop with actual LN.

The other purpose is instant onboarding without a real channel open... my understanding of 0conf is a real channel open that is used before confirmation... what do you mean by 0conf that never confirms?

@Evanfeenstra Evanfeenstra force-pushed the public-onion-utils branch 2 times, most recently from d2859f4 to 0eddcbc Compare November 2, 2023 16:59
@Evanfeenstra Evanfeenstra changed the title public onion utils, scid_utils, utils for creating/peeling payments public create_payment_onion in onion_utils Nov 2, 2023
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/mod.rs Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator

Is this (potentially) superseded by #2700?

@Evanfeenstra
Copy link
Contributor Author

Is this (potentially) superseded by #2700?

This PR only has the "create_payment_onion" fn in it now (the "other side" of #2700)

@Evanfeenstra
Copy link
Contributor Author

@valentinewallace FYI I removed the +1 in best_block_height + 1 in create_onion_payment that you had suggested earlier, as it was making many tests fail. Not sure if that's needed? Or should it be up to the caller to add 1 to best_block_height?

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
@TheBlueMatt TheBlueMatt merged commit 2e33acb into lightningdevkit:main Nov 7, 2023
14 of 15 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.

None yet

5 participants