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

Reject received payments where PaymentRequest Expiry Tag time has been exceeded. #749

Merged
merged 5 commits into from
Nov 15, 2018

Conversation

n1bor
Copy link
Contributor

@n1bor n1bor commented Oct 27, 2018

Fixes #748

sstone
sstone previously requested changes Nov 2, 2018
Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

Nice! And choosing to return an unknown payment hash is consistent with our current behaviour. Just a few minor formatting changes/typos to fix and we're good. Thanks!

Expiry Tag time has been exceeded.
Fixes ACINQ#748
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

How about implementing this like so (loosely inspired by how guava's cleanes stale cache entries), that would result in a single path to UnknownPaymentHash:

  1. Put the following code in a separate purgeExpiredRequests method:
hash2preimage.collect {
        case e@(_, (_, pr)) if pr.expiry.isEmpty => e // requests that don't expire are kept forever
        case e@(_, (_, pr)) if pr.timestamp + pr.expiry.get > currentSeconds => e // clean up expired requests
})
  1. Create a PurgeExpiredRequests object to make the intent obvious:
case PurgeExpiredRequests =>
      context.become(run(purgeExpiredRequests(hash2preimage)))
  1. In the handler for UpdateAddHtlc, use the newly created method:
purgeExpiredRequests(hash2preimage.get(htlc.paymentHash)) match {

@n1bor
Copy link
Contributor Author

n1bor commented Nov 14, 2018

Not sure this is any better/clearer. I think I have done what you suggested.
I just wanted to avoid making it collect over all pending requests for every payment as that would scale very badly if getting 100's payments a minute/second...

@pm47
Copy link
Member

pm47 commented Nov 15, 2018

I took the liberty to commit a new version that addresses your concerns. WDYT?

pm47
pm47 previously approved these changes Nov 15, 2018
@pm47 pm47 dismissed sstone’s stale review November 15, 2018 12:56

requested changes have been made, format is now correct

@pm47 pm47 merged commit 738b4fa into ACINQ:master Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants