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

bLIP-0025: Allow forwarding HTLCs that underpay the onion encoded value #25

Conversation

valentinewallace
Copy link
Contributor

For context, it is expected that many lightning users will be connected to the lightning network via
LSPs (Lightning Service Providers), who will be responsible for managing channel liquidity on end
users' behalf.

Often, users are onboarded to these services via a just-in-time inbound channel when they first
receive a payment. However, this channel open costs money, and so liquidity providers may want to
take an extra fee from the received payment so that end users can help bear the cost of this initial
on-chain fee. This bLIP outlines how they may take said fee.

@valentinewallace valentinewallace changed the title bLIP-0024: Allow forwarding HTLCs that underpay the onion encoded value bLIP-0025: Allow forwarding HTLCs that underpay the onion encoded value May 22, 2023
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 4241675 to d17353d Compare May 22, 2023 18:57
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from d17353d to e435847 Compare May 22, 2023 19:08
@valentinewallace
Copy link
Contributor Author

This needs some fleshing out, see spec meeting notes here lightning/bolts#1082 (comment).

@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from e435847 to 3692c46 Compare May 23, 2023 23:17
@valentinewallace
Copy link
Contributor Author

I updated this to disallow MPP unless the receiver has some way of communicating to the penultimate hop when all MPP parts have been aggregated. It'd be nice to add that as a separate bLIP and update this one, in the future. Discussed with @t-bast here: https://gnusha.org/lightning-dev/2023-05-23.log

Will take this out of draft after opening a PR for the implementation in LDK and adding a link.

Copy link
Contributor

@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.

One comment, but not a huge deal.

blip-0025.md Outdated Show resolved Hide resolved
blip-0025.md Outdated Show resolved Hide resolved
blip-0002.md Show resolved Hide resolved
blip-0025.md Outdated Show resolved Hide resolved
blip-0025.md Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from 3692c46 to ed6f44e Compare July 26, 2023 22:53
@valentinewallace valentinewallace marked this pull request as ready for review July 26, 2023 22:54
@@ -101,6 +101,13 @@ The following table contains extension tlv fields for the `payment_onion_payload
| 7629169 | `podcasting_2_0` | [bLIP 10](./blip-0010.md) |
| 5482373484 | `keysend_preimage` | [bLIP 3](./blip-0003.md) |

#### `update_add_htlc`

The following table contains extension tlv fields for the `update_add_htlc` message:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
The following table contains extension tlv fields for the `update_add_htlc` message:
The following table contains extension TLV fields for the `update_add_htlc` message:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipped this to keep consistency with the rest of the file.

blip-0025.md Outdated Show resolved Hide resolved
blip-0025.md Outdated Show resolved Hide resolved
blip-0025.md Outdated Show resolved Hide resolved
blip-0025.md Outdated Show resolved Hide resolved
blip-0025.md Outdated

Penultimate hop on the path:
* In their outbound `update_add_htlc` message, MUST include a TLV record keyed by type 65537 with a
TLV value of `u64` containing the amount of extra fee they took from the receiver's final received
Copy link

Choose a reason for hiding this comment

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

Suggested change
TLV value of `u64` containing the amount of extra fee they took from the receiver's final received
TLV value of `tu64` containing the amount of extra fee they took from the receiver's final received

The bolt spec says

For the final value in TLV records, truncated integers may be used. Leading zeros in truncated integers MUST be omitted:

tu64: a 0 to 8 byte truncated unsigned integer

It makes sense to use a tu64 here, since this is the final value of the TLV. And it saves some space for free.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, we already shipped this quite a while ago, and its not like this is going to ever go from one packet on the wire to two packets on the wire, so I don't really think this is worth it.

Copy link

Choose a reason for hiding this comment

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

Ah, I didn't realize this was already shipped.

@tnull
Copy link
Contributor

tnull commented Jul 4, 2024

Hmm, given this has been used in production for a while by LSPS2 just-in-time channel providers, should we move forward with merging this eventually? (cc @valentinewallace @TheBlueMatt @t-bast)

@t-bast
Copy link
Contributor

t-bast commented Jul 4, 2024

I'm using a slightly different one in #36 to also include the related funding_txid, but both can definitely coexist depending on the use-case, so I'm fine with moving forward on this PR.

It is missing a link to the bLIP in README.md and comments should be addressed, but otherwise LGTM.

@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch 2 times, most recently from 99b5fa1 to cb889f3 Compare July 8, 2024 15:43
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Jul 8, 2024

Addressed feedback and rebased due to merge conflicts, diff prior to rebase was:

diff --git a/README.md b/README.md
index bba6b55..2f8096a 100644
--- a/README.md
+++ b/README.md
@@ -25,3 +25,4 @@ For more detail on the process, please read [bLIP-0001](./blip-0001.md) and
 | [10](./blip-0010.md)     | Podcasting 2.0            | Satoshis Stream             | Active |
 | [11](./blip-0011.md)     | NameDesc                  | Hampus Sjöberg              | Active |
 | [17](./blip-0017.md)     | Hosted Channels           | Anton Kumaigorodskiy        | Active |
+| [25](./blip-0025.md)     | Forward less than onion   | Valentine Wallace           | Active |
diff --git a/blip-0025.md b/blip-0025.md
index 01a8871..3f032d7 100644
--- a/blip-0025.md
+++ b/blip-0025.md
@@ -26,9 +26,9 @@ Penultimate hop on the path:
   value, in millisatoshis

 Receiver:
-* MUST fail the HTLC if they did not expect an extra fee to be taken
+* MUST fail the HTLC if they did not expect an extra fee to be taken or if the extra fee taken is
+  too high
 * MUST either accept or reject the HTLC as if it had received `htlc_value + extra_fee`
-* MUST reject the HTLC if the extra fee taken is too high

 ## Motivation

@@ -47,15 +47,10 @@ that the fee taken by the penultimate hop is as-expected, in practice this may b
 right. If there were a bug in this logic, a sender could exploit it by forwarding less than the
 invoice's expected value, and receive proof-of-payment that they paid the full value.

-In the JIT channel context, MPP should be disallowed unless the receiver can communicate to the LSP
-when all MPP parts have been aggregated, because otherwise the LSP may be at risk of wastefully
-opening a new channel for each MPP part.
-
-## Implementation notes
-If this feature is being used in the context of the penultimate hop opening a just-in-time channel
-to the receiver, MPP should be disabled in the invoice unless the recipient has a way to communicate
-to the penultimate hop when all MPP parts have been received. See
-<https://github.com/BitcoinAndLightningLayerSpecs/lsp/pull/22> "Invoice Generation" section.
+## Implementation Notes
+See
+<https://github.com/BitcoinAndLightningLayerSpecs/lsp/tree/main/LSPS2#3--invoice-generation>
+for invoice requirements if this feature is being used in the JIT channel context.

 ## Reference Implementations
 LDK: <https://github.com/lightningdevkit/rust-lightning/pull/2319>

@t-bast
Copy link
Contributor

t-bast commented Jul 8, 2024

We had an interesting discussion in #36 (comment) that is related to this, it's probably worth a read. This isn't a no-go at all for me if you want to go ahead with this TLV to support your existing use-cases, but it points out that it's probably hard to use as a general purpose tool.

blip-0025.md Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2023-05-forward-less-than-onion branch from cb889f3 to 1bad9b2 Compare July 8, 2024 16:06
@valentinewallace
Copy link
Contributor Author

Thanks @t-bast, updated with this diff:

diff --git a/blip-0025.md b/blip-0025.md
index 3f032d7..a306ba4 100644
--- a/blip-0025.md
+++ b/blip-0025.md
@@ -20,12 +20,19 @@ This bLIP is licensed under the CC0 license.

 ## Specification

-Penultimate hop on the path:
-* In their outbound `update_add_htlc` message, MUST include a TLV record keyed by type 65537 with a
-  TLV value of `u64` containing the amount of extra fee they took from the receiver's final received
-  value, in millisatoshis
+We define a TLV field for `update_add_htlc` that allows a relaying node to
+relay a smaller amount than the amount encoded in the onion:

-Receiver:
+1. `tlv_stream`: `update_add_htlc_tlvs`
+2. types:
+    1. type: 65537 (`extra_fee`)
+    2. data:
+        - [`u64`:`amount_msat`]
+
+The writer:
+* MUST set `extra_fee` to the amount of extra fee they took from the receiver's final value
+
+The receiver:
 * MUST fail the HTLC if they did not expect an extra fee to be taken or if the extra fee taken is
   too high
 * MUST either accept or reject the HTLC as if it had received `htlc_value + extra_fee`

@valentinewallace
Copy link
Contributor Author

We had an interesting discussion in #36 (comment) that is related to this, it's probably worth a read. This isn't a no-go at all for me if you want to go ahead with this TLV to support your existing use-cases, but it points out that it's probably hard to use as a general purpose tool.

Hmm so it sounds like this bLIP is not very compatible with splicing and may be deprecated in the near future? I don't feel strongly about what to do about that, maybe @tnull can weigh in. It sounds like this may still be useful for compat with LSPS2?

@TheBlueMatt
Copy link
Contributor

I'll follow up there but I'm really not clear on why we want something different in bLIP 36. In any case, however, this is live in existing LSPS specs so we should at least land a bLIP that describes it.

@tnull
Copy link
Contributor

tnull commented Jul 8, 2024

Hmm so it sounds like this bLIP is not very compatible with splicing and may be deprecated in the near future? I don't feel strongly about what to do about that, maybe @tnull can weigh in. It sounds like this may still be useful for compat with LSPS2?

Yeah, I agree with Matt here: even if we end up going a different route in #36, we'd still want to land this as it's used and deployed by LSPS2 services.

Copy link
Contributor

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 1bad9b2, LGTM 👍, feel free to merge this!

@TheBlueMatt TheBlueMatt merged commit 43d18b5 into lightning:master Jul 9, 2024
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.

6 participants