[bug]: lncli listchaintxns in reversed order

[9rib-on-the-grind]

Background

From lncli documentation:

NAME:
lncli listchaintxns - List transactions from the wallet.

DESCRIPTION:

List all transactions an address of the wallet was involved in.

This call will return a list of wallet related transactions that paid
to an address our wallet controls, or spent utxos that we held. The
start_height and end_height flags can be used to specify an inclusive
block range over which to query for transactions. If the end_height is
less than the start_height, transactions will be queried in reverse.

If the end_height is less than the start_height, transactions will be queried in reverse.

Despite the order of start_height and end_height the transactions are returned in the same order.

Your environment

• version of lnd v0.15.1-beta
• which operating system (uname -a on *Nix) Linux f466fc441522 6.1.4-zen2-1-zen #1 ZEN SMP PREEMPT_DYNAMIC Sat, 07 Jan 2023 15:27:06 +0000 x86_64 Linux
• version of bitcoind Bitcoin Core RPC client version v22.0.0

Steps to reproduce

• start lnd
• mine blocks
• send transaction
• mine blocks
• send transaction
• mine blocks
• call lncli listchaintxns

Expected behaviour

lncli listchaintxns --start_height 1 --end_height 2000 returns transactions in direct order.
lncli listchaintxns --start_height 2000 --end_height 1 returns transactions in reversed order.

Actual behaviour

Both return transactions in reversed order:

lncli listchaintxns --start_height 1130 --end_height 1140 | grep block_height
    "block_height": 1140,
    "block_height": 1134,

lncli listchaintxns --start_height 1140 --end_height 1130 | grep block_height
    "block_height": 1140,
    "block_height": 1134,

[positiveblue]

It's true. I wonder what is better here:

• Do not support reverse results
• Support them but with a --reverse flag and return an error if start_date > end_date
• Implement the documented behaivour

I assume not many people has applications build on top of this feature because it has not working for a while.

[9rib-on-the-grind]

I guess the most intuitive behavior would be:

• start_height <= 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)

[Chinwendu20]

Hello please can i work on this?

[positiveblue]

@Chinwendu20 ofc, let us know if you have any questions during development + remember to link this issue in the PR

[Chinwendu20]

Thanks @positiveblue . I would work on this

[lndcontrib]

I would work on this

It's ok. I'll work on another issue.

[Chinwendu20]

Hello all, I might be a bit slow with this but rest assured I am working on it. I am using this as a motivation to get comfortable with the codebase as I aim to apply as a summer of bitcoin intern under this project. I am currently reading documentation to help me become more acquainted with this unfamiliar territory. As soon as I am comfortable I would submit a PR or ask for help but at least from a place of clarity then.

[lucasdcf]

Take your time @Chinwendu20

Feel free to contact me on Telegram if you need help: @lucasdcf

[Chinwendu20]

Thanks @lucasdcf . Hello I think this 'bug' was intended, see this snippet of code from a function being called by the listchaintxns.

lnd/lnrpc/rpc_utils.go

Lines 88 to 99 in 983d917

	// Sort transactions by number of confirmations rather than height so
	// that unconfirmed transactions (height =0; confirmations =-1) will
	// 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 {
	return txDetails.Transactions[i].NumConfirmations <
	txDetails.Transactions[j].NumConfirmations
	})
	return txDetails
	}

I think we just need to modify the docs.

[9rib-on-the-grind]

Thanks @lucasdcf . Hello I think this 'bug' was intended, see this snippet of code from a function being called by the listchaintxns.

lnd/lnrpc/rpc_utils.go

Lines 88 to 99 in 983d917

	// Sort transactions by number of confirmations rather than height so
	// that unconfirmed transactions (height =0; confirmations =-1) will
	// 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 {
	return txDetails.Transactions[i].NumConfirmations <
	txDetails.Transactions[j].NumConfirmations
	})
	return txDetails
	}

I think we just need to modify the docs.

Good point. If this is the case, modifying the docs seems like a reasonable idea.

[positiveblue]

I may be missing something here but even if we are sorting by number of confirmations rather than height lncli listchaintxns --start_height 1130 --end_height 1140 | grep block_height should be different than lncli listchaintxns --start_height 1140 --end_height 1130 | grep block_height when we have two entries with "block_height": 1140 and "block_height": 1134

[9rib-on-the-grind]

I may be missing something here but even if we are sorting by number of confirmations rather than height lncli listchaintxns --start_height 1130 --end_height 1140 | grep block_height should be different than lncli listchaintxns --start_height 1140 --end_height 1130 | grep block_height when we have two entries with "block_height": 1140 and "block_height": 1134

Why the outputs should be different? A transaction with "block_height": 1134 will always have more confirmations than a transaction with "block_height": 1140. So the later comes first anyway.

[positiveblue]

yes, but then there will never be any difference between --start_height {low_high} --end_height {high_height} and --start_height {high_height} --end_height {low_height}

I think it would be better to sort by height and always put the non-confirmed first for example.

We can leave the API consumers to do the last step and sort it however they want (while we always return them sorted by confirmation order) but then we should return an error if --start_height > --end_height + fix the docs.

[Chinwendu20]

I would be happy to implement whatever solution is agreed upon. Thanks for the input @positiveblue

[9rib-on-the-grind]

I think that's ok, we should return an error in the second case.

yes, but then there will never be any difference between --start_height {low_high} --end_height {high_height} and --start_height {high_height} --end_height {low_height}

I think this would be unclear: non-confirmed transactions first, and right after that very old transactions.

I think it would be better to sort by height and always put the non-confirmed first for example.

I guess this is the best solution.

We can leave the API consumers to do the last step and sort it however they want (while we always return them sorted by confirmation order) but then we should return an error if --start_height > --end_height + fix the docs.

Do you mean sorted by the number of confirmations, as it is done now? + error + docs

while we always return them sorted by confirmation order

[Chinwendu20]

I still think there is a use case for users to see confirmed transactions first before non-confirmed transactions i.e. start_height > end_height. I do not think we should return error in that case. We can just remove the bit where there is the same output in both cases. I guess that was the intention of this issue, just that when I saw something contrary boldly commented in the codebase, I thought that might be the behavior agreed upon. I am going through the commit history to see if there is a reason it was kept that way

[9rib-on-the-grind]

In this case, we should add an additional flag, this will be much clearer than controlling order by swapping start_height and end_height.

I still think there is a use case for users to see confirmed transactions first before non-confirmed transactions i.e.

As were proposed earlier.

I guess the most intuitive behavior would be:

• start_height <= 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)

But considering implementation details, I think it would be sufficient and more clear to add an error and change the docs.

// Sort transactions by number of confirmations rather than height

[Chinwendu20]

Oh okay I will implement it as discussed

[mohamedawnallah]

#7496