Modified the listchanneltxn subcommand #7496

Chinwendu20 · 2023-03-08T22:17:43Z

Change Description

start_height must be <= end_height, error otherwise
transactions are returned in direct order (oldest first)
an additional flag --reverse is added to change the order (--reverse true, recent first)
Modified docs by modifying comments in the listchaintxn description command

Steps to Test

Carry out a debug installation using this command:
make && make install

Test command:
./lncli-debug --no-macaroons listchaintxns --start_height 200 --end_height 7000| grep block_height
Expected outcome: transactions are returned in the order, oldest first

./lncli-debug --no-macaroons listchaintxns --start_height 7000 --end_height 200| grep block_height
Expected outcome: Error

./lncli-debug --no-macaroons listchaintxns --start_height 200 --end_height 7000 --reverse| grep block_height
Expected outcome: transactions are returned in the reverse order, newest first

Change after review

Test command:
./lncli-debug --no-macaroons listchaintxns --start_height 200 --end_height 7000 --reverse| grep block_height
Expected outcome: transactions are returned in the order, oldest first

./lncli-debug --no-macaroons listchaintxns --start_height 7000 --end_height 200| grep block_height
Expected outcome: Error

./lncli-debug --no-macaroons listchaintxns --start_height 200 --end_height 7000| grep block_height
Expected outcome: transactions are returned in the order, newest first

positiveblue

Nice work @Chinwendu20 🎉

I left some comments, let me know what you think about them. One thing we should avoid is breaking changes in the API. Until now we were ordering by NumConfirmations so from newest to oldest (non-confirmed being the newest ones). We should keep that behaivour and use the --reverse to sort them from older to newer 👌

cmd/lncli/commands.go

positiveblue · 2023-03-09T06:20:34Z

lnrpc/lightning.proto

@@ -746,6 +746,11 @@ message GetTransactionsRequest {

    // An optional filter to only include transactions relevant to an account.
    string account = 3;
+
+    //  It modifies the way the transactions are listed, in reverse order i.e.


format: it fits in two lines

Okay thank you. I will make it fit in two lines.

positiveblue · 2023-03-09T06:24:02Z

lnrpc/rpc_utils.go

@@ -76,24 +76,25 @@ func RPCTransaction(tx *lnwallet.TransactionDetail) *Transaction {
 }

 // RPCTransactionDetails returns a set of rpc transaction details.
-func RPCTransactionDetails(txns []*lnwallet.TransactionDetail) *TransactionDetails {
+func RPCTransactionDetails(txns []*lnwallet.TransactionDetail, reverse bool) *TransactionDetails {


what do you think about not adding the extra arg here so we do not have to hardcode the param in other calls and reversing the array, if needed, directly in the GetTransactions grpc implementation?

positiveblue · 2023-03-09T06:26:57Z

cmd/lncli/commands.go

+	end_height := ctx.Int64("end_height")
+
+	if start_height > end_height {
+


formatt: extra line

positiveblue · 2023-03-09T06:40:15Z

cmd/lncli/commands.go

-	less than the start_height, transactions will be queried in reverse.
-	To get all transactions until the chain tip, including unconfirmed
-	transactions (identifiable with BlockHeight=0), set end_height to -1.
+	block range over which to query for transactions. The value of the start_height flag must


I think we are giving too much information here. We can summarize it with something like

... block range over which to query for transactions. By default the result is sorted by oldest first. Use the reverse flag to invert the order. To get all transactions until the chain tip, including unconfirmed transactions (identifiable with BlockHeight=0), set end_height to -1. Non-confirmed transactions considered the newest.

positiveblue · 2023-03-13T22:33:33Z

cmd/lncli/commands.go

+	start_height := ctx.Int64("start_height")
+	end_height := ctx.Int64("end_height")
+
+	if (start_height > end_height) &&


This logic looks like a good candidate for a switch statement. What do you think?

switch { case ctx.IsSet("start_height") && ctx.IsSet("end_height"): start_height := ctx.Int64("start_height") end_height := ctx.Int64("end_height") if end_height == -1 && start_height != 0 { return errors.New("...") } if end_height != -1 && start_height > end_height { return errors.New("...") } req.StartHeight = int32(start_height) req.EndHeight = int32(end_height) case ctx.IsSet("start_height"): req.StartHeight = int32(ctx.Int64("start_height")) case ctx.IsSet("end_height"): req.EndHeight = int32(ctx.Int64("end_height")) }

It is really important that this check is replicated in the server side. If you follow the full flow the btcwallet library used for checking the range will return the result reversed if start_height > end_height starting from end to start. Because this is something that we do not want to expose in the API (we have the --reverse flag for that) we need to catch it before processing the range.

This logic looks like a good candidate for a switch statement. What do you think?

switch { case ctx.IsSet("start_height") && ctx.IsSet("end_height"): start_height := ctx.Int64("start_height") end_height := ctx.Int64("end_height") if end_height == -1 && start_height != 0 { return errors.New("...") } if end_height != -1 && start_height > end_height { return errors.New("...") } req.StartHeight = int32(start_height) req.EndHeight = int32(end_height) case ctx.IsSet("start_height"): req.StartHeight = int32(ctx.Int64("start_height")) case ctx.IsSet("end_height"): req.EndHeight = int32(ctx.Int64("end_height")) }

It is really important that this check is replicated in the server side. If you follow the full flow the btcwallet library used for checking the range will return the result reversed if start_height > end_height starting from end to start. Because this is something that we do not want to expose in the API (we have the --reverse flag for that) we need to catch it before processing the range.

Thank you so much. I would work with the switch statement but are you saying that I should make some changes in the btcwallet as well or write your statement below as comments

positiveblue · 2023-03-13T22:47:51Z

lnrpc/rpc_utils.go

-	// follow the most recently set of confirmed transactions. If we sort
-	// by height, unconfirmed transactions will follow our oldest
-	// transactions, because they have lower block heights.
-	sort.Slice(txDetails.Transactions, func(i, j int) bool {


I think we can leave this here but modify it if needed by the caller.

Oh okay thanks I will move it back there

positiveblue · 2023-03-13T22:51:22Z

rpcserver.go

-	return lnrpc.RPCTransactionDetails(transactions), nil
+	txDetails := lnrpc.RPCTransactionDetails(transactions)
+
+	if !req.Reverse {


If we leave the previous logic in RPCTransactionDetails, we can do the opposite sorting here only when we need it

txs := lnrpc.RPCTransactionDetails(transactions) if !req.Reverse { for i, j := 0, len(txs)-1; i < j; i, j = i+1, j-1 { txs[i], txs[j] = txs[j], txs[i] } } return txs, nil

Okay thank you. I will use this.

positiveblue · 2023-03-13T22:55:31Z

cmd/lncli/commands.go

@@ -1798,19 +1798,26 @@ var listChainTxnsCommand = cli.Command{
 				"until the chain tip, including unconfirmed, " +
 				"set this value to -1",
 		},
+		cli.BoolFlag{
+			Name: "reverse",
+			Usage: "It modifies the way the transactions are listed, " +


nit: If true, returned transactions will be sorted from oldest to newest

positiveblue

Perfect @Chinwendu20, I think we are getting close 🎉

There are some extra spaces, comments that do not end with ., etc... You can check if the code follows the guidelines here

We also try to split the commit in individual logic units. In this case I think that what makes sense is 3 commits:

Add rpc fields
Handle req.reverse field in the rpcserver.go
Add --reverse flag to the cli

Chinwendu20 · 2023-03-14T05:24:00Z

Thanks would this involve amending existing commit messages or would I have to take Git HEAD to the tip before I started making changes then make all the changes one after the other, to get the commit to look like that

positiveblue · 2023-03-14T05:46:39Z

Thanks would this involve amending existing commit messages or would I have to take Git HEAD to the tip before I started making changes then make all the changes one after the other, to get the commit to look like that

In this case I would take the second approach. You can reset your head while keeping the changes (git reset --soft HEAD~2), then add the couple of fixes + create the right commit structure (git commit -p is your friend here).

When the log is in good shape remember to add one last commit updating the release notes (you can check other PRs to see how it's done)

Chinwendu20 · 2023-03-27T19:00:02Z

ok working on it @positiveblue

Chinwendu20 · 2023-03-28T08:23:09Z

I have made the change @positiveblue

ellemouton · 2023-04-18T11:29:06Z

@Chinwendu20 - are you planning to continue work on this? if so, please click the "re-request" review button :)

Chinwendu20 · 2023-04-18T15:21:19Z

Yes, thank you. Just seeing that there is a conflict. I will fix it and re-request a review @ellemouton

ellemouton · 2023-04-20T14:04:18Z

@Chinwendu20 - it seems that not all the comments left in the previous review have been addressed?

Chinwendu20 · 2023-04-21T09:06:08Z

Thanks please do you know how I can reply to those reviews. Last time I did that it showed pending and @positiveblue could not see it.

ellemouton · 2023-04-21T09:08:05Z

you have to go into "Files Changed" and then click the "Review Changes" button. Then you will be able to click "Submitt Review" which would submit your comments

Chinwendu20

https://github.com/lightningnetwork/lnd/pull/7496/files#r1134697179

Thanks you so much for the review. It is actually the other way around, if true, returned transactions are returned first

cmd/lncli/commands.go

Chinwendu20 · 2023-04-21T09:23:22Z

Okay thank you

Chinwendu20 · 2023-09-09T19:12:02Z

Hello @positiveblue please I have made changes to this PR do you mind reviewing it again?

Chinwendu20 · 2023-09-09T19:30:24Z

Reverting the lnrpc changes... hold on

Chinwendu20 · 2023-09-09T20:15:00Z

All done. Ready for review.

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 · 2023-09-18T09:22:12Z

Hello, I have added the release note and fixed the lint error, do you mind rerunning the workflow?

- Added reverse flag to listchaintxns command. - Listchaintxns command returns error when start_height is less than end_height if end_height is not -1. - Listchaintxns command returns error when start_height is not zero but end_height is -1. Signed-off-by: Ononiwu Maureen <[email protected]>

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 · 2023-09-18T10:52:38Z

So sorry for the back and forth, for some reason. I cannot seem to see any linting error like it is on the CI when I run make lint locally:

mohamedawnallah · 2024-03-06T19:21:24Z

@Chinwendu20 Is there anything I could help with so that we could get this PR? The CI regards Linting the code seems passed.

mohamedawnallah · 2024-03-10T20:42:53Z

@Chinwendu20 I had a similar issue regards running the make lint command since I've 8GB of RAM. This is solved by passing the number of concurrent workers to be 1 so the full command make lint workers=1 hope it works for you :)

So sorry for the back and forth, for some reason. I cannot seem to see any linting error like it is on the CI when I run make lint locally:

Chinwendu20 · 2024-03-11T11:37:50Z

Thanks @mohamedawnallah. I think I was waiting for review here?

lightninglabs-deploy · 2024-09-03T01:32:53Z

@positiveblue: review reminder

lightninglabs-deploy · 2024-09-09T00:59:45Z

Closing due to inactivity

lightninglabs-deploy · 2024-09-09T02:02:24Z

Closing due to inactivity

lightninglabs-deploy · 2024-09-09T03:05:41Z

Closing due to inactivity

lightninglabs-deploy · 2024-09-09T04:09:22Z

Closing due to inactivity

lightninglabs-deploy · 2024-09-09T05:13:05Z

Closing due to inactivity

lightninglabs-deploy · 2024-09-09T06:16:09Z

Closing due to inactivity

lightninglabs-deploy · 2024-09-09T07:18:52Z

Closing due to inactivity

coderabbitai · 2024-09-09T07:20:58Z

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>.
- 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 generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @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.

positiveblue suggested changes Mar 9, 2023

View reviewed changes

positiveblue reviewed Mar 13, 2023

View reviewed changes

positiveblue suggested changes Mar 13, 2023

View reviewed changes

Chinwendu20 force-pushed the fix-lncli-listchannels branch from 8390a5d to 64e934c Compare March 28, 2023 08:22

Chinwendu20 force-pushed the fix-lncli-listchannels branch from 64e934c to 688494e Compare April 20, 2023 12:36

Chinwendu20 requested a review from positiveblue April 20, 2023 13:50

Chinwendu20 commented Apr 21, 2023

View reviewed changes

cmd/lncli/commands.go Outdated Show resolved Hide resolved

Chinwendu20 force-pushed the fix-lncli-listchannels branch from 688494e to 598dd2b Compare September 9, 2023 19:05

Chinwendu20 force-pushed the fix-lncli-listchannels branch from 598dd2b to fe630db Compare September 9, 2023 20:13

Chinwendu20 force-pushed the fix-lncli-listchannels branch from fe630db to 1f8d4e9 Compare September 14, 2023 11:43

lnrpc: Reverse bool added to Gettransactionrequest

41c244f

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 force-pushed the fix-lncli-listchannels branch from 1f8d4e9 to dc32629 Compare September 18, 2023 09:16

Chinwendu20 added 2 commits September 18, 2023 10:50

lnd: Added reverse functionality to GetTransactions.

aa0272a

Signed-off-by: Ononiwu Maureen <[email protected]>

docs: Added release note for changes.

8b98851

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 force-pushed the fix-lncli-listchannels branch from dc32629 to 8b98851 Compare September 18, 2023 10:51

mohamedawnallah mentioned this pull request Mar 6, 2024

[bug]: lncli listchaintxns in reversed order #7316

Open

guggero added beginner Issues suitable for new developers up for grabs PRs which have been abandoned by their original authors and can be taken up by someone else needs rebase PR has merge conflicts labels Sep 9, 2024

guggero closed this Sep 9, 2024

		end_height := ctx.Int64("end_height")

		if start_height > end_height {

Modified the listchanneltxn subcommand #7496

Modified the listchanneltxn subcommand #7496

Conversation

Chinwendu20 commented Mar 8, 2023 • edited Loading

Change Description

Steps to Test

positiveblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

positiveblue Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Chinwendu20 Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

positiveblue Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Chinwendu20 Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

positiveblue left a comment

Choose a reason for hiding this comment

Chinwendu20 commented Mar 14, 2023

positiveblue commented Mar 14, 2023

Chinwendu20 commented Mar 27, 2023

Chinwendu20 commented Mar 28, 2023

ellemouton commented Apr 18, 2023

Chinwendu20 commented Apr 18, 2023

ellemouton commented Apr 20, 2023

Chinwendu20 commented Apr 21, 2023

ellemouton commented Apr 21, 2023 • edited Loading

Chinwendu20 left a comment

Choose a reason for hiding this comment

Chinwendu20 commented Apr 21, 2023

Chinwendu20 commented Sep 9, 2023

Chinwendu20 commented Sep 9, 2023

Chinwendu20 commented Sep 9, 2023

Chinwendu20 commented Sep 18, 2023

Chinwendu20 commented Sep 18, 2023 • edited Loading

mohamedawnallah commented Mar 6, 2024 • edited Loading

mohamedawnallah commented Mar 10, 2024

Chinwendu20 commented Mar 11, 2024

lightninglabs-deploy commented Sep 3, 2024

lightninglabs-deploy commented Sep 9, 2024

lightninglabs-deploy commented Sep 9, 2024

lightninglabs-deploy commented Sep 9, 2024

lightninglabs-deploy commented Sep 9, 2024

lightninglabs-deploy commented Sep 9, 2024

lightninglabs-deploy commented Sep 9, 2024

lightninglabs-deploy commented Sep 9, 2024

coderabbitai bot commented Sep 9, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Chinwendu20 commented Mar 8, 2023 •

edited

Loading

positiveblue Mar 13, 2023 •

edited

Loading

Chinwendu20 Apr 21, 2023 •

edited

Loading

positiveblue Mar 13, 2023 •

edited

Loading

Chinwendu20 Apr 21, 2023 •

edited

Loading

ellemouton commented Apr 21, 2023 •

edited

Loading

Chinwendu20 commented Sep 18, 2023 •

edited

Loading

mohamedawnallah commented Mar 6, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)