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

LSPS2: JIT Channels. #22

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

ZmnSCPxj-jr
Copy link
Contributor

@npslaney can you also tag Breez?

@cdecker

@moneyball
Copy link

@kingonly

@moneyball
Copy link

@BitcoinErrorLog

LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
LSPS3/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

ZmnSCPxj-jr commented Mar 21, 2023

A difficulty is that one mitigation plans for bad clients is to suspend forwarding until the first payment (the one that pays for the channel open) is claimed, since the client really does have to pay for the inbound capacity service. In principle if that kind of mitigation was not a consideration, there would otherwise be no need for a payment_amount and the LSP would just make non-standard onion forwards (deducting its opening_fee) until it could claim the opening_fee needed, then use standard onion forwards for all other forwards beyond that, but if the client then did not claim the non-standard forwards and claimed only the standard forwards (the ones that do not pay the opening_fee), then it got service without paying for it. At the same time, we need to know how much payment_amount total to deduct from for the first payment so that payers can use multipath payment; if the LSP did not know this payment_amount, it would only forward those set of payment parts that would let it claim its opening_fee and block the rest, but the rest of the payment parts are needed to form the full payment in a multpart payment.

LSPS3/README.md Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link

Maybe the payment amount should be optional, allowing some clients to not provide it, while others who know the first payment amount can? In general, the whole "enter the amount you want to be paid" thing is pretty terrible UX, and relying on receivers to do that in order to receive funds really sucks.

@ZmnSCPxj-jr
Copy link
Contributor Author

ZmnSCPxj-jr commented Mar 22, 2023

@TheBlueMatt writes

Maybe the payment amount should be optional, allowing some clients to not provide it, while others who know the first payment amount can? In general, the whole "enter the amount you want to be paid" thing is pretty terrible UX, and relying on receivers to do that in order to receive funds really sucks.

It kinda conflicts with the combination of the below:

  • Support multipart payments
  • Mitigation by denying use of the channel until the first received payment is completed and pays for the opening (i.e. block future payments until we can get the channel paid for).

If we can remove one or the other, then it becomes safe to completely remove payment_size and I would be willing to remove it entirely and not make it optional:

  • Remove multipart: then as soon as we get a payment part that is greater than the opening_fee we open the channel, let that single payment through but block future payments until that first payment is claimed, but now cannot support multipart.
    • e.g. suppose the receiver is set to receive 1.0 units, and opening_fee is 0.3 units. The payer splits multipart to 0.8 + 0.2 which reaches LSP in that order. LSP would let the 0.8 part through deducting 0.3 and sending the 0.5 through, but block the 0.2 units part a part of the cannot-use-unless-paid mitigation. But the receiver is expecting a total of 1.0 units(minus 0.3 opening_fee, thus 0.7 units) and cannot safely claim that payment, but the LSP will refuse to also send through the 0.2 unit other part simply because it wants to mitigate use of the channel until the opening_fee is claimed.
  • Remove cannot-use-unless-paid mitigation: we can then trivially support multipart by just letting all payments through (so that the client can wait for all parts to reach it and then claim all of them atomically), deducting from only the first ones until we claim the entire opening_fee, but now cannot mitigate if the first payments are HODLed-then-failed but subsequent payments (which did not have opening_fee deducted) are claimed.

The payment_size is basically the risk the LSP takes on; we allow up to payment_size through on multiple incoming payments, and take on the risk that up to payment_size gets through with the payments that actually deduct and pay for opening_fee are denied. Without payment_size, we cannot bound this risk on the LSP side.

Now you might point out that multipart payments with HTLCs allows the LSP to identify the parts of a single payment, but the intent is that in the future we will do PTLCs and that information will not be available to the LSP, so it would not be future-proof.

That is actually what I was explaining in #22 (comment) but I guess I was writing that sleep-deprived so it is hard to understand.

Of the two, I would be more willing to sacrifice the cannot-use-unless-paid mitigation than multipart, but Breez mentioned that and it would be better for you to discuss this with them somehow.

@ZmnSCPxj-jr
Copy link
Contributor Author

TODO: consider a lightning_fee_base and lightning_fee_proportional. Can the LSP change this, say after opening_fee_params.valid_until? Or should we require LSP to use fixed in-Lightning fees?

@JssDWt
Copy link
Contributor

JssDWt commented Mar 23, 2023

  • Remove multipart: then as soon as we get a payment part that is greater than the opening_fee we open the channel, let that single payment through but block future payments until that first payment is claimed, but now cannot support multipart.

What if the LSP takes a fixed percentage of the htlc amount for every htlc until the settled fee is greater than or equal to the price of the channel? That could lead to overpaying the fee if there is multiple payments at once, but the client can choose to fail one of the payments if it feels it is overpaying.
A scheme like this would allow for 0 amount invoices too potentially and allow for overpaying invoices. It's just the LSP fee that becomes >= instead of ==.

@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Explain LSP-trusts-client and client-trusts-LSP and why these are not compatible, use min_fee_msat instead of base_msat, require opening_fee_params_menu ordering from lowest to highest, add client_trusts_lsp return flag from lsps2.buy to indicate that LSP wants client-trusts-LSP.

LSPS2/README.md Outdated Show resolved Hide resolved
The LSP generates non-standard forwards, where the amount received
by the client is smaller than specified in the onion;
the client MUST accept the non-standard forward(s), provided they
sum to at least `payment_size_msat - opening_fee`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should refer to lightning/blips#25 at least here, maybe other places also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

blip-0025 is currently in draft state. Should we defer LSPS2 merge until after bLIP -0025 is merged?

@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Removed references to LSPS3, strongly clarify that LSP-trusts-client allows client to wait 0, 1, 3, 6, 10000 confirms

LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
Comment on lines +653 to +783
* SHOULD wait for payment parts of a single payment hash until all
parts sum up to at least `payment_size_msat`.
* MUST NOT fail with `mpp_timeout`, as the final hop is the client
and that error is only allowed at the final hop.
* MUST fail with `unknown_next_peer` if all parts have **NOT**
summed up to at least `payment_size_msat` **AND**
`opening_fee_params.valid_until` has passed.
* MAY fail with `unknown_next_peer` if it receives too many payment
parts.
Copy link
Contributor

Choose a reason for hiding this comment

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

@SeverinAlexB You can't explicitly return unknown_next_peer from LND (although the diff for that change would be trivial, similar to this breez/lnd@3bd3c78)

But a trick you can use is to forward the htlc without modifying. Because LND doesn't know the channel id, it will return unknown_next_peer.

LSPS2/README.md Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: fix typos, add links to common-schemas.

@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the jit-channels branch 4 times, most recently from 87ed947 to 326d297 Compare July 17, 2023 14:36
Copy link
Collaborator

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

Some more comments. Most importantly, there are still a lot of questions around the non-custom forward. A "Notes On Implementation" as we have it for LSPS0 would help a lot.

LSPS2/README.md Outdated Show resolved Hide resolved
Comment on lines +91 to +102
and needs to protect its funds, even though most of the time it is in
"LSP trusts client" model.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
and needs to protect its funds, even though most of the time it is in
"LSP trusts client" model.
and needs to protect its funds, even though most of the time it SHOULD be in
"LSP trusts client" model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is that this is a non-normative example, as noted in the previous line:

The LSP might do this for example if it detects it is being attacked

Thus, normative text like "SHOULD" would be inappropriate, as this is an example, not a prescription.

LSPS2/README.md Outdated
### 0. API Version

The client can determine the versions supported by the LSP via the
`lsps2.getversions` call.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why method names with no underscore? This should be lsps2.get_version. I don't understand why we whould use a different naming schema for methods compared to any other variables. Snake case everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shrug. not really motivated to change this. IMO bikeshed issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

@tnull tnull Jul 26, 2023

Choose a reason for hiding this comment

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

shrug. not really motivated to change this. IMO bikeshed issue.

This is not bikeshedding. We def. want to have a uniform API over all LSPS. I agree it doesn't really matter much which way to go as long it has the same format, but given we use snake case everywhere else we probably want to use this here also?

The LSP SHOULD provide a `valid_until` time that is at least 10 minutes from
the current time.
[<LSPS0.datetime>][]
* `min_lifetime` is a number of blocks that the LSP promises it will keep the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lifetime is defined by the client in LSPS1. Here the lsp defines the lifetime. Should we add an optional min_lifetime parameter so the client can determine the lifetime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably ask at next meeting.

LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
Comment on lines +653 to +783
* SHOULD wait for payment parts of a single payment hash until all
parts sum up to at least `payment_size_msat`.
* MUST NOT fail with `mpp_timeout`, as the final hop is the client
and that error is only allowed at the final hop.
* MUST fail with `unknown_next_peer` if all parts have **NOT**
summed up to at least `payment_size_msat` **AND**
`opening_fee_params.valid_until` has passed.
* MAY fail with `unknown_next_peer` if it receives too many payment
parts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this issue again, I don't believe LND is capable of a custom forward that differs from the amount defined in the onion. This is not good. I reached out to Alex.

Also, don't know about CLN. Our main requirement for this spec is to be compatible with the major implementations with the available API.

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.

Did another pass, mostly wording suggestions/nits.

LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated
it has no inbound capacity, and the client cannot pre-buy this capacity
(for instance, it also has no outbound capacity or on-chain funds).
* The client determines the parameters of a particular LSP via a
`lsps2.getinfo` request.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really clarify whether we'll use snake_case or not.

LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the jit-channels branch 2 times, most recently from e290bae to 70fdb14 Compare July 26, 2023 07:16
@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Add support for funding batching on LSP side, require bLIP-0025 / extra_fee, change wordings.

@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: make method names snake_case, make status DRAFT, added note about prepayment probes and how LSPS2 sucks because of it.

@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Fix a typo in the example parameters for lsps2.buy (s/base_msat/min_fee_msat/)

@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: fix conflict in README.md

@rbndg rbndg merged commit f6f1887 into BitcoinAndLightningLayerSpecs:main Jul 28, 2023
@ZmnSCPxj-jr ZmnSCPxj-jr deleted the jit-channels branch July 31, 2023 11:47
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.