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

Base AMP #643

Merged
merged 3 commits into from Dec 13, 2019
Merged

Base AMP #643

merged 3 commits into from Dec 13, 2019

Conversation

@rustyrussell
Copy link
Collaborator

rustyrussell commented Jul 17, 2019

This is a commit-separated and TLV-enhanced version of #511 by @ZmnSCPxj

We need the total amount, due to amountless invoices.

Copy link
Collaborator

ZmnSCPxj left a comment

Otherwise seems OK to me.

04-onion-routing.md Outdated Show resolved Hide resolved
@@ -277,6 +277,10 @@ The field is big-endian. The least-significant bit is numbered 0,
which is _even_, and the next most significant bit is numbered 1,
which is _odd_.

| Bits | Name |Description | Link |
|------|-------------|-----------------------|------------------------------------------------|
| 0/1 | `basic_mpp` | Payee understands mpp.| [BOLT #4](04-onion-routing.md#basic-multi-part-payments) |

This comment has been minimized.

Copy link
@ZmnSCPxj

ZmnSCPxj Jul 17, 2019

Collaborator

What would be the point of using the even bit?

For odd bits, it seems to me to mean, "payer may use basic_mpp".

For even bits, what is it mean? "Payer must use basic_mpp"?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 18, 2019

Author Collaborator

We always assign in pairs. In future, it's theoretically possible you could set this if no single channel had sufficient capacity, I guess, but it's a bit weird!

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

We may also want to add an additional section here w.r.t path finding advisory using routing hints. If the amount of the invoice is greater than available inbound bandwidth of any of their channels, then they can "guide" the sender with regular or "special" routing hints to increase the likelihood of success.

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch from 435c1ed to 7dc7a40 Jul 17, 2019
Copy link
Collaborator

t-bast left a comment

Good stuff, this is great to get started with rather simple multi-path payments and learn from real-world usage before adding more advanced features.

04-onion-routing.md Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
11-payment-encoding.md Outdated Show resolved Hide resolved
@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch from e63c872 to 70ec87d Jul 18, 2019
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Jul 18, 2019

Typos fixed, spellcheck made happy, rebased to solve conflict.

No non-formatting changes.

04-onion-routing.md Outdated Show resolved Hide resolved
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Jul 19, 2019

More typo fixes...

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch 2 times, most recently from 8ba54e6 to e79f8a8 Jul 19, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jul 27, 2019

Can be rebased now that the onion stuff has been merged in.

@@ -140,6 +141,9 @@ Currently defined tagged fields are:
* `fee_base_msat` (32 bits, big-endian)
* `fee_proportional_millionths` (32 bits, big-endian)
* `cltv_expiry_delta` (16 bits, big-endian)
* `9` (5): `data_length` variable. One or more bytes containing features

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

This is already covered for regular AMP, but for regular MPP, I think we should also include a random identifier here and require that the sender include this for end-to-end security (in the onion). Otherwise, if it's a 0-value invoice, then intermediate nodes can probe, and then use that information to possibly take most/all of the HTLC as "fees".

This comment has been minimized.

Copy link
@t-bast

t-bast Jul 29, 2019

Collaborator

Great point, there is indeed an attack vector here for the next-to-last node!
Your suggested fix assumes that the payment request will stay hidden from the intermediate nodes though (otherwise it doesn't fix anything if the intermediate nodes also know this random identifier).
It could be important to highlight this point to applications that want to leverage MPP: using MPP for donations should be a two-steps process, where a user who wants to donate needs to go to your website to get an invoice generated for him (and only him). That invoice must not be shared publicly.

@@ -277,6 +277,10 @@ The field is big-endian. The least-significant bit is numbered 0,
which is _even_, and the next most significant bit is numbered 1,
which is _odd_.

| Bits | Name |Description | Link |
|------|-------------|-----------------------|------------------------------------------------|
| 0/1 | `basic_mpp` | Payee understands mpp.| [BOLT #4](04-onion-routing.md#basic-multi-part-payments) |

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

We may also want to add an additional section here w.r.t path finding advisory using routing hints. If the amount of the invoice is greater than available inbound bandwidth of any of their channels, then they can "guide" the sender with regular or "special" routing hints to increase the likelihood of success.

@@ -257,6 +257,9 @@ This is a more flexible format, which avoids the redundant `short_channel_id` fi
1. type: 6 (`short_channel_id`)
2. data:
* [`short_channel_id`:`short_channel_id`]
1. type: 8 (`basic_mpp`)
2. data:
* [`tu64`:`total_msat`]

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

I suggest a minimal two-tuple of (amt, randID) as mentioned in my earlier comment.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 5, 2019

Contributor

Shouldn't randID be defined as a separate type, so that it can be used to pay non-amp to zero-valued invoices?

- MAY send more than one HTLC to pay the invoice.
- MUST use the same `payment_preimage` on all HTLCs in the set.
- MUST set `total_msat` to the amount it wishes to pay.
- SHOULD send all payments at approximately the same time.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

This conflicts w/ two bullets below (try to re-derive if they fail).

We should also have a section below here detailing failure modes due to partial settles, as it's possible that some portion of the funds just never get to the receiver, and become windfall for intermediate nodes.

This comment has been minimized.

Copy link
@t-bast

t-bast Jul 29, 2019

Collaborator

We should also have a section below here detailing failure modes due to partial settles, as it's possible that some portion of the funds just never get to the receiver, and become windfall for intermediate nodes.

Can you explain what you mean? I didn't get that.

- if the total `amount_msat` of this HTLC set equals or exceeds `total_msat`:
- SHOULD fulfill all HTLCs in the HTLC set
- otherwise:
- SHOULD wait for at least 60 seconds for remaining members of the HTLC set.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

A static value here isn't very encompassing. It may be the case that a higher level application has the sender purposefully delay the remaining shards.

This comment has been minimized.

Copy link
@t-bast

t-bast Jul 29, 2019

Collaborator

True but I think the receiver should enforce a maximum duration for a receive session, otherwise a malicious sender can lock funds from a lot of people almost indefinitely (if what you were thinking of was some kind of sliding window on the receiver end where you reset the timeout each time you receive a valid share).

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 5, 2019

Contributor

Could it be useful to specify the hold timeout in the onion payload, so that the sender picks this value themselves?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Aug 5, 2019

Author Collaborator

Yeah, we need to pick something, and it can't really be up to the sender or recipient since it effects the entire network. Implementations will pick something, so might as well specify it.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 12, 2019

Contributor

In addition to the seconds timeout, does something need to be specified in terms of block height? It could be that the first htlc comes in, meets the invoice cltv delta requirement, but its value isn't high enough to settle the invoice. The htlc is held. Then a block comes in. At that point, the cltv delta requirement isn't met anymore. Should the htlc be canceled?

This comment has been minimized.

Copy link
@ariard

ariard Sep 7, 2019

Contributor

Agree better to specify in terms of block height. If CLTV height is shared between all HTLCs, you should be able to wait until it minus some block height delta to land an unilateral closing.

Also with current wording it's not clear what receiving node should do after delay expiration, back to step upper ("SHOULD fulfill ...") or go to the one below ("MUST fail ...")


Because invoices do not necessarily specify an amount, and because
payers can add noise to the final amount, the total amount must be
sent explicitly. The requirements allow exceeding this slightly, as

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 27, 2019

Member

If we don't add a mandatory random identifier along the way, then we should discourage sending more than the invoice amount.

- otherwise:
- MUST add it to the HTLC set corresponding to that `payment_hash`.
- SHOULD fail the entire HTLC set if `total_msat` is not the same for
all HTLCs in the set.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 5, 2019

Contributor

Does this mean that an adversary can prevent an amp payment from completion by periodically sending a very small shard with a random total_msat, thereby cancelling any shards that have been acquired?

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 5, 2019

Contributor

I guess those htlcs are cancelled early in the process because their total_msat doesn't match what is in the invoice database. How would it work in that case for zero-valued invoices? First htlc that arrives sets the amount?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Aug 6, 2019

Author Collaborator

You're right, will change to ignore & fail if total_msat is different to avoid griefing.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 12, 2019

Contributor

We must make sure that an attacker can't cancel other shards, but also that they can't block the genuine shards from being accepted. If the attacker is the first to set the tracked total_msat to a random value, they could possibly block the payment for the timeout duration (and repeat this thereafter).

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 12, 2019

Contributor

In that light, the proposed randID may actually be required not only to prevent fee stealing with 0-valued invoices, but also to keep an attacker from interfering with the multi path settle process.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Aug 28, 2019

Collaborator

The attacker can generate a new sender-generated randID for this. But if the receiver would match on a receiver-generated randID, the attack wouldn't work.

This only applies to regular, non-mpp payments I think.

Indeed the receiver-generated randID limits payment to the anyone who has seen the invoice, versus anyone willing to probe with the payment hash. With sender-generated randID, I think the attack works for mpp as well?

the penultimate node can find out whether they are the penultimate node by crafting a new single hop route instead of forwarding as they should. If the newly crafted route succeeds, they know that the next node is the final destination.

IIUC the attack isn't limited to the penultimate node, just easier because they have less guess on the amount, right?

This is contradictory to being able to pay to settled invoices (to prevent leaking whether an invoice is paid or not). We could cancel all other sets, mark the invoice as settled, but if a new set comes in after that, we'd accept it again. Then we could just as well settle all sets that meet the requirements always.

Interesting, so using mpp you could silently learn the intended receiver without ever paying at all, and still successfully forward the payment by:

  • send partial mpp to receiver (not full amount)
  • if it gets held, you've found the receiver
  • wait 60s for htlc to be canceled
  • forward original htlc as normal

Having the receiver-generated randID will ensure that the receiver can cancel probers' htlcs immediately so they don't see the htlc being held (both before or after) the real payer completes their payment.

In the penultimate node attack-scenario above, I think they can also still do fee sniping on zero valued mmp invoices without a receiver-generated randID. The attacker crafts a new route with a new, lower totalAmt (single shard), and buys the preimage cheaply.

Indeed I think the attack works in general if we allow overpayment (without the receiver-generated randID), though zero-value invoices are the worst offenders since they permit the greatest amount of overpayment.

Okay, I'm convinced of the need for a receiver-generated randID :)

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 29, 2019

Contributor

IIUC the attack isn't limited to the penultimate node, just easier because they have less guess on the amount, right?

Yes, sounds like it. @t-bast was there anything specific in the second-to-last case?

This comment has been minimized.

Copy link
@t-bast

t-bast Aug 29, 2019

Collaborator

Correct, anyone in the route could probe like that, but it's a lot easier for the penultimate node: it only has to test the next node in the route (which it knows because it is in the onion).

If you are the second-to-last node, you need to have the intuition of which node to probe, which is a lot harder in practice (unless you already suspect who the recipient is).

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 29, 2019

Contributor

Ok, thanks for that clarification! What about calling it "invoice secret"? (is this a better term than 'receiver-generated randid`?)

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 29, 2019

Contributor

Actually, we started calling this payment_address which is more descriptive in the context of #658.


| Bits | Name |Description | Link |
|------|-------------|-----------------------|------------------------------------------------|
| 0/1 | `basic_mpp` | Payee understands M.P.P.| [BOLT #4](04-onion-routing.md#basic-multi-part-payments) |

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 5, 2019

Contributor

Will there also be a global feature bit that advertises mpp in the graph, so that spontaneous AMP payments can be made too?

I think that in that case the invoice flag is still required, because the payer may not have the (up to date) node features of the payee? Does this mean that basic_mpp implies var_onion_optin?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 17, 2019

Author Collaborator

Yes, and yes. I'll add that specifically.

@rustyrussell rustyrussell mentioned this pull request Aug 6, 2019
- MUST set `total_msat` to at least that `amount`, and less
than or equal to twice `amount`.
- MUST ensure that the total `amount_msat` of the HTLC set which arrive at the payee
is greater or equal to `total_msat`.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 12, 2019

Contributor

What is the rationale for allowing the total htlc set amount_msat to be greater than total_msat? And should this be capped too, to be consistent with single path payments? Otherwise the following would be possible (if I understood this correctly): invoice amt: 1000 sat, total_msat: 1000 sat, first htlc: 500 sat, second htlc: 1 btc?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Aug 28, 2019

Collaborator

I agree here that total_msat should be equal to total amount_msat of the intended htlc set. Even if the sender wishes to overpay, they should know exactly the amount by which they want to overpay. The relation then would be

invoice_amount <= total_msat == sum amount_msat

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 17, 2019

Author Collaborator

This fails in the case of independent payers (eg. bill splitting), and it also restricts fuzzing on re-attempts. You now have to decide the total fuzz up-front, and thus leak more information if you have to split a payment since it will always add to the previous amount exactly.

And note that overpaying is consistent with existing payments, requiring exact payment would not be.

- MUST ensure that the total `amount_msat` of the HTLC set which arrive at the payee
is greater or equal to `total_msat`.
- MUST ensure that no (incomplete) non-failed subset of the HTLC set adds
to a total `amount_msat` greater or equal to `total_msat`.

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 12, 2019

Contributor

What does this mean exactly? If the sender has enough htlcs in flight to pay the invoice but the htlcs are not settled, they shouldn't add another htlc?

This comment has been minimized.

Copy link
@t-bast

t-bast Aug 19, 2019

Collaborator

Exactly that's it. If you've sent an HTLC set that pays the total amount, you should wait for (some of) them to fail or settle. If you send another HTLC you will end up paying more than what you intended to pay.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Aug 28, 2019

Collaborator

As written I think this conflicts with the prior bullet. This says the sum can't be greater or equal, the prior says it must be greater or equal

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 17, 2019

Author Collaborator

No strict subset. This has been a source of confusion, though, so we need to reword it.


#### Requirements

The writer:

This comment has been minimized.

Copy link
@ariard

ariard Sep 7, 2019

Contributor

nit: Not introduced here but I found the writer/reader wording a bit weird outside bolt-11 shouldn't we stick with sending node/receiving node?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 17, 2019

Author Collaborator

These are routed through multiple nodes though, so it's best to be clear. Maybe we should change to be "origin node" instead of writer, but reader can't always be replaced with "final node"....?

This comment has been minimized.

Copy link
@ariard

ariard Sep 18, 2019

Contributor

IIRC, we used "processing node" in other parts of the spec to name intermediary nodes

- MAY include `basic_mpp` for the final node.
- if it does include `basic_mpp`:
- MAY send more than one HTLC to pay the invoice.
- MUST use the same `payment_preimage` on all HTLCs in the set.

This comment has been minimized.

Copy link
@ariard

ariard Sep 7, 2019

Contributor

Maybe we should detail all set HTLCs have same final_ctlv ? I mean the expiry field from invoice should scope all HTLC buddies but they will have different first_cltv due to different cltv_delta on their mutual paths.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 17, 2019

Author Collaborator

There's no reason I can think of to specify this though?

This comment has been minimized.

Copy link
@ariard

ariard Sep 18, 2019

Contributor

Having same final_ctlv encumbering every HTLC of the set should avoid one being expired and timeout while others being fulfilled due to misbehavior and so a partial payment.. But would require doing the same on whole multiple paths which is not realistic..

- if the total `amount_msat` of this HTLC set equals or exceeds `total_msat`:
- SHOULD fulfill all HTLCs in the HTLC set
- otherwise:
- SHOULD wait for at least 60 seconds for remaining members of the HTLC set.

This comment has been minimized.

Copy link
@ariard

ariard Sep 7, 2019

Contributor

Agree better to specify in terms of block height. If CLTV height is shared between all HTLCs, you should be able to wait until it minus some block height delta to land an unilateral closing.

Also with current wording it's not clear what receiving node should do after delay expiration, back to step upper ("SHOULD fulfill ...") or go to the one below ("MUST fail ...")


An implementation may choose not to fulfill an HTLC set which
otherwise meets the amount criterion (eg. some other failure, or
invoice timeout), however if it were to fulfill only some of them,

This comment has been minimized.

Copy link
@ariard

ariard Sep 7, 2019

Contributor

In the same way, if you try to pull some HTLCs of the set before all of them are locked on final_hop channels it may be a risk? Let's say if you pick up different paths and some nodes share the same operator, if you provide preimage for HTLC#1 but HTLC#3 isn't locked yet, preimage could be replay on HTLC#3 first links without payment being forwarded until payee. This point should be better underscored.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 17, 2019

Author Collaborator

Agree better to specify in terms of block height. If CLTV height is shared between all HTLCs, you should be able to wait until it minus some block height delta to land an unilateral closing.

No, you're tying up channels across the network. We're not going to hold things for many minutes before this becomes a spam attack. Certainly not waiting for blocks!

Also with current wording it's not clear what receiving node should do after delay expiration, back to step upper ("SHOULD fulfill ...") or go to the one below ("MUST fail ...")

Both apply. You SHOULD wait 60 seconds, you MUST have a some timeout.

In the same way, if you try to pull some HTLCs of the set before all of them are locked on final_hop channels it may be a risk? Let's say if you pick up different paths and some nodes share the same operator, if you provide preimage for HTLC#1 but HTLC#3 isn't locked yet, preimage could be replay on HTLC#3 first links without payment being forwarded until payee. This point should be better underscored.

Yes, we need to add "MUST NOT fulfill a set if the total is not sufficient".

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch 2 times, most recently from f4a845b to 71e846a Sep 17, 2019
04-onion-routing.md Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Show resolved Hide resolved
@@ -245,6 +245,9 @@ This is a more flexible format, which avoids the redundant `short_channel_id` fi
1. type: 2 (`amt_to_forward`)
2. data:
* [`tu64`:`amt_to_forward`]
1. type: 3 (`payment_secret`)

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 30, 2019

Author Collaborator

This should be even, as we agreed to it in advance.

@t-bast t-bast mentioned this pull request Oct 1, 2019
3 of 4 tasks complete
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Oct 3, 2019

OK, I applied proposed changes as a series of separate fixups for easy review.

I'll squash them together at the end once we're all happy.

Copy link
Collaborator

t-bast left a comment

LGTM, I'll update my branch with support for the mpp_timeout error and will comment further if that makes me think of additional information to include in there.

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Nov 4, 2019

OK, as agreed at meeting, I used standard feature bits for BOLT11 features. That means an explicit "basic_mpp" feature bit is required, though I overloaded the "var_onion_optin" feature a little: if it's even you have to use a secret.

Copy link
Collaborator

cfromknecht left a comment

Completed another pass, most of the requirements on the sender/receiver side look pretty far along!

For me the primary question now is whether payment_secret should be gated on basic_mpp instead of overloading var_onion_optin. I'm not sure we need a feature bit just to signal whether the receiver supports reassembling multiple htlcs (see inline comment), and if that's the case the whole proposal only needs one feature bit: basic_mpp signaling optional/required payment_data record.

I ran into some of these issues when implementing lightningnetwork/lnd#3679, which for now will just include the payment secret if it's there, tho it would be a lot cleaner if it had an associated feature bit.

Getting closer 🤓

04-onion-routing.md Show resolved Hide resolved
04-onion-routing.md Show resolved Hide resolved
- MUST include `amt_to_forward` and `outgoing_cltv_value` for every node.
- MUST include `short_channel_id` for every non-final node.
- MUST NOT include `short_channel_id` for the final node.
- Unless `node_announcement` contains `var_onion_optin` or the [BOLT #11](11-payment-encoding.md#tagged-fields) invoice contains an `s` field:

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Nov 6, 2019

Collaborator

shouldn't this be: Unless node_announcement or BOLT 11 invoice contains var_onion_option? A node could support TLV w/o supporting this proposal.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Nov 12, 2019

Author Collaborator

Sure, but you can't add payment_data unless you have the payment_atgnwt!

04-onion-routing.md Show resolved Hide resolved
@@ -27,6 +27,7 @@ These flags may only be used in the `init` message:
| 4/5 | `option_upfront_shutdown_script` | Commits to a shutdown scriptpubkey when opening channel | [BOLT #2][bolt02-open] |
| 6/7 | `gossip_queries` | More sophisticated gossip control | [BOLT #7][bolt07-query] |
| 10/11 | `gossip_queries_ex` | Gossip queries can include additional information | [BOLT #7][bolt07-query] |
| 12/13 | `basic_mpp` | Node can receive basic multi-part payments | IN9 | [BOLT #3][bolt04-mpp] |

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Nov 6, 2019

Collaborator

i've been looking at this and wondering, what does it meant to require a multi-path payment. if the sender can get there in one shard, why would the receiver require them to use multiple paths? even then, the granularity would be 1 htlc or >1 htlc, which doesn't seem very useful.

i'm starting to think we don't really need a bit to indicate whether we can or cannot assemble multiple htlcs, and that this could just indicate optional/required for sending the payment_data record. most of complexity in mpp is on the sender side anyway, so idk how much we would be saving people by allowing them to choose between 1 and >1 on the receiver-side.

12/13 is already option_static_remotekey

This comment has been minimized.

Copy link
@t-bast

t-bast Nov 8, 2019

Collaborator

I have some additional thoughts too. The reason we initially split the payment_secret and multi_part feature bits was to allow light payment receivers to require a payment_secret while not supporting multi-part. I still think this is a valid scenario and I think we can't achieve it now that there's no feature bit for payment_secret (unless we use var_onion_optin to implicitly do that as suggested, but I'm not a huge fan).

Receiving multi-part payments means you have to keep some state (partial HTLC set received) until either the payment succeeds or times out. It's hard to guess what other people will want to do in the future, but I can imagine a tiny payment processor in a vending machine that wouldn't want this overhead and would only support single-part payment, while enforcing the payment_secret.

I think our feature bit set should allow that kind of tuning, so I think it would be better to have a pair of feature bits for multi-part (14/15, even though the even one wouldn't make much sense) and a pair of feature bits for payment secret (16/17). This way a very constrained payment processor chip can set 16 without setting 14 nor 15.

Do you think this is overkill?

This comment has been minimized.

Copy link
@joostjager

joostjager Nov 11, 2019

Contributor

I think if you look at the total processing power that is required for a Lightning node, the tracking of a multi-path set is not significant. I'd rather add such specialized feature bit only when that "light receiver" actually presents itself.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Nov 11, 2019

Author Collaborator

If you don't have incoming capacity in a single channel, you might indicate you NEED MPP, maybe?

I think keeping the features separate is cleaner.

11-payment-encoding.md Show resolved Hide resolved
flattened features is merged!].

Note that the `var_onion_optin` feature is slightly overloaded to indicate
support for the `s` field: making this compulsory prevents probing attacks

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Nov 6, 2019

Collaborator

would be in favor of not overloading var_onion_optin with the s field. there's a huge gap between adding TLV onion and actually being able to implement this proposal, perhaps they should be separate?

This comment has been minimized.

Copy link
@t-bast

t-bast Nov 8, 2019

Collaborator

There's a huge gap between implementing var_onion_optin and implementing MPP. But implementing support for payment secrets is really simple, right?
So making var_onion_optin compulsory to imply that you need to send the payment secret (potentially for existing single-part payments) seems acceptable, even though I'd prefer a dedicated feature bit to make it more explicit (but at the cost of more feature bits in the system...).

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Nov 12, 2019

Sorry for missing yesterday's meeting, here are my updated comments on the points raised yesterday:

  • naming: payment_nonce was my first choice, but since n was already assigned in Bolt 11 I changed to payment_secret (which is somewhat misleading because it's not entirely secret), but that can be named payment_nonce and use something else than n in the invoice. Other options: payment_id, invoice_id?
  • splitting in two features instead of overloading var_onion_optin (payment_thing and mpp): thanks, I really wanted that separation to happen ;)

I'll get these latest changes implemented in Eclair today, will update here if anything looks weird.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Nov 12, 2019

One question about the encoding of payment_data (specifically the tu64 part).
If encoding a payment secret 0x1111111111111111111111111111111111111111111111111111111111111111 and a total_msat of 1105 (0x0451), is the result:

0x08 22 1111111111111111111111111111111111111111111111111111111111111111 0451

or

0x08 23 1111111111111111111111111111111111111111111111111111111111111111 020451

(ie is tu64 explicitly prefixed by its length or implicitly by the length of the whole TLV record?)

I prefer the second option because it's more composable (you can have multiple tu64 in the same TLV record) whereas the first one feels hackish (especially to only save 1 byte).

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Nov 13, 2019

@t-bast currently as specified it’s the former, so the length is implied by the size of the record. indeed we could have a separate prefix-truncated uint (ptu64?) encoding as you suggest when multiple variable length values are being encoded. Note that for the current application ptu64 would only save one byte over just making a whole new record, so it’s not necessary in this case but could be useful in the future

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Nov 14, 2019

Sorry for missing yesterday's meeting, here are my updated comments on the points raised yesterday:

* naming: `payment_nonce` was my first choice, but since `n` was already assigned in Bolt 11 I changed to `payment_secret` (which is somewhat misleading because it's not entirely secret), but that can be named `payment_nonce` and use something else than `n` in the invoice. Other options: `payment_id`, `invoice_id`?

payment_handle?

* splitting in two features instead of overloading `var_onion_optin` (`payment_thing` and `mpp`): thanks, I really wanted that separation to happen ;)

Yep, done.

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Nov 14, 2019

One question about the encoding of payment_data (specifically the tu64 part).
If encoding a payment secret 0x1111111111111111111111111111111111111111111111111111111111111111 and a total_msat of 1105 (0x0451), is the result:

0x08 22 1111111111111111111111111111111111111111111111111111111111111111 0451

or

0x08 23 1111111111111111111111111111111111111111111111111111111111111111 020451

(ie is tu64 explicitly prefixed by its length or implicitly by the length of the whole TLV record?)

I prefer the second option because it's more composable (you can have multiple tu64 in the same TLV record) whereas the first one feels hackish (especially to only save 1 byte).

tu64 is defined as the former. Your argument extends to "just add a separate tlv", since this only saves one byte :) Turns out a single value is a common case, worth optimizing IMHO.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Nov 14, 2019

tu64 is defined as the former. Your argument extends to "just add a separate tlv", since this only saves one byte :) Turns out a single value is a common case, worth optimizing IMHO.

Got it, I feel it's a bit overkill that we optimize this for at most a one byte gain per TLV record against more edge cases to handle in the encoder, but if you both feel it's worth it I'll adapt ;)

Features are listed in [09-features.md](BOLT #9) [FIXME: Or will be after
flattened features is merged!].

Note that the `payment_secret` feature probing attacks from nodes

This comment has been minimized.

Copy link
@t-bast

t-bast Nov 14, 2019

Collaborator
Suggested change
Note that the `payment_secret` feature probing attacks from nodes
Note that the `payment_secret` feature prevents probing attacks from nodes

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 13, 2019

Collaborator

still unresolved?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Dec 13, 2019

Author Collaborator

Ah, this was fixed in #712 which reworks this paragraph anyway. Let me roll it in here, then I can rebase #712

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Nov 20, 2019

tu64 is defined as the former. Your argument extends to "just add a separate tlv", since this only saves one byte :) Turns out a single value is a common case, worth optimizing IMHO.

Got it, I feel it's a bit overkill that we optimize this for at most a one byte gain per TLV record against more edge cases to handle in the encoder, but if you both feel it's worth it I'll adapt ;)

In particular, it gives us back the bytes in the onion which TLV costs us, making the trade-off more palatable.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We also define what the basic_mpp feature means in an invoice, by
reference to the next commit.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch from 3a09bc5 to 9441a66 Nov 26, 2019
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Nov 26, 2019

Trivial squash and rebase on master.

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch from 9441a66 to 8a28e0f Dec 2, 2019
04-onion-routing.md Show resolved Hide resolved
scenarios in which the senders are genuinely independent (friends
splitting a bill, for example).

The restriction on sending an HTLC once the set is over the agreed total prevents the preimage being released before all

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 6, 2019

Collaborator

nit: long line

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Dec 13, 2019

OK, @t-bast has completed interoperability testing between Eclair and c-lightning (successfully, though he discovered some UX suckage as we only allow making such payment manually). As a result, the PR in c-lightning is now merged into master (ElementsProject/lightning#3335).

Normally, this would mean I would now merge this PR into the spec, however I think we should delay pending @cfromknecht since there are outstanding questions?

Copy link
Collaborator

cfromknecht left a comment

LGTM! 🥂

Features are listed in [09-features.md](BOLT #9) [FIXME: Or will be after
flattened features is merged!].

Note that the `payment_secret` feature probing attacks from nodes

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 13, 2019

Collaborator

still unresolved?

09-features.md Show resolved Hide resolved
This also defines the TLV format for payment_secret; the two are intertwined.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/base-amp branch from 8a28e0f to f8e9c29 Dec 13, 2019
@rustyrussell rustyrussell dismissed Roasbeef’s stale review Dec 13, 2019

Resolved at meeting, acked by @cfromknecht

@rustyrussell rustyrussell merged commit 4c3d016 into lightningnetwork:master Dec 13, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

rustyrussell commented Dec 13, 2019

Merged after successful @t-bast Eclair <-> clightning interop test. 🍰 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.