-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fixes the total num of payments returned by QueryPayments when there are dates restrictions #8883
base: master
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Please check the contribution guidelines regarding Ideal Git Commit Structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved CI run.
44987f4
to
07bf0a1
Compare
@MPins I don't think your fix solves the issue. Using Lines 4178 to 4184 in a965535
|
At this point, the offset was exhausted and all invoices, that respect the query, were collected to |
I am also beginner. As far I understand the code, the maximum length of Edit: |
You right! Thanks! I'll be working on it. |
@yyforyongyu now the correction is considering the point indicate by @feelancer21, when the request comes with --count_total_payments and --max_payments causing pagination. Ready to go, thank you both! |
Thanks. I see some redundant code now, reading the payment and checking the criteria are mainly the same in |
a29b155
to
d91efbc
Compare
Yes, thats make sense. Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter failed, and a nit in the method name. Could you add a release note to 0.18.3? Then it should be good to go.
0469d98
to
b5d7255
Compare
Thanks! The linter failed because there was a missing blank line before a 'return'. I can see that in others parts of the code, is there an specific case where the blank line is a must? The release note could be the following:
Should I update the release note file, or you do? |
It's needed for readability. The linter only checks new code based on a configured look-ahead commit hash. That's probably why you'd see some other parts still use this pattern.
Yeah it's usually the author who does the update. You'll find the right section and add the note, also update the author list in the end of the release note file. |
@yyforyongyu just commited the release notes 0.18.3 changes. Thank you! |
66fae02
to
6fd6272
Compare
@yyforyongyu I did not understand the lint fail!! Any Tip? |
hmm this has failed in two of the itests,
Think it's related? |
I have run the tests after the change and they are running properly! Running integration tests with btcd backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To see the failure, you need to run it against the SQL db via make itest icase=list_payments dbbackend=postgres
.
Also I'm not sure if #8530 is indeed an issue - what if a user actually wants to know the total num of payments instead of the ranged payments?
If we want to go with the current design, we should also update the count_total_payments
here to be count_payments
, and explain in details when the total num will be counted, and when the ranged num will be counted,
Lines 4184 to 4190 in 04dde98
/* | |
If set, all payments (complete and incomplete, independent of the | |
max_payments parameter) will be counted. Note that setting this to true will | |
increase the run time of the call significantly on systems that have a lot | |
of payments, as all of them have to be iterated through to be counted. | |
*/ | |
bool count_total_payments = 5; |
|
||
countFn := func(sequenceKey, hash []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This func won't work in SQL unless we change ForAll
to ForEach
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the change ... now it is working even to SQL databases ... but as you said there is no otimization when there is a SQL database available because we cannot use the ForAll
instead of ForEach
as it is done before this fix.
As you said below maybe we shouldn't touch it until we can find an efficient counting method. Maybe this is another PR to come!
|
||
countFn := func(sequenceKey, hash []byte) error { | ||
r := bytes.NewReader(hash) | ||
paymentHash, err := deserializePaymentIndex(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since counting payments is already slow, this would make it even slower - for nodes with millions of payments this would take a very long time. I think at least we should make this query conditional, that,
- if there aren't any date restrictions, use the original
totalPayments++
approach. - if there is a date range, use this
countFn
.
But I still think this would be very slow, cc @bhandras for some help here, maybe we shouldn't touch it until we can find an efficient counting method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is a way to measure the performance impact and only move forward if it is near the same of we have today without the fix.
About it is an issue or not, I have the following opinion: |
I'm not sure how to proceed here... It sounds to me like the expectation for what the Maybe what we need is a new flag, Then we can change the description of the |
Thanks a lot for the input so far @MPins ! I'd recommend pausing the development of this PR at the moment as we will soon bring native SQL support for payments in 0.19.0 - maybe we can find a more efficient way to perform the queries by then. |
QueryPayments function was changed to return the total num of payments considering the date start and date end restrictions existing on the query.
When there are no payments expected returning, besides checking if the paymensts is empty, check also if the total num of payments is zero.
@MPins, remember to re-request review from reviewers when ready |
!lightninglabs-deploy mute |
Fixes #8530
Change Description
The function QueryPayments was not considering the dates restriction when counting the total num of payments.
Steps to Test
lncli listpayments --creation_date_start 1650000000 --count_total_payments --creation_date_end 1650000001 | grep total_num_payments
This command should return 0 as there are no payments between the date start and the date end.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.