channeldb+invoices: fix what pending invoice means in ChannelDB#3838
channeldb+invoices: fix what pending invoice means in ChannelDB#3838joostjager merged 2 commits intolightningnetwork:masterfrom
Conversation
bhandras
changed the title
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Straight-forward change. But it is a breaking rpc change. How to deal with that?
To be fair #3694 already did this but this PR completes it. |
How did it do that? It auto-canceled invoices, but it didn't change which invoices where returned when listing? |
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
How did it do that? It auto-canceled invoices, but it didn't change which invoices where returned when listing?
You're correct, this change does indeed modify how pending_only flag works.
Looking at old behavior, pending meant: one of {accepted, open, canceled}; now pending means: one of {accepted, open}. When listing all invoices, behavior remains the same. With previous behavior users who were curious about actual pending (to be paid) invoices had to filter by hand. We can keep compatibility by adding a new flag, but imo it's more confusing.
That is what we generally do. And mark the old flag as deprecated in at least one major release. |
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Proto file also needs to be updated for new meaning of this boolean.
halseth
left a comment
halseth
left a comment
There was a problem hiding this comment.
I think it is okay to slightly change the RPC behavior. You can argue that the current behavior is a bug.
Thanks for pointing this out. Updated. |
bhandras
added
bug fix
code health
channeldb/invoice_test.go
Outdated
There was a problem hiding this comment.
Doesn't this reduce the effect of this test, because we rely on the code under test to provides us with the expectations?
There was a problem hiding this comment.
I'd argue that it actually makes the test more strict as before we relied on FetchAllInvoices too. Ideally expected results should be constant and possbily included in the test itself rather then generated/selected etc.
There was a problem hiding this comment.
The removal of FetchAllInvoices is good, but it is now replaced by something that uses the code under test to generate the expectations. This test is testing whether the filtering is correct. I'd say we don't need to do the deep equal on the exact invoice contents for that. It is tested elsewhere already. For me, building a list of payment hashes here and asserting on it in the various filter cases is enough.
There was a problem hiding this comment.
I'm not convinced that this is not a test relaxation (compared to what it was before). Maybe it's more correct this way though. PTAL.
There was a problem hiding this comment.
Isn't the updatedInvoice still used as an expectation now?
There was a problem hiding this comment.
Oh, it is changed in a later commit. Generally we don't want to touch the same code in one pr twice. Switching them around as you had before lowers the summed delta of all commits.
There was a problem hiding this comment.
Sorry, that was a rebase mistake on my end. Corrected.
bhandras
force-pushed
the
i1404
branch
2 times, most recently
from
8ee9c05 to
fa44f40
Compare
bhandras
force-pushed
the
i1404
branch
5 times, most recently
from
4424e32 to
557108f
Compare
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Commit order seems to be not optimal in this revision?
Yeah, forgot to tamper with dates when rebasing interactively. It was in good order just GitHub orders by date. Fixed now. |
channeldb/invoice_test.go
Outdated
There was a problem hiding this comment.
Still using updatedInvoice for expectation?
bhandras
force-pushed
the
i1404
branch
4 times, most recently
from
9df7188 to
d523d9e
Compare
|
After much discussion about what to include in the PR and how to change tests @joostjager and I decided to go back in time somewhat and just revert to the version where we keep |
This commit removes random invoice state selection when testing FetchAllInvoicesWithPaymentHash.
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.
3 participants
This pull request is a partial fix for issue #1404 where part of the problem comes from
not being able to filter out expired invoices due to those not being canceled
(fixed in PR #3694) while also having a misleading definition of what pending invoice
state means.
The PR changes the meaning of the
pending_onlyflag inlistinvoicesRPC to list invoices that are either in
ContractOpenorContractAcceptedstate. Previously when the
pending_onlyflag was set thelistinvoicesRPC returned all invoices that are not settled which equals to invoices in
ContractOpen,ContractAcceptedorContractCanceledstate.