Trampoline payments #1034

JssDWt · 2024-07-08T13:56:06Z

This PR adds trampoline payment functionality to the SDK.

If a client sends a payment, normally the greenlight node will find routes through the gossip graph, and attempt to pay over those routes. If a route fails, it tries again. For every attempt there are multiple rounds where the greenlight node has to communicate with the signer. Trampoline payments have the advantage there will be only one outgoing payment attempt, and the LSP will do the retrying, therefore less communication rounds are necessary, hopefully improving payment performance. The tradeoff is the user may pay more fees for the route, and the user loses some privacy in the current setup, because the LSP will learn the payment destination. The initial default trampoline policy will be 0,5%.

This PR leans on the corresponding greenlight PR for trampoline payments, which is still a work in process: Blockstream/greenlight#475

The LSP will need to run this plugin: https://github.com/breez/trampoline

Some notes about this PR:

Trampoline payments should only be attempted when the LSP is not in the route hint. This is because the LSP side trampoline plugin doesn't cooperate nicely with lspd in it's current state.
The trampoline payments are automatically fetched with listpays. That's nice, because it doesn't require any changes to sync. The payments from listpays have the wrong amount_msat due to the way trampoline payments work. We solve that by putting the actual amount in a payment label for trampoline payments.

TODO:

needs update of the greenlight commit id before getting merged, since it's pointing at a PR

dangeross

Looking good. I would suggest also:

Checking that the LSP can do trampoline payments (via an LSPInformation flag?)
Having a config option to not use trampoline payments (for privacy preserving)
Is it worth prechecking the outbound liquidity to the LSP before the trampoline payment or can the LSP more efficiently do that? (This is think about the edge case of switching LSP and maybe most/all of the liquidity is still with the old LSP)

roeierez · 2024-07-10T06:30:42Z

Good points @dangeross .
Regarding checking lsp liquidity since this check comes with some latency price for each payment and it is an edge case I would consider trying trampoline and if failing for liquidity fallback to regular payment. I think it should fail fast since the node plugin should check that before sending any htlcs

JssDWt · 2024-07-12T08:17:28Z

@dangeross @roeierez please check my last 7 commits.
Instead of calling lsp_info (which is a network call) I added two changes:

the LSP pubkey is now cached for quick lookup.
the node_state now contains the peer's 'features' as well (currently only trampoline is a supported feature)

With these two pieces of information we can check:

does the route hint contain a hint to our LSP?
Does the LSP support trampoline?

Next to that I added a flag in the SendPaymentRequest and LnurlPayRequest to signal the user doesn't want to use trampoline.

dangeross

Looking good @JssDWt

dangeross · 2024-07-12T15:26:33Z

libs/sdk-core/src/breez_services.rs

+            let features = self
+                .node_api
+                .connect_peer(node_id.clone(), address.clone())
+                .await
+                .map_err(|e| SdkError::ServiceConnectivity {
+                    err: format!("(LSP: {node_id}) Failed to connect: {e}"),
+                })?;
+            node_state.connected_peers.push(ConnectedPeer {
+                id: node_id.clone(),
+                features,
+            });
+            self.persister.set_node_state(&node_state)?;


I'm wondering if updating the node_state's connected peers could done in node_api.connect_peer(), but AFAIK this is the only time its called

libs/sdk-core/src/models.rs

JssDWt · 2024-07-12T16:22:58Z

I had a case where the trampoline payment was not attempted. I don't think this mechanism is robust enough. Maybe the peer feature check has to be removed?

JssDWt · 2024-07-13T13:11:09Z

The status of this PR today is:

Trampoline payments work.
There is a transport error from greenlight's trampoline plugin that makes it seem the payment failed.
The peer feature check doesn't seem robust enough. Trampoline payments were skipped at least once when testing. It could be we have to remove the following lines so the greenlight trampoline plugin is in charge of the feature check. If these lines are removed, the corresponding peerfeatures migration and the udl change can also be removed.
https://github.com/breez/breez-sdk/pull/1034/files#diff-6f499d3ded5e493c1bd95b827a0a93aeb3d2bfa3d04a6f26be62d87aff026e43R370-R384
I wonder whether trampoline_pay has the same assurences as pay in the sense that it should always work and wait for the node to be available.

JssDWt · 2024-08-12T11:04:27Z

Trampoline payments now are opt-in through the use_trampoline flag.
Removed the feature bit check on the client side, meaning the client will always call trampoline_pay when opting in to trampoline payments. Even if the LSP doesn't support them. The call should fail fast though.

JssDWt · 2024-08-12T11:11:45Z

Re-requesting review from @roeierez and @dangeross

dangeross

Looks good I think, just 2 issues with the comments

libs/sdk-common/src/lnurl/specs/pay.rs

libs/sdk-core/src/models.rs

dangeross

LGTM.

I imagine at some point people will ask about LSP trampoline fees. Maybe we should follow up at some point to add the fee rate to the LSP info?

JssDWt · 2024-08-12T15:55:16Z

I imagine at some point people will ask about LSP trampoline fees. Maybe we should follow up at some point to add the fee rate to the LSP info?

Yes.
The fee rate is currently not advertised in our custom protocol. That could be an additional feature. We assume a fixed fee of 0.5%.

roeierez

Looks good!

roeierez · 2024-08-13T07:15:50Z

libs/sdk-core/src/breez_services.rs

+        self.persister.set_lsp_id(lsp.id)?;
+        self.persister.set_lsp_pubkey(lsp.pubkey.clone())?;


As it seems these functions are always being called together now, perhaps better to have one function that takes these two arguments?

This is currently in 2 places, and setting lsp id without pubkey is in one place here. If you still think we should make that a single function, I'll fix that. We could make the function check whether the new lsp id is the same, if it's not, erase the lsp pubkey if not supplied.

I see, I missed that. Then it is less priority but I think it is slightly better to have one function.

Now setting these in a single function in 4a0a150

roeierez · 2024-08-13T07:18:43Z

libs/sdk-core/src/greenlight/node_api.rs

+        // TODO: Ensure this works with trampoline payments
+        // NOTE: If this doesn't work with trampoline payments, the sync also
+        // needs updating.
+        let payment = Self::fetch_outgoing_payment_with_retry(client, result.payment_hash).await?;


About the comment, do we know if that works with trampoline?

Not yet, for that we need the transport error to be resolved.

roeierez · 2024-08-13T07:23:18Z

libs/sdk-core/src/greenlight/node_api.rs

+
+        // For trampoline payments the amount_msat doesn't match the actual
+        // amount. If it's a trampoline payment, take the amount from the label.
+        let (payment_amount, client_label) = serde_json::from_str::<PaymentLabel>(payment.label())


Do we have the bolt11 we paid in case of trampoline? If so perhaps better to take the amount from there? If we don't perhaps it is better to persist it as part of the label? I think from UX perspective the user needs the paid invoice.

The invoice is there, but possibly it's a zero amount invoice (which we support). Having the amount in the label handles both cases.

Right. I think we have a mechanism that adds the amount (in case of zero invoice) to the extrenal_info (persist_pending_payment).
But I am not against of having that in the label which I think is better.

JssDWt · 2024-08-15T08:29:34Z

@dangeross re-requesting review due to this added commit: 4a0a150

JssDWt · 2024-08-15T10:08:28Z

Bumped greenlight because Blockstream/greenlight#475 was merged

tools/sdk-cli/src/commands.rs

extract trampoline payment data from label

This update ensures that lsp pubkey and lsp id are always set in the same call. Sometimes the lsp pubkey is not yet available. Then only the lsp id will be set. If the lsp id changes, the pubkey is then removed. If the lsp id remains the same, the node pubkey will still be persisted.

Changes introduced in: breez/breez-sdk-greenlight#1034

JssDWt force-pushed the jssdwt-trampoline branch from 90bffef to d7263e5 Compare July 8, 2024 13:57

JssDWt requested a review from roeierez July 8, 2024 14:03

JssDWt force-pushed the jssdwt-trampoline branch from d7263e5 to 05e29e7 Compare July 8, 2024 19:53

dangeross reviewed Jul 10, 2024

View reviewed changes

JssDWt force-pushed the jssdwt-trampoline branch 2 times, most recently from 8a68398 to 5371c3c Compare July 12, 2024 07:41

dangeross reviewed Jul 12, 2024

View reviewed changes

JssDWt changed the title ~~DRAFT: Trampoline payments~~ Trampoline payments Jul 12, 2024

JssDWt marked this pull request as ready for review July 12, 2024 21:29

JssDWt force-pushed the jssdwt-trampoline branch from 38d0eaf to dfccdd6 Compare July 13, 2024 13:12

JssDWt force-pushed the jssdwt-trampoline branch 5 times, most recently from bd8b83f to 11607cd Compare August 12, 2024 11:02

JssDWt requested a review from dangeross August 12, 2024 11:11

dangeross reviewed Aug 12, 2024

View reviewed changes

libs/sdk-common/src/lnurl/specs/pay.rs Outdated Show resolved Hide resolved

libs/sdk-core/src/models.rs Outdated Show resolved Hide resolved

JssDWt force-pushed the jssdwt-trampoline branch from 11607cd to 6417851 Compare August 12, 2024 11:45

dangeross approved these changes Aug 12, 2024

View reviewed changes

roeierez requested changes Aug 13, 2024

View reviewed changes

roeierez approved these changes Aug 13, 2024

View reviewed changes

JssDWt requested a review from dangeross August 15, 2024 08:29

JssDWt force-pushed the jssdwt-trampoline branch from 4a0a150 to bfbe095 Compare August 15, 2024 10:07

dangeross reviewed Aug 15, 2024

View reviewed changes

tools/sdk-cli/src/commands.rs Show resolved Hide resolved

dangeross approved these changes Aug 15, 2024

View reviewed changes

JssDWt added 8 commits August 15, 2024 16:11

bump greenlight

43e84f9

persist the lsp pubkey in cache

df81604

connect to lsp also with existing node

b48b29c

add use_trampoline flags to send_payment and lnurl_pay

69a3f98

add trampoline client support

75a21b3

extract trampoline payment data from label

add trampoline payment support

42c0c07

saturating sub payment fee

1fd05f3

JssDWt force-pushed the jssdwt-trampoline branch from bfbe095 to 985bfed Compare August 15, 2024 14:11

JssDWt merged commit 985bfed into main Aug 16, 2024
9 checks passed

JssDWt temporarily deployed to github-pages August 16, 2024 11:37 — with GitHub Actions Inactive

erdemyerebasmaz added a commit to breez/breez-sdk-liquid that referenced this pull request Aug 20, 2024

Add use_trampoline to LnUrlPayRequest and generate bindings

87ed4e4

Changes introduced in: breez/breez-sdk-greenlight#1034

erdemyerebasmaz added a commit to breez/breez-sdk-liquid that referenced this pull request Aug 20, 2024

Add use_trampoline to LnUrlPayRequest and generate bindings

ed3654a

Changes introduced in: breez/breez-sdk-greenlight#1034

erdemyerebasmaz added a commit to breez/breez-sdk-liquid that referenced this pull request Aug 20, 2024

Add use_trampoline to LnUrlPayRequest and generate bindings

97d6099

Changes introduced in: breez/breez-sdk-greenlight#1034

JssDWt deleted the jssdwt-trampoline branch October 14, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trampoline payments #1034

Trampoline payments #1034

JssDWt commented Jul 8, 2024 •

edited

Loading

dangeross left a comment

roeierez commented Jul 10, 2024

JssDWt commented Jul 12, 2024 •

edited

Loading

dangeross left a comment

dangeross Jul 12, 2024

JssDWt commented Jul 12, 2024 •

edited

Loading

JssDWt commented Jul 13, 2024 •

edited

Loading

JssDWt commented Aug 12, 2024

JssDWt commented Aug 12, 2024

dangeross left a comment

dangeross left a comment

JssDWt commented Aug 12, 2024

roeierez left a comment

roeierez Aug 13, 2024

JssDWt Aug 13, 2024

roeierez Aug 13, 2024

JssDWt Aug 15, 2024

roeierez Aug 13, 2024

JssDWt Aug 13, 2024

roeierez Aug 13, 2024

JssDWt Aug 13, 2024

roeierez Aug 13, 2024

JssDWt commented Aug 15, 2024

JssDWt commented Aug 15, 2024

		self.persister.set_lsp_id(lsp.id)?;
		self.persister.set_lsp_pubkey(lsp.pubkey.clone())?;

Trampoline payments #1034

Trampoline payments #1034

Conversation

JssDWt commented Jul 8, 2024 • edited Loading

dangeross left a comment

Choose a reason for hiding this comment

roeierez commented Jul 10, 2024

JssDWt commented Jul 12, 2024 • edited Loading

dangeross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JssDWt commented Jul 12, 2024 • edited Loading

JssDWt commented Jul 13, 2024 • edited Loading

JssDWt commented Aug 12, 2024

JssDWt commented Aug 12, 2024

dangeross left a comment

Choose a reason for hiding this comment

dangeross left a comment

Choose a reason for hiding this comment

JssDWt commented Aug 12, 2024

roeierez left a 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

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

JssDWt commented Aug 15, 2024

JssDWt commented Aug 15, 2024

JssDWt commented Jul 8, 2024 •

edited

Loading

JssDWt commented Jul 12, 2024 •

edited

Loading

JssDWt commented Jul 12, 2024 •

edited

Loading

JssDWt commented Jul 13, 2024 •

edited

Loading