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.

Sign up for GitHub

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

fix: listpayments --count_total_payments should honor --creation_date_start and --creation_date_end #8565

Closed
wants to merge 4 commits into from
Closed

fix: listpayments --count_total_payments should honor --creation_date_start and --creation_date_end #8565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b33b39
channeldb: add creation ts start end check in query all
violetguos Mar 18, 2024
a2b4967
docs: Reformat w linter, add RELEASE note
violetguos Mar 20, 2024
21832d4
channeldb: Add CountTotal to all test cases
violetguos Mar 20, 2024
2c8d039
multi: Fix the linter whitespace for multi line
violetguos Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion channeldb/payments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,10 @@ func (d *DB) QueryPayments(query PaymentsQuery) (PaymentsResponse, error) {
// Counting the total number of payments is expensive, since we
// literally have to traverse the cursor linearly, which can
// take quite a while. So it's an optional query parameter.
if query.CountTotal {
if query.CountTotal && query.CreationDateStart == 0 &&
query.CreationDateEnd == 0 {
// It will only take effect if CreationDateStart and
// CreationDateEnd are not specified.
var (
Comment on lines +600 to 603
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think this is the behaviour we want. I think that we still want to count how many payments there were but we just want to count the ones that happened between the given start and end times.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the feedback @ellemouton

the original bug report by @AndySchroder specifies that

$ lncli listpayments --creation_date_start 1650000000 --count_total_payments --creation_date_end 1650000001|grep total_num_payments
    "total_num_payments": "0"
$

it seems like the original issue had a different interpretation on the expected behaviour

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont agree. In that example he is specifying a difference in start and end time of 1 second (millisecond?). So it is expected that there are no payments during that time

Copy link

Choose a reason for hiding this comment

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

yes, that time difference was specified to assume there were no payments between that short period of time.

Copy link

Choose a reason for hiding this comment

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

I dont think this is the behaviour we want. I think that we still want to count how many payments there were but we just want to count the ones that happened between the given start and end times.

yes, this was the original purpose of the bug

Copy link

@AndySchroder AndySchroder Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the feedback @ellemouton

the original bug report by @AndySchroder specifies that

$ lncli listpayments --creation_date_start 1650000000 --count_total_payments --creation_date_end 1650000001|grep total_num_payments
    "total_num_payments": "0"
$

it seems like the original issue had a different interpretation on the expected behaviour

no, please understand the interpretation that @ellemouton is providing. she understands bug report correctly.

totalPayments uint64
err error
Expand Down
53 changes: 53 additions & 0 deletions channeldb/payments_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ func TestQueryPayments(t *testing.T) {
// expectedSeqNrs contains the set of sequence numbers we expect
// our query to return.
expectedSeqNrs []uint64
// totalCount corresponds to the PaymentsResponse's TotalCount
// This will only be set if the
// CountTotal field in the query was set to true.
totalCount uint64
}{
{
name: "IndexOffset at the end of the payments range",
Expand All @@ -259,6 +263,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 0,
lastIndex: 0,
expectedSeqNrs: nil,
totalCount: 0,
},
{
name: "query in forwards order, start at beginning",
Expand All @@ -271,6 +276,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 1,
lastIndex: 3,
expectedSeqNrs: []uint64{1, 3},
totalCount: 0,
},
{
name: "query in forwards order, start at end, overflow",
Expand All @@ -283,6 +289,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 7,
lastIndex: 7,
expectedSeqNrs: []uint64{7},
totalCount: 0,
},
{
name: "start at offset index outside of payments",
Expand All @@ -307,6 +314,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 5,
lastIndex: 7,
expectedSeqNrs: []uint64{5, 6, 7},
totalCount: 0,
},
{
name: "start at offset index outside of payments, " +
Expand All @@ -320,6 +328,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 6,
lastIndex: 7,
expectedSeqNrs: []uint64{6, 7},
totalCount: 0,
},
{
name: "query in reverse order, start at end",
Expand All @@ -332,6 +341,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 6,
lastIndex: 7,
expectedSeqNrs: []uint64{6, 7},
totalCount: 0,
},
{
name: "query in reverse order, starting in middle",
Expand All @@ -344,6 +354,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 1,
lastIndex: 3,
expectedSeqNrs: []uint64{1, 3},
totalCount: 0,
},
{
name: "query in reverse order, starting in middle, " +
Expand All @@ -357,6 +368,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 1,
lastIndex: 3,
expectedSeqNrs: []uint64{1, 3},
totalCount: 0,
},
{
name: "all payments in reverse, order maintained",
Expand All @@ -369,6 +381,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 1,
lastIndex: 7,
expectedSeqNrs: []uint64{1, 3, 4, 5, 6, 7},
totalCount: 0,
},
{
name: "exclude incomplete payments",
Expand All @@ -381,6 +394,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 7,
lastIndex: 7,
expectedSeqNrs: []uint64{7},
totalCount: 0,
},
{
name: "query payments at index gap",
Expand All @@ -405,6 +419,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 1,
lastIndex: 1,
expectedSeqNrs: []uint64{1},
totalCount: 0,
},
{
name: "query payments reverse on index gap",
Expand All @@ -417,6 +432,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 1,
lastIndex: 1,
expectedSeqNrs: []uint64{1},
totalCount: 0,
},
{
name: "query payments forward on index gap",
Expand All @@ -429,6 +445,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 3,
lastIndex: 4,
expectedSeqNrs: []uint64{3, 4},
totalCount: 0,
},
{
name: "query in forwards order, with start creation " +
Expand All @@ -443,6 +460,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 5,
lastIndex: 6,
expectedSeqNrs: []uint64{5, 6},
totalCount: 0,
},
{
name: "query in forwards order, with start creation " +
Expand All @@ -457,6 +475,7 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 7,
lastIndex: 7,
expectedSeqNrs: []uint64{7},
totalCount: 0,
},
{
name: "query with start and end creation time",
Expand All @@ -471,6 +490,33 @@ func TestQueryPayments(t *testing.T) {
firstIndex: 3,
lastIndex: 5,
expectedSeqNrs: []uint64{3, 4, 5},
totalCount: 0,
},
{
name: "query count total with start and end creation",
query: PaymentsQuery{
IndexOffset: 0,
MaxPayments: 2,
Reversed: true,
IncludeIncomplete: true,
CreationDateStart: 5,
CreationDateEnd: 6,
CountTotal: true,
},
firstIndex: 5,
lastIndex: 6,
expectedSeqNrs: []uint64{5, 6},
totalCount: 0,
},
{
name: "query count total only",
query: PaymentsQuery{
CountTotal: true,
},
firstIndex: 0,
lastIndex: 0,
expectedSeqNrs: nil,
totalCount: 6,
},
}

Expand Down Expand Up @@ -580,6 +626,13 @@ func TestQueryPayments(t *testing.T) {
len(tt.expectedSeqNrs), len(querySlice.Payments))
}

if tt.totalCount != querySlice.TotalCount {
t.Errorf("Total Count does not match "+
"expected total count. Want %d, got %d",
tt.totalCount,
querySlice.TotalCount)
}

for i, seqNr := range tt.expectedSeqNrs {
q := querySlice.Payments[i]
if seqNr != q.SequenceNum {
Expand Down
5 changes: 5 additions & 0 deletions docs/release-notes/release-notes-0.18.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
- [Contributors (Alphabetical Order)](#contributors-alphabetical-order)

# Bug Fixes
* [Fix a bug](https://github.com/lightningnetwork/lnd/pull/8565) where
`listpayments --count_total_payments` flag disregarded `--creation_date_start`
and `--creation_date_end`. The payments between the dates would be queried
instead of the total payments if the dates were supplied.
Copy link

Choose a reason for hiding this comment

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

No, this is not correct.


* [Fix a bug](https://github.com/lightningnetwork/lnd/pull/8097) where
`sendcoins` command with `--sweepall` flag would not show the correct amount.

Expand Down