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

invoices+channeldb: reject payments to expired invoices #3694

Merged
merged 5 commits into from Dec 14, 2019

Conversation

@bhandras
Copy link
Collaborator

bhandras commented Nov 8, 2019

invoices+channeldb: reject payments to expired invoices


This change is Reviewable

@bhandras bhandras requested review from Roasbeef and joostjager Nov 8, 2019
Copy link
Member

Roasbeef left a comment

Solid fix! Nice to finally patch this behavior. No major comments on my end, just mostly relevant areas of the contribution/style guide that aren't fully adhere to.

An example of a better PR title would've been something along the lines of:

invoices+channeldb: reject payments to expired invoices 
channeldb/invoices.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
@bhandras bhandras changed the title Fix for issue #3448 invoices+channeldb: reject payments to expired invoices Nov 10, 2019
Copy link
Collaborator

joostjager left a comment

Perhaps not in this PR, but eventually we also may want to actively cancel invoices when they expire. So that there state is reflectly properly even without someone paying to them first.

invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
@bhandras bhandras force-pushed the bhandras:i3448 branch from 4ed4eb5 to 6382da1 Nov 11, 2019
@halseth halseth removed request for cfromknecht, halseth and wpaulino Nov 11, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Nov 12, 2019

Working on a different timer in #3415 using a priority queue

@alexbosworth

This comment has been minimized.

Copy link
Contributor

alexbosworth commented Nov 12, 2019

Can we change the default invoice time to be higher than an hour? How about 3 hours? Otherwise I anticipate this change resulting in some error rate increases.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Nov 13, 2019

Why would error rates increase? The sender already checks the expiry now. Or are you thinking about other implementations that may not do this? Or people decoding the pay req and using SendPayment?

@bhandras bhandras force-pushed the bhandras:i3448 branch from 6382da1 to ebed600 Nov 13, 2019
@bhandras bhandras requested review from joostjager and Roasbeef Nov 13, 2019
@alexbosworth

This comment has been minimized.

Copy link
Contributor

alexbosworth commented Nov 13, 2019

Why would error rates increase? The sender already checks the expiry now. Or are you thinking about other implementations that may not do this? Or people decoding the pay req and using SendPayment?

There is decoding and sending yes, but there is also a natural race condition between acceptance and sending. It can easily take me 10 minutes to complete a payment to some destinations, but the timer would have shown "you have 10 minutes left" when I pressed the button. Clocks may not be perfectly synchronized, etc.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Nov 14, 2019

What does changing the default fix about this? The problem remains, only with a different time.

@alexbosworth

This comment has been minimized.

Copy link
Contributor

alexbosworth commented Nov 14, 2019

I didn't say it fixed it, it just can counterbalance it

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Nov 15, 2019

Perhaps not in this PR, but eventually we also may want to actively cancel invoices when they expire. So that there state is reflectly properly even without someone paying to them first.

I would prefer this being a follow up. Can also make an issue on it for someone to pick up.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Nov 15, 2019

I would prefer this being a follow up. Can also make an issue on it for someone to pick up.

@bhandras and I discussed this offline. Both approaches aren't compatible with each other, so doesn't work very well as a follow up.

@bhandras

This comment has been minimized.

Copy link
Collaborator Author

bhandras commented Nov 15, 2019

Even though the original issue is solved with the current PR, as discussed with @joostjager, it is cleaner to implement a watcher for invoice expiry and cancellation. I'll update this PR soon.

@alexbosworth

This comment has been minimized.

Copy link
Contributor

alexbosworth commented Nov 17, 2019

Here is a data point on how many errors are due to expiry

  • 708 - payment not completed before timeout
  • 603 - invoice already paid
  • 392 - invoice expired
  • 146 - payment is in transition
  • 72 - unable to find a path

https://twitter.com/pwkad/status/1196124533759062019

@bhandras bhandras force-pushed the bhandras:i3448 branch from 6194b04 to 8b79124 Nov 20, 2019
This commit changes how FetchAllInvoicesWithPaymentHash behaves
when the DB is empty and also adds a unit test to test that
case as well as normal expected behavior.
@bhandras bhandras force-pushed the bhandras:i3448 branch from 8cbdd42 to 0758b93 Dec 13, 2019
@bhandras bhandras requested a review from joostjager Dec 13, 2019
v0.9.0-beta automation moved this from Needs Review to Approved Dec 14, 2019
Copy link
Member

Roasbeef left a comment

LGTM 🐌

@Roasbeef Roasbeef merged commit e34bc3d into lightningnetwork:master Dec 14, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 62.182%
Details
v0.9.0-beta automation moved this from Approved to Done Dec 14, 2019
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 14, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 16, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 16, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 16, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 18, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 18, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 18, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 18, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 18, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.

This commit also updates the doc for ListInvoiceRequest pending_only
flag to update the meaning such that pending_only means invoices that
are not settled or invoices that are not canceled.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
bhandras added a commit to bhandras/lnd that referenced this pull request Dec 19, 2019
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
matheusd added a commit to matheusd/dcrlnd that referenced this pull request Jan 9, 2020
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
matheusd added a commit to matheusd/dcrlnd that referenced this pull request Jan 22, 2020
This commits builds on top of PR lightningnetwork#3694 to further clarify invoice
state by defining pending invoices as the ones which are not
settled or canceled. Automatic cancellation of expired invoices
makes this possbile. While this change only directly affects
ChannelDB, users of the listinvoices RPC will receive actual
pending invoices when pending_only flag is set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.