pagination: add pagination to wallet transactions by Abdulkbk · Pull Request #8998 · lightningnetwork/lnd

Abdulkbk · 2024-08-11T15:54:24Z

Change Description

This PR adds pagination to getting transactions RPC calls by introducing index_offset and max_transactions
to specify the number of transactions to skip and the maximum number of transactions to return respectively.

Steps to Test

Steps for reviewers to follow to test the change.

Start LND
Run lncli listchaintxns -index_offset=n -max_transactions=m where n and m are any numbers
The result will be a list skipping n numbers of transactions and with m transactions

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

coderabbitai · 2024-08-11T15:54:28Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

guggero

Thanks for the PR. Looks pretty good already, have a bit of early feedback.

lnrpc/lightning.proto

guggero · 2024-08-12T07:19:38Z

lnwallet/interface.go

 	// empty, transactions of all wallet accounts are returned.
 	ListTransactionDetails(startHeight, endHeight int32,
-		accountFilter string) ([]*TransactionDetail, error)
+		accountFilter string, indexOffset int, maxTnxs int) ([]*TransactionDetail, error)


nit: break before maxTnxs to avoid too long line. Also, should be spelled maxTxns?

Avoiding a long line was why I named it maxTnx, but after breaking the line, I now renamed it to maxTransactions

Looks like this is still unaddressed?

Oh I see, I unintentionally addressed this in another commit

guggero · 2024-08-12T07:20:08Z

rpcserver.go

 	}

+	// To remain backward compatible, If the number of transactions was not
+	// specified, then we'll default to returning the entire transactions


nit: missing full stop at end of sentence.

Unresolving this issue as it's not addressed.

lnwallet/btcwallet/btcwallet.go

Abdulkbk · 2024-08-12T10:18:26Z

Thanks for the PR. Looks pretty good already, have a bit of early feedback.

Thanks for taking a look. I will address the feedback you left.

Abdulkbk · 2024-08-13T22:12:41Z

Thanks for the PR. Looks pretty good already, have a bit of early feedback.

I've addressed the feedback you left, added a few changes too.

guggero

Thanks for adding the extra parameters. Still needs a few updates before I'll start manual testing. But getting there 👍

cmd/lncli/commands.go

rpcserver.go

lnwallet/test/test_interface.go

guggero · 2024-08-16T09:13:27Z

lnwallet/interface.go

 	// empty, transactions of all wallet accounts are returned.
 	ListTransactionDetails(startHeight, endHeight int32,
-		accountFilter string) ([]*TransactionDetail, error)
+		accountFilter string, indexOffset int, maxTnxs int) ([]*TransactionDetail, error)


Looks like this is still unaddressed?

lightninglabs-deploy · 2024-11-04T00:34:52Z

@Abdulkbk, remember to re-request review from reviewers when ready

yyforyongyu · 2024-11-06T03:39:59Z

Approved CI run

guggero

Thanks for the updates! Did another pass, we're definitely getting closer.

lnrpc/lightning.proto

lnwallet/btcwallet/btcwallet.go

lnrpc/lightning.proto

cmd/commands/commands.go

guggero · 2024-11-06T08:30:53Z

lnwallet/btcwallet/btcwallet.go

-	return txDetails[indexOffset:end], nil
+	txDetails = txDetails[indexOffset:end]
+
+	return txDetails, uint64(firstIdx), uint64(lastIdx), nil


Why do we need these two extra variables at all? Shouldn't firstIdx = indexOffset and lastIdx = end-1?

If my previous comment is correct, then this wouldn't be the case. However, I can replace the end variable with lastIdx, since they are the same?.

We'll want zero-based indexes here. See my other comment, unfortunately the invoices template you looked at has a special case with 0 that we don't need to port over to this.

I updated it to use a zero-based index.

guggero · 2024-11-06T08:32:46Z

lnwallet/btcwallet/btcwallet.go

+	// If maxTransactions is set to -1, then we'll return all transactions
+	// starting from the offset.
+	if maxTransactions == -1 {
+		lastIdx = len(txDetails)


This is incorrect. The last index is len(txDetails)-1. But for the slice access ([a:b]) it's correct, since b is exclusive.

My understanding is that first_index_offset and last_index_offset are 1-based indices. I am using the implementation found here as a reference, and it states here that the AddIndex starts from 1.

Also since the last_index_offset will be used as the new index_offset for the next call, wouldn't returning len(txDetails)-1 result in repeating the last transaction from the previous call as the first transaction in the new call? This is because the slice access [indexOffset: ...] is inclusive.

Unfortunately the example from the invoices is a bad one. There the add index has a special meaning and zero is kind of a special case...

But an index in a pagination should always return the exact indexes of the first and last entry. Then my pagination query can just look something like this (pseudo code):

offset := 0 pageSize := 20 result := nil for { result = query(offset, pageSize) if len(result) < pageSize { return } offset = result.last_index + 1 }

rpcserver.go

lnwallet/mock.go

lnwallet/test/test_interface.go

Abdulkbk · 2024-11-12T11:22:02Z

Thank you, @guggero. I have addressed your feedback and left some questions.

guggero

Getting super close!

rpcserver.go

guggero · 2024-11-12T12:37:31Z

lnwallet/btcwallet/btcwallet.go

+	// If maxTransactions is set to -1, then we'll return all transactions
+	// starting from the offset.
+	if maxTransactions == -1 {
+		lastIdx = len(txDetails)


Unfortunately the example from the invoices is a bad one. There the add index has a special meaning and zero is kind of a special case...

But an index in a pagination should always return the exact indexes of the first and last entry. Then my pagination query can just look something like this (pseudo code):

offset := 0 pageSize := 20 result := nil for { result = query(offset, pageSize) if len(result) < pageSize { return } offset = result.last_index + 1 }

lnrpc/lightning.proto

guggero · 2024-11-12T12:40:10Z

lnwallet/btcwallet/btcwallet.go

-	return txDetails[indexOffset:end], nil
+	txDetails = txDetails[indexOffset:end]
+
+	return txDetails, uint64(firstIdx), uint64(lastIdx), nil


We'll want zero-based indexes here. See my other comment, unfortunately the invoices template you looked at has a special case with 0 that we don't need to port over to this.

yyforyongyu

Very useful feature! Left some comments re how to test it more to make it robust.

yyforyongyu · 2024-11-12T13:52:34Z

lnrpc/lightning.proto

+    The maximal number of transactions returned in the response to this query.
+    This value should be set to -1 to return all transactions.
+    */
+    int32 max_transactions = 5;


hmmm as a user I would be a bit confused here...why would i ever wanna the RPC to return zero num of txns? Should we instead use 0 as the default, and return all the txns if max_transactions is not set(hence 0)?

Think using 0 also has the benefit of setting this to uint32, so it's never negative - one less case to worry about.

Yes, I believe this makes a lot of sense.

yyforyongyu · 2024-11-12T13:53:14Z

rpcserver.go

 	}

+	// To remain backward compatible, If the number of transactions was not
+	// specified, then we'll default to returning the entire transactions


Unresolving this issue as it's not addressed.

yyforyongyu · 2024-11-12T14:09:15Z

lnwallet/test/test_interface.go

 		t.Fatalf("expected 0 transactions, got: %v", len(txDetails))
 	}
+
+	// Next we try to query for only 5 transactions.


seems a perfect fit for table-driven tests, if it's easier, we can copy the part where we create all the txns in testListTransactionDetails, create a new test testListTransactionDetailsOffset, and check all the cases.

when there are no txns, a larger index_offset should give us nothing.

when there are no txns, a larger max_transactions should give us nothing.

when there are no txns, larger max_transactions and index_offset should give us nothing.

when there are 10 txns, check index_offset=0, max_transactions=-1 for the default case

when there are 10 txns, check index_offset=5, max_transactions=-1 for a within bound offset

when there are 10 txns, check index_offset=9, max_transactions=-1 for an edge offset

when there are 10 txns, check index_offset=10, max_transactions=-1 for an outbound offset

when there are 10 txns, check index_offset=0, max_transactions=1 for a within bound max num

when there are 10 txns, check index_offset=0, max_transactions=10 for an edge max num

when there are 10 txns, check index_offset=0, max_transactions=11 for an outbound max num

I have created a new commit and added a new test as you suggested.

Abdulkbk · 2024-11-13T23:12:37Z

Thank you, @guggero and @yyforyongyu, for your review. I have addressed the feedback you left.

guggero

One last nit. And an entry in the docs/release-notes/release-notes-0.19.0.md file would be nice, then this can land 🎉

lnwallet/test/test_interface.go

Abdulkbk · 2024-11-20T16:43:18Z

Thank you, @guggero. I have addressed your feedback and added a release note.

This commit adds index_offset and max_transactions to the list of parameters passed during gettransactions call. index_offset specify transactions to skip and max_transactions the total transactions returned

yyforyongyu · 2024-11-21T04:07:38Z

approved CI run, which take a final look when it is completed!

Abdulkbk · 2024-11-21T08:06:35Z

approved CI run, which take a final look when it is completed!

noticed the linter reported unnecessary trailing space in the code, so I fixed that.

guggero

LGTM 🎉

yyforyongyu

One final thing re the unit test, then it's good to go!

yyforyongyu · 2024-11-22T04:12:03Z

lnwallet/test/test_interface.go

+		startHeight, chainTip, "", 0, 10,
+	)
+	require.NoError(t, err)
+	require.Less(t, len(txDetails), 10)


that's do a strict check to be safu? require.Len(t, txDetails, 5)

cmd/commands/commands.go

lnrpc/walletrpc/walletkit_server.go

This commit modifies listtransactiondetails method definition to take in additional params: index_offset and maxTxn

In this commit we introduce first and last indices for the tranasctions returned which can be used to seek for further transactions in a pagination style. Signed-off-by: Abdullahi Yunus <abdoollahikbk@gmail.com>

In this commit, we test for different values of index_offset and max_transactions settings when getting transactions to make sure the right number of transactions are returned along with the right first and last indices.

Abdulkbk · 2024-11-22T07:33:25Z

I've addressed the feedback you left @yyforyongyu

yyforyongyu

LGTM🚢

Abdulkbk marked this pull request as draft August 11, 2024 15:57

guggero reviewed Aug 12, 2024

View reviewed changes

Abdulkbk marked this pull request as ready for review August 12, 2024 15:30

Abdulkbk force-pushed the trx-pagination branch 2 times, most recently from 21fb0a8 to a6609f5 Compare August 13, 2024 21:38

Abdulkbk mentioned this pull request Aug 13, 2024

Provide ability to paginate transactions similar to invoices and payments #4719

Closed

guggero self-requested a review August 14, 2024 07:48

guggero reviewed Aug 16, 2024

View reviewed changes

Abdulkbk force-pushed the trx-pagination branch 2 times, most recently from 31aed37 to 594f3f1 Compare August 23, 2024 01:05

Abdulkbk force-pushed the trx-pagination branch from 594f3f1 to e4c47bb Compare October 26, 2024 16:42

Abdulkbk force-pushed the trx-pagination branch from e4c47bb to e5458b9 Compare November 5, 2024 13:34

Abdulkbk requested a review from guggero November 5, 2024 13:36

guggero reviewed Nov 6, 2024

View reviewed changes

Abdulkbk force-pushed the trx-pagination branch 4 times, most recently from 36a59ed to 650de87 Compare November 10, 2024 14:02

guggero self-requested a review November 12, 2024 11:25

guggero reviewed Nov 12, 2024

View reviewed changes

yyforyongyu reviewed Nov 12, 2024

View reviewed changes

Abdulkbk force-pushed the trx-pagination branch from 650de87 to 7c8cf9d Compare November 12, 2024 14:12

Abdulkbk force-pushed the trx-pagination branch 4 times, most recently from 5f6fef4 to 35b8812 Compare November 13, 2024 22:50

guggero self-requested a review November 14, 2024 08:57

Abdulkbk requested a review from yyforyongyu November 14, 2024 15:58

guggero reviewed Nov 20, 2024

View reviewed changes

lnwallet/test/test_interface.go Outdated Show resolved Hide resolved

Abdulkbk force-pushed the trx-pagination branch from 35b8812 to 61ed760 Compare November 20, 2024 16:38

lnrpc: modify gettransaction parameters

037db42

This commit adds index_offset and max_transactions to the list of parameters passed during gettransactions call. index_offset specify transactions to skip and max_transactions the total transactions returned

Abdulkbk force-pushed the trx-pagination branch from 61ed760 to d131218 Compare November 20, 2024 17:05

Abdulkbk force-pushed the trx-pagination branch from d131218 to cf1fb70 Compare November 21, 2024 08:04

guggero approved these changes Nov 21, 2024

View reviewed changes

yyforyongyu reviewed Nov 22, 2024

View reviewed changes

Abdulkbk added 4 commits November 22, 2024 08:21

multi: modify listtxn definition

cd1df4a

This commit modifies listtransactiondetails method definition to take in additional params: index_offset and maxTxn

multi: return txns first and last indices

762d015

In this commit we introduce first and last indices for the tranasctions returned which can be used to seek for further transactions in a pagination style. Signed-off-by: Abdullahi Yunus <abdoollahikbk@gmail.com>

lnwallettest: test for tranasctions pagination

6ee8d2c

In this commit, we test for different values of index_offset and max_transactions settings when getting transactions to make sure the right number of transactions are returned along with the right first and last indices.

add release note

1c6790b

Abdulkbk force-pushed the trx-pagination branch from cf1fb70 to 1c6790b Compare November 22, 2024 07:30

guggero requested a review from yyforyongyu November 22, 2024 07:36

yyforyongyu approved these changes Nov 22, 2024

View reviewed changes

guggero merged commit 94f7ed4 into lightningnetwork:master Nov 22, 2024

guggero mentioned this pull request Feb 25, 2025

[bug]: lncli listchaintxns in reversed order #7316

Open

Abdulkbk deleted the trx-pagination branch July 21, 2025 17:25

Conversation

Abdulkbk commented Aug 11, 2024

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

coderabbitai bot commented Aug 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abdulkbk Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Abdulkbk commented Aug 12, 2024

Uh oh!

Abdulkbk commented Aug 13, 2024

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Nov 4, 2024

Uh oh!

yyforyongyu commented Nov 6, 2024

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abdulkbk Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot commented Aug 11, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

Abdulkbk Aug 19, 2024 •

edited

Loading

Abdulkbk Nov 10, 2024 •

edited

Loading