Invoices that are paid multiple times do not reflect their full amount paid

[alexbosworth]

Background

When accounting for received payments, invoices should reflect the full amount received, even in the case of a non-standard payment where a payment hash is reused across payments.

Your environment

• lnd "0.5.0-beta commit=v0.5-beta-330-g881ed0870930e9fc2833c3bebd042b1453c56e06"
• MacOS Mojave
• Bitcoin Core

Steps to reproduce

1. Create an invoice
2. Pay the invoice on one node
3. Pay the invoice on another node
4. Lookup the invoice to check on amt_paid_msat

Expected behaviour

amt_paid_msat = invoice amount x 2

Actual behaviour

amt_paid_msat = invoice amount x 1

[alexbosworth]

Note: I appreciate that at invoice can be paid multiple times if the receiver wishes to allow this potentially unsafe behavior, however I also think that by default the node should not allow receiving a second payment to an already resolved payment hash

[mlerner]

I can pick this one up.

[Roasbeef]

@mlerner sounds good to me!

[mlerner]

@alexbosworth: I confirmed that if the same node tries to send payment for an invoice more than once, the later payments fail. Additionally, I checked that multiple nodes paying the same invoice aren't accounted for in the receiving node's database, and the payments succeed.

Should controlling whether a receiver will accept payment for a resolved invoice be configurable (so that a receiving node can turn on the ability to receive multiple payments for the same invoice), or should receiving nodes always fail the payments for a resolved invoice?

FWIW, my reading of Eclair's code is that multiple payments for the same payment hash will fail because the payment hash is used as a primary key in their database: https://github.com/ACINQ/eclair/blob/71e50520ec8627cbcb5cc3f7d669b9274aa5a3bd/eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala#L45

[alexbosworth]

In this issue I would say that receiving multiple times should be reflected in the accounting of an invoice

More broadly I would say that the receiver should be able to choose whether or not they re-resolve preimages

[cfromknecht]

The reason we accept multiple payments atm is due to possible ambiguity of restart behavior. We currently don't record the CircuitKey (ChanID, HtlcID) which ultimately settled the invoice, so in the presence of processing multiple HTLCs that could settle an invoice, we don't know which one to reject and which one to accept when replaying the events.

In order to properly do the accounting proposed here, an invoice will have to keep track of these circuit keys. Otherwise, we also risk double counting the same HTLC after a restart.

However, I'd say that once we've gone through the trouble of properly tracking which CircuitKey settled an invoice, the proper fix is then to reject all others and not accept payment to the same invoice.

TL;DR: properly doing this accounting requires fixing the restart ambiguity, which means can safely reject duplicates and not have to actually modify the accounting.

[cfromknecht]

Just remembered there are some additional privacy arguments that could be made in favor of accepting duplicate payments to the same invoice, so there may still be some desire to accept duplicates. With a generic enough solution to the ambiguity discussed above, this would be fairly straightforward.

[heuristics222]

If you decide to allow multiple payments of the same invoice, subscribers using SubscribeInvoices should be notified of the subsequent payments, currently they are not.

[sergioabril]

if amt_paid_msat is going to show the total amount paid (eg. amount x 2), would be possible to add a new field to the invoice, holding an array of payments (like a payment_log), maybe just with the amount, timestamp and node, so you can get more details of what happened to that invoice when is paid more than once?

[rahil471]

My suggestion would be to not allow and payment to an invoice that is already settled. But we know who doesn't like some extra money.. :\

I'm coming from an angle of Lightning integration in an exchange/wallet provider.
The only way to know of an incoming invoice is via SubscribeInvoices and check the amt_paid_sat field. Can't depend on value because in case of 0 any-amount invoice the value stays 0.

Now if we send multiple settled events for same rhash I wonder how this can be handled in wallet provides, probably have to compromise the immutability of the records.
Also, how would the settled_index be handled? Because we consider invoice settled once total value is paid off.

Not sure if I'm making sense to you guys, but we can take this conversation on slack if needed.

[joostjager]

Fix coming up in #3390