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.

Sign up for GitHub

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

Jump to bottom

Allow nodes to overshoot final htlc amount and expiry #1032

Merged
merged 2 commits into from
Nov 8, 2022
Merged

Allow nodes to overshoot final htlc amount and expiry #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5a9966
Allow nodes to overshoot the final htlc amount and expiry
t-bast Oct 11, 2022
391ed84
Clarify `min_final_cltv_expiry`
t-bast Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions 02-peer-protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,9 +877,9 @@ received.

The critical settings here are the `cltv_expiry_delta` in
[BOLT #7](07-routing-gossip.md#the-channel_update-message) and the
related `min_final_cltv_expiry` in [BOLT #11](11-payment-encoding.md#tagged-fields).
related `min_final_cltv_expiry_delta` in [BOLT #11](11-payment-encoding.md#tagged-fields).
`cltv_expiry_delta` is the minimum difference in HTLC CLTV timeouts, in
the forwarding case (B). `min_final_cltv_expiry` is the minimum difference
the forwarding case (B). `min_final_cltv_expiry_delta` is the minimum difference
between HTLC CLTV timeout and the current block height, for the
terminal case (C).

Expand Down
13 changes: 7 additions & 6 deletions 04-onion-routing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,15 +250,16 @@ The reader:
- if it is not the final node:
- MUST return an error if:
- `short_channel_id` is not present,
- it cannot forward the HTLC to the peer indicated by the channel `short_channel_id`.
- incoming `amount_msat` - `fee` < `amt_to_forward` (where `fee` is the advertised fee as described in [BOLT #7](07-routing-gossip.md#htlc-fees))
- `cltv_expiry` - `cltv_expiry_delta` < `outgoing_cltv_value`
- it cannot forward the HTLC to the peer indicated by the channel `short_channel_id`.
- incoming `amount_msat` - `fee` < `amt_to_forward` (where `fee` is the advertised fee as described in [BOLT #7](07-routing-gossip.md#htlc-fees))
- `cltv_expiry` - `cltv_expiry_delta` < `outgoing_cltv_value`
- if it is the final node:
- MUST treat `total_msat` as if it were equal to `amt_to_forward` if it
is not present.
- MUST return an error if:
- incoming `amount_msat` != `amt_to_forward`.
- incoming `cltv_expiry` != `cltv_expiry_delta`.
- incoming `amount_msat` < `amt_to_forward`.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we relax this field we must also start counting payment_amt = SUM(htlcs.amt_to_forward) at the final node. If we don't, a second-to-last node can overpay the htlc amount and mess with the tracking of MPP shards (that would lead to a premature settle of the HTLC set).

This would become even worse with the relaxation of the total_msat check, as it would get very easy to cause such a premature settle for a malicious node.

I think the ultimate solution would be what is mentioned in https://github.com/lightning/bolts/pull/1032/files#r993085640, adding arequired_incoming_htlc_amt field. This would restrict all sorts of funny business routing nodes can do to the HTLC amt they send to the next hop, and would require them to always forward exactly amt_to_fwd. To me that sounds like it would solve all the probing issues also, as there would not be possible for a routing node to under- nor overpay, regardless of the next hop is the final or not.

Copy link
Collaborator Author

@t-bast t-bast Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think you're over-complicating this. From the recipient's point of view, they should be happy as soon as they receive at least total_amount_msat: they've been paid what they wanted and should release the preimage. From the sender's point of view, they should be happy as soon as they receive the preimage while paying at most total_amount_msat.

If intermediate nodes overpay an HTLC, neither the sender not the recipient are negatively impacted. The sender may even get the preimage for less than what they intended to pay: this is great, why would they prevent that?

as there would not be possible for a routing node to under- nor overpay

It should never be possible for an intermediate node to underpay, I agree with that. However, I don't see how overpaying can be an issue, and it's actually necessary to allow overpaying if you want to enable inbound fees?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the theory, but are implementations properly doing this in practice? I guess it should be handled anyway, since there are never any guarantees all shards will be settled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's at least how eclair handles it ;)

- incoming `cltv_expiry` < `outgoing_cltv_value`.
t-bast marked this conversation as resolved.
Show resolved Hide resolved
- incoming `cltv_expiry` < `current_block_height` + `min_final_cltv_expiry_delta`.
t-bast marked this conversation as resolved.
Show resolved Hide resolved

Additional requirements are specified [below](#basic-multi-part-payments).

Expand Down Expand Up @@ -397,7 +398,7 @@ the final node with the following values:
* `payment_secret`: set to the payment secret specified by the recipient (e.g.
`payment_secret` from a [BOLT #11](11-payment-encoding.md) payment invoice)
* `outgoing_cltv_value`: set to the final expiry specified by the recipient (e.g.
`min_final_cltv_expiry` from a [BOLT #11](11-payment-encoding.md) payment invoice)
`min_final_cltv_expiry_delta` from a [BOLT #11](11-payment-encoding.md) payment invoice)
* `amt_to_forward`: set to the final amount specified by the recipient (e.g. `amount`
from a [BOLT #11](11-payment-encoding.md) payment invoice)

Expand Down
6 changes: 3 additions & 3 deletions 07-routing-gossip.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,7 +1065,7 @@ channel:
3. C: 30 blocks
4. D: 40 blocks

C also uses a `min_final_cltv_expiry` of 9 (the default) when requesting
C also uses a `min_final_cltv_expiry_delta` of 9 (the default) when requesting
payments.

Also, each node has a set fee scheme that it uses for each of its
Expand All @@ -1089,7 +1089,7 @@ The network will see eight `channel_update` messages:

**B->C.** If B were to send 4,999,999 millisatoshi directly to C, it would
neither charge itself a fee nor add its own `cltv_expiry_delta`, so it would
use C's requested `min_final_cltv_expiry` of 9. Presumably it would also add a
use C's requested `min_final_cltv_expiry_delta` of 9. Presumably it would also add a
_shadow route_ to give an extra CLTV of 42. Additionally, it could add extra
CLTV deltas at other hops, as these values represent a minimum, but chooses not
to do so here, for the sake of simplicity:
Expand All @@ -1109,7 +1109,7 @@ per [HTLC Fees](#htlc-fees):
200 + ( 4999999 * 2000 / 1000000 ) = 10199

Similarly, it would need to add B->C's `channel_update` `cltv_expiry_delta` (20), C's
requested `min_final_cltv_expiry` (9), and the cost for the _shadow route_ (42).
requested `min_final_cltv_expiry_delta` (9), and the cost for the _shadow route_ (42).
Thus, A->B's `update_add_htlc` message would be:

* `amount_msat`: 5010198
Expand Down
10 changes: 5 additions & 5 deletions 11-payment-encoding.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ Currently defined tagged fields are:
* `n` (19): `data_length` 53. 33-byte public key of the payee node
* `h` (23): `data_length` 52. 256-bit description of purpose of payment (SHA256). This is used to commit to an associated description that is over 639 bytes, but the transport mechanism for the description in that case is transport specific and not defined here.
* `x` (6): `data_length` variable. `expiry` time in seconds (big-endian). Default is 3600 (1 hour) if not specified.
* `c` (24): `data_length` variable. `min_final_cltv_expiry` to use for the last HTLC in the route. Default is 18 if not specified.
* `c` (24): `data_length` variable. `min_final_cltv_expiry_delta` to use for the last HTLC in the route. Default is 18 if not specified.
* `f` (9): `data_length` variable, depending on version. Fallback on-chain address: for Bitcoin, this starts with a 5-bit `version` and contains a witness program or P2PKH or P2SH address.
* `r` (3): `data_length` variable. One or more entries containing extra routing information for a private route; there may be more than one `r` field
* `pubkey` (264 bits)
Expand Down Expand Up @@ -175,7 +175,7 @@ A writer:
- MAY include one `x` field.
- if `x` is included:
- SHOULD use the minimum `data_length` possible.
- MUST include one `c` field (`min_final_cltv_expiry`).
- MUST include one `c` field (`min_final_cltv_expiry_delta`).
- MUST set `c` to the minimum `cltv_expiry` it will accept for the last
HTLC in the route.
- SHOULD use the minimum `data_length` possible.
Expand Down Expand Up @@ -216,7 +216,7 @@ A reader:
- MUST use the `n` field to validate the signature instead of performing signature recovery.
- if there is a valid `s` field:
- MUST use that as [`payment_secret`](04-onion-routing.md#tlv_payload-payload-format)
- if the `c` field (`min_final_cltv_expiry`) is not provided:
- if the `c` field (`min_final_cltv_expiry_delta`) is not provided:
- MUST use an expiry delta of at least 18 when making the payment
- if an `m` field is provided:
- MUST use that as [`payment_metadata`](04-onion-routing.md#tlv_payload-payload-format)
Expand Down Expand Up @@ -618,9 +618,9 @@ Breakdown:
* `x`: expiry time
* `qy`: `data_length` (`q` = 0, `y` = 2; 0 * 32 + 4 == 4)
* `jw5q`: 604800 seconds (`j` = 18, `w` = 14, `5` = 20, `q` = 0; 18 * 32^3 + 14 * 32^2 + 20 * 32 + 0 == 604800)
* `c`: `min_final_cltv_expiry`
* `c`: `min_final_cltv_expiry_delta`
* `qp`: `data_length` (`q` = 0, `p` = 1; 0 * 32 + 1 == 1)
* `2`: min_final_cltv_expiry = 10
* `2`: min_final_cltv_expiry_delta = 10
* `r`: tagged field: route information
* `zj`: `data_length` (`z` = 2, `j` = 18; 2 * 32 + 18 == 82)
* `q0gxwkzc8w6323m55m4jyxcjwmy7stt9hwkwe2qxmy8zpsgg7jcuwz87fcqqeuqqqyqqqqlgqqqqn3qq9q`:
Expand Down