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

BOLT 4, 9, 11: Base Atomic Multipath Payments. #511

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants

@ZmnSCPxj ZmnSCPxj force-pushed the ZmnSCPxj:base-amp branch from 79abe45 to 1dcb769 Nov 14, 2018

@renepickhardt

As mentioned I suggest minor changes to increase the readability. But I can basically ACK these changes.

The same `payment_hash` is used in all paths.
The final node hop data of each path
MUST have the `incomplete_payment` flag set,
and MUST have the same `amt_to_forward` for all paths.

This comment has been minimized.

@renepickhardt

renepickhardt Nov 16, 2018

Below we don't specify what the receiver should do if they receive final htlcs for the the payment hash but different amt_to_forward. Maybe send error messages back?
This actuallyseems to be a problem because with this I can easily spam htlcs to all channels wich will never be fulfilled (also by making an incomplete base AMP) creating a dos attack

This comment has been minimized.

@renepickhardt

renepickhardt Nov 16, 2018

Never mind saw that you put it to the requirements in line 9xx. Still it is not clear to me how to mitigate dos attacks (but I guess that did attack can also exist without base AMP)

@@ -791,6 +839,16 @@ The channel from the processing node has been disabled.
The CLTV expiry in the HTLC is too far in the future.
1. type: BADONION|PERM|22 (`invalid_flag`)
An even-numbered bit of `flags0` is set, which is not understood

This comment has been minimized.

@renepickhardt

renepickhardt Nov 16, 2018

Why do we need that?

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 16, 2018

Collaborator

In keeping, with "It's OK to be odd" rule, which has the corollary "It's BAD to be even".

This comment has been minimized.

@rustyrussell

rustyrussell Nov 22, 2018

Collaborator

BADONION has a specific meaning here, which means "I can't even decode it, so you're getting a message from the prevoius node". This is weaker, so should not have that bit set.

- if `incomplete_payment` is set:
- MUST use `amt_to_forward` to evaluate for
`incorrect_payment_amount` errors.
- SHOULD suspend incoming HTLCs:

This comment has been minimized.

@renepickhardt

renepickhardt Nov 16, 2018

Maybe change to : SHOULD suspend incoming HTLCs with the same payment hash:

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 16, 2018

Collaborator

My mental model is a single-threaded server, so approximately, you would write code as below:

  1. If the incomplete_payment is set, suspend incoming HTLC (also set a timeout trigger to fail suspended HTLCs due to taking too long)
  2. Scan through all suspended HTLCs and sum up their total.
  3. If the total in all suspended HTLCs >= amt_to_forward, then claim them.

Although the changed wording seems fine to me, either way.

payment that is less than its advertised payment amount.
We assume that no final node would want to lose money.
We assume here that the `payment_preimage` is of value.

This comment has been minimized.

@renepickhardt

renepickhardt Nov 16, 2018

I would leave out this 3rd Paragraph of the 4 added rational paragraphs. I think the other 3 work without this one

`payment_hash`:
- MUST fail the HTLC.
- MUST return an `incorrect_payment_amount` error.
- if the total `incoming_htlc_amt` of all suspended HTLCs and

This comment has been minimized.

@renepickhardt

renepickhardt Nov 16, 2018

It is confusing. We are at the part where the amt_to_forward is different. Now you give an rational when to succeed the htlcs.

I would separate this somehow

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 16, 2018

Collaborator

They are on the same level, so my mental model is that these rules are evaluated in that order, and this rule is not gated by the amt_to_forward rule.

@@ -257,6 +271,40 @@ compare its values against those of the HTLC. See the
If not for the above, since it need not forward payments, the final node could
simply discard its payload.
### Base Atomic Multipath Payments

This comment has been minimized.

@rustyrussell

rustyrussell Nov 22, 2018

Collaborator

Let's drop this phrase altogether. "Partial Payments" is better.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 25, 2018

Collaborator

Atomic Multipath Payments is how the users know this feature, so I am hesitant to change it.

This comment has been minimized.

@Roasbeef

Roasbeef Nov 26, 2018

Member

Which users now this as AMP? When the majority of those that are aware of LN, here "AMP", they think of the original ML post. The only individuals that know of this partial payment feature are those that were at the summit.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 27, 2018

Collaborator

Okay, I shall change it then.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 27, 2018

Collaborator

"ZKCP-preseerving proper multipath payments"

The `amt_to_forward` value will be the actual total amount that the final node
should expect.
Thus, in a multipath payment, the incoming HTLC `incoming_htlc_amt` will be lower

This comment has been minimized.

@rustyrussell

rustyrussell Nov 22, 2018

Collaborator

I don't think we should do this at all. Just let the recipient send a receipt when they have >= expected amount.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 25, 2018

Collaborator

I have come to agree.

The same `payment_hash` is used in all paths.
The final node hop data of each path
MUST have the `incomplete_payment` flag set,
and MUST have the same `amt_to_forward` for all paths.

This comment has been minimized.

@rustyrussell

rustyrussell Nov 22, 2018

Collaborator

Let's rework this to be clear:

The payer:
  - if `wait_on_incomplete` is not set in the invoice:
    - MUST NOT set the `incomplete_payment` flag.
  - otherwise:
    - If they split the payment into multiple HTLCs:
     - MUST use the same `payment_preimage` on all HTLCs.
     - MUST set the `incomplete_payment` flag on all HTLC onions.
     - MUST ensure that the total amount of all the HTLCs at the payee meets the `amount` requirement of the invoice.
     - SHOULD send all payments at approximately the same time.
     - SHOULD try to use diverse paths to the recipient for each HTLC.
     - SHOULD retry and/or re-divide HTLCs which fail.
     - MUST NOT allow the total value of active HTLCs to exceed the amount it is prepared to pay.

The payee:
  - if an onion with `incomplete_payment` is set for an invoice which flagged `wait_on_incomplete`:
    - MUST not immediately fail the HTLC if it is valid except for the amount being too small.
    - SHOULD wait for at least 60 seconds for more partial payments for the same `payment_preimage`.
    - MUST accept all partial payments once the total amount meets or exceeds the invoice `amount`.
    - MUST reply with `incomplete_payment_too_slow` to all partial payments which it times out.
@@ -791,6 +839,16 @@ The channel from the processing node has been disabled.
The CLTV expiry in the HTLC is too far in the future.
1. type: BADONION|PERM|22 (`invalid_flag`)
An even-numbered bit of `flags0` is set, which is not understood

This comment has been minimized.

@rustyrussell

rustyrussell Nov 22, 2018

Collaborator

BADONION has a specific meaning here, which means "I can't even decode it, so you're getting a message from the prevoius node". This is weaker, so should not have that bit set.

@ZmnSCPxj ZmnSCPxj force-pushed the ZmnSCPxj:base-amp branch from 9842221 to 0c9ba2f Nov 25, 2018

@rustyrussell rustyrussell added this to the v1.1 milestone Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment