Reject payments for expired invoices

[araspitzu]

Ensure that we don't accept incoming HTLCs for expired invoices, fixes #1055.

[codecov-io]

Codecov Report

Merging #1057 into master will increase coverage by 1.72%.
The diff coverage is 80%.

@@            Coverage Diff            @@
##           master   #1057      +/-   ##
=========================================
+ Coverage   81.97%   83.7%   +1.72%     
=========================================
  Files          97     100       +3     
  Lines        7489    8836    +1347     
  Branches      299     341      +42     
=========================================
+ Hits         6139    7396    +1257     
- Misses       1350    1440      +90

Impacted Files	Coverage Δ
...src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala	100% <ø> (ø)	⬆️
.../fr/acinq/eclair/payment/LocalPaymentHandler.scala	100% <100%> (ø)	⬆️
...scala/fr/acinq/eclair/payment/PaymentRequest.scala	91.5% <66.66%> (-0.5%)	⬇️
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala	92.42% <0%> (-1.46%)	⬇️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	96.82% <0%> (-0.24%)	⬇️
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala	100% <0%> (ø)	⬆️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala	100% <0%> (ø)	⬆️
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala	100% <0%> (ø)	⬆️
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala	100% <0%> (ø)	⬆️
...src/main/scala/fr/acinq/eclair/wire/TlvTypes.scala	100% <0%> (ø)
... and 19 more

[pm47]

That's better, but still not compliant because BOLT 11 sets a default one-hour expiry:

x (6): data_length variable. expiry time in seconds (big-endian). Default is 3600 (1 hour) if not specified.

[pm47]

That's better, but still not compliant because BOLT 11 sets a default one-hour expiry

This is in fact mitigated by (h/t @araspitzu):

eclair/eclair-core/src/main/scala/fr/acinq/eclair/payment/LocalPaymentHandler.scala

Line 50 in 32145e8

val expirySeconds = expirySeconds_opt.getOrElse(nodeParams.paymentRequestExpiry.toSeconds)

[araspitzu]

@sstone i see the point of not introducing complexity at the db layer but the rationale behind it was that we can have a fast lookup for expired invoices. The case of getPendingPaymentRequestAndPreimage is a bit particular though because it is invoked when we receive an update_add_htlc and we lookup the invoice by payment hash so the expire_at filter here is a bit redundant, also there is some added value (in code readibility) in doing the check in the payment handler. Would it be okay for you to move the check up to the payment handler but leave the expire_at in the db for the other calls?

[sstone]

It's not really complexity that worries me, it's rather having a "get data" method that returns different results even if the database you're reading from has not changed. Having expire_at in the db is fine, but checking it there does not feel right, it should be next to the other checks in our payment handler (another option which I like less would be to explicitly pass a current time parameter to the method instead of using Platform.currentTime)

[araspitzu]

Right, but then if we want to remove non-determinism from these db calls I see only two options:

• Get rid of expire_at and do all the checks/filters in an upper layer
• Keep expire_at and have the current time passed as a parameter to the call

I propose doing option #2 but as a special case for getPendingPaymentRequestAndPreimage we don't pass the current time to the db call instead we explicitly check the expiry in the payment handler. Other calls that use expire_at, namely listPaymentRequests and listPendingPaymentRequests can have the current time as function parameter.

[sstone]

Ok I propose that for this PR we just fix getPendingPaymentRequestAndPreimage and check the expiry in payment handler.

[sstone]

I don't think that comment is true, it will return all payment requests included the ones that have been paid

[araspitzu]

In SqlitePaymentDb#L158 we filter for received_at IS NULL so this retrieves only non paid payment requests, note that we update the record setting received_at when receiving a payment.

[sstone]

Ah ok my bad.... I think we're good to go!