rpc: Add option to list transactions from oldest to newest in `listtransactions` RPC command #22775

ben-kaufman · 2021-08-23T02:50:21Z

Currently, the RPC listtransactions command returns the latest X transactions of the wallet starting from Y. This means that to get the X oldest transacions would require first to call getwalletinfo command to get the total transactions count, and then use that to know what count to pass the listtransactions command.

This PR adds an optional flag to allow instead to start going through the transactions list from the oldest to the newest, so it will be possible to get the oldest X transactions of the wallet starting from Y.

For example, this will return the oldest transaction of the wallet:

bitcoin-cli -rpcuser=bitcoin -rpcpassword=secret -rpcwallet= listtransactions "*" 1 0 true false

This will return the 2nd and 3rd oldest transactions of the wallet:

bitcoin-cli -rpcuser=bitcoin -rpcpassword=secret -rpcwallet= listtransactions "*" 2 1 true false

Not including the new flag or setting it to true will not change the current behavior.

S3RK · 2021-08-23T06:38:12Z

I think the underlying RPCs of the console commands are primarily for machines and not for humans. So it seems it's not a big deal to make two calls, and it might be even preferable than adding more parameters.

Why is it important for the desired use case to work from console rather than from GUI?

ben-kaufman · 2021-08-23T14:49:35Z

There are cases where the RPC is used remotely, for example as a hidden service exposed over Tor. Then latency can be very bad and every call takes a long time, so reducing the number of calls is very important in these cases. In Specter, for example, we have many such users connecting to a remote Bitcoin Core over Tor, and so have to try and reduce the number of separate calls to the minimum.

benthecarman

Needs a release note and should add a test

benthecarman · 2021-08-23T17:43:30Z

src/wallet/rpcwallet.cpp

@@ -1414,6 +1414,7 @@ static RPCHelpMan listtransactions()
                    {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
                    {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
                    {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
+                    {"ascending_order", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for ascending order (newest to oldest), false for descending order (oldest to newest)"}, "The direction from which to start getting transactions"},


Should probably say which is the default in the hint

ben-kaufman · 2021-08-23T20:06:30Z

Added test and release note in 3f9112c

benthecarman

tACK 3f9112c

lsilva01

Tested ACK 3f9112c on Ubuntu 20.04

ghost · 2021-08-24T09:15:07Z

I think the underlying RPCs of the console commands are primarily for machines and not for humans. So it seems it's not a big deal to make two calls, and it might be even preferable than adding more parameters.

Agree with this

have to try and reduce the number of separate calls to the minimum.

This also makes sense

Since not including the new flag or setting it to true will not change the current behavior, its an improvement to listtransactions which can be helpful for some users.

Example: #22608 (comment)

ghost

utACK 3f9112c

kristapsk · 2021-08-24T09:57:46Z

Have you seen #19443? There skip is replaced with options, which could be used for various things, including reverse order.

ghost · 2021-08-24T10:46:24Z

@kristapsk looks interesting. Will try today.

ben-kaufman · 2021-08-24T17:44:03Z

Updated with d07577b to minimise code duplication.

benthecarman

tACK d07577b

benthecarman · 2021-08-24T19:18:56Z

Have you seen #19443? There skip is replaced with options, which could be used for various things, including reverse order.

Looks like it needs rebase

S3RK · 2021-08-25T06:27:46Z

Concept ACK

jamesob

code ACK 3f9112c, unsure on concept (jamesob/ackr/22775.1.ben-kaufman.rpc_add_option_to_list_t)

Code looks fine, tests were a little confusing but seem to be verifying the right thing. I built and ran functional tests locally.

This seems like a fairly specific option relative to the functionality introduced in #19443. My gut inclination would be that if we're going to change the RPC API, it would make sense to put thought into something that might allow more general sorting in the way the other PR does, though the benefit here is that this change is much simpler.

Since I don't myself have a great sense of what wallet providers would find useful, it'd be nice to get commentary from multiple external projects (e.g. Spectre, OP's project) about whether they would find this particular change worth special-casing instead of something more general, since we won't want to deprecate an arg like this shortly after introducing it if another sorting ask comes around.

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

code ACK 3f9112c6291cbc22a3e14bbc6f39f454b40032ca ([`jamesob/ackr/22775.1.ben-kaufman.rpc_add_option_to_list_t`](https://github.com/jamesob/bitcoin/tree/ackr/22775.1.ben-kaufman.rpc_add_option_to_list_t))

Code looks fine, tests were a little confusing but seem to be verifying the right thing. I built and ran functional tests locally.

This seems like a fairly specific option relative to the functionality introduced in #19443. My gut inclination would be that if we're going to change the RPC API, it would make sense to put thought into something that might allow more general sorting in the way the other PR does, though the benefit here is that this change is much simpler.

Since I don't myself have a great sense of what wallet providers would find useful, it'd be nice to get commentary from multiple external projects (e.g.  Spectre, OP's project) about whether they would find this particular change worth special-casing instead of something more general, since we won't want to deprecate an arg like this shortly after introducing it if another sorting ask comes around.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmEpNYsACgkQepNdrbLE
TwWNBxAAjRm9Dq84h1dVLeWjB9QQUQHLC0MGh5YBRf033gP8oFqChyrQELvFHA0B
QmT0bQtfq3JdRe+Y3XsgZ/SrcVhu4lQFFYbsY5DYAp14w9teW7VmZPNXrL23uXnn
7/D9Xg7kmggRBKnv896xKW5YIV+VSpvyQGw7lBGa8Tc/9yV8aUK6+/jTFo6j5kfo
abLLOq2oqvhUkJ9s/akzpvONWXmhmfjgSuWLfa7TD6qnO1m2BhbWGkGeuPvONyPH
vPs2vckxv4Mm6oBADTkb+Dsc9R8rDabETwnn4kvXwmMVOJpREfoQClxIvfMWfD47
222DvQU4ClUVFxZzMWjN7v4nxNXf5VA5c1SVc/uTeAyCpG7NRb00C7hqC5JYtYut
BEsoyPncH23X1y0JMWagUZkW/58xsvOjyvuM4tTTX5IfauPtoU+RadDzGRka5+qJ
1Fy/lrdq629oCrhvxDRI8lgFjVWRQpMBaRLYOCOPxTZhI04wm4PwCXjABBxCGWVt
pApZ21KgJMa4h+oVy7mcVPany6XcaFQh+gKHm8zfg1Q//GINJhhChEWg52LxlMRa
PUOgGv/TCar8pycY2ZXP5YDU2ON1yy4WXIc9Ygy7eu66pVZUJB9/2fvB/tTjD1SW
pHX40BH63G6mnm4kaYuRi74c/+eSAAGSARafWX7qmz6fbT9tpIY=
=89DY
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-4.19.0-17-amd64-x86_64-with-glibc2.28

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare --enable-wallet --with-daemon CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang --with-bignum=no PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -fdebug-prefix-map=$(abs_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: Debian clang version 11.1.0-++20210428103820+1fdec59bffc1-1~exp1~20210428204437.162

kristapsk · 2021-08-27T20:21:07Z

@jamesob Functionality that could be used for this PR ar now split off from #19443 as #22807, which is pretty simple. Universal options argument was why I mentioned #19443 here in first place, not whole paginatebypointer thing.

jonatack

I'm not sure if this is the best way to do this, but some review suggestions follow.

jonatack · 2021-09-15T16:06:01Z

src/wallet/rpcwallet.cpp

+        result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom }); // Return oldest to newest
+    } else {
+        result.push_backV({ txs.cbegin() + nFrom, txs.cbegin() + nFrom + nCount }); // Return oldest to newest
+    }


This comment should be removed

- // ret is newest to oldest -

The second comment below seems incorrect (can probably drop them)

Use clang format

if (ascending_order) { - result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom }); // Return oldest to newest + result.push_backV({txs.rend() - nFrom - nCount, txs.rend() - nFrom}); } else { - result.push_backV({ txs.cbegin() + nFrom, txs.cbegin() + nFrom + nCount }); // Return oldest to newest + result.push_backV({txs.cbegin() + nFrom, txs.cbegin() + nFrom + nCount}); }

jonatack · 2021-09-15T16:08:19Z

src/wallet/rpcwallet.cpp

@@ -1414,6 +1414,7 @@ static RPCHelpMan listtransactions()
                    {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
                    {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
                    {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
+                    {"ascending_order", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for ascending order (newest to oldest), false for descending order (oldest to newest), default is true"}, "The direction from which to start getting transactions"},


Subjective: calling the argument ascending would be shorter and just as clear to me.

The following is simpler and would print a help that seems clearer and easier to understand:

Suggested change

{"ascending_order", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for ascending order (newest to oldest), false for descending order (oldest to newest), default is true"}, "The direction from which to start getting transactions"},

{"ascending_order", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether to list transactions by ascending or descending order"},

returns

5. ascending (boolean, optional, default=true) Whether to list transactions by ascending or descending order

instead of

5. ascending_order (boolean, optional, default=true for ascending order (newest to oldest), false for descending order (oldest to newest), default is true) The direction from which to start getting transactions

Maybe just a negative number for skip?

At the very least, make it an options object...

jonatack · 2021-09-15T16:10:43Z

src/wallet/rpcwallet.cpp

+
+    if (!request.params[4].isNull()) {
+        ascending_order = request.params[4].get_bool();
+    }


suggestion

- // Default ascending_order to true if no value has been specified - bool ascending_order = true; - - if (!request.params[4].isNull()) { - ascending_order = request.params[4].get_bool(); - } + const bool ascending_order{request.params[4].isNull() ? true : request.params[4].get_bool()};

jonatack · 2021-09-15T16:55:31Z

doc/release-notes-22775.md

+--------
+- A new optional ascending_order flag in the `listtransactions` RPC
+will allow to go through the transactions list from the oldest to
+the newest in addition to the current newest to oldest behavior. (#22775)


suggestion

New RPCs -------- -- A new optional ascending_order flag in the `listtransactions` RPC -will allow to go through the transactions list from the oldest to -the newest in addition to the current newest to oldest behavior. (#22775) + +- RPC `listtransactions` has a new optional `ascending_order` argument that + allows selecting whether to return transactions in the default order of + ascending (oldest to newest) or descending (newest to oldest). (#22775)

jonatack · 2021-09-15T17:07:39Z

test/functional/wallet_listtransactions.py

+                     self.nodes[0].listtransactions(count=1, skip=len(self.nodes[0].listtransactions(count=10000)) - 1, ascending_order=True))
+        # The second and third transactions counting from the oldest (descending order) should be the 2 before the last transaction counting from the newest (ascending order)
+        assert_equal(self.nodes[0].listtransactions(count=2, skip=1, ascending_order=False),
+                     self.nodes[0].listtransactions(count=2, skip=len(self.nodes[0].listtransactions(count=10000)) - 3, ascending_order=True))


suggestions

- # The first transaction counting from the oldest (descending order) should be the last transaction counting from the newest (ascending order) + size = len(self.nodes[0].listtransactions(count=10000)) # get size of transactions list + # The first transaction in descending order should be the last transaction in ascending order. assert_equal(self.nodes[0].listtransactions(count=1, ascending_order=False), - self.nodes[0].listtransactions(count=1, skip=len(self.nodes[0].listtransactions(count=10000)) - 1, ascending_order=True)) - # The second and third transactions counting from the oldest (descending order) should be the 2 before the last transaction counting from the newest (ascending order) + self.nodes[0].listtransactions(count=1, ascending_order=True, skip=size - 1)) + # The second and third txns in descending order should be the 2 before the last txn in ascending order. assert_equal(self.nodes[0].listtransactions(count=2, skip=1, ascending_order=False), - self.nodes[0].listtransactions(count=2, skip=len(self.nodes[0].listtransactions(count=10000)) - 3, ascending_order=True)) + self.nodes[0].listtransactions(count=2, ascending_order=True, skip=size - 3))

luke-jr · 2021-09-20T01:40:29Z

src/wallet/rpcwallet.cpp

@@ -1414,6 +1414,7 @@ static RPCHelpMan listtransactions()
                    {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
                    {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
                    {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
+                    {"ascending_order", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for ascending order (newest to oldest), false for descending order (oldest to newest), default is true"}, "The direction from which to start getting transactions"},


Maybe just a negative number for skip?

At the very least, make it an options object...

DrahtBot · 2021-11-15T14:29:42Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

DrahtBot · 2021-12-08T05:22:56Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2022-01-03T09:12:28Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2022-07-25T07:03:24Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2022-07-25T07:16:50Z

Closing for now. Let us know when you are working on this again and if it should be reopened.

fanquake added the RPC/REST/ZMQ label Aug 23, 2021

benthecarman reviewed Aug 23, 2021

View reviewed changes

ben-kaufman force-pushed the listtransactions-set-order branch from ddeff14 to 3f9112c Compare August 23, 2021 20:05

benthecarman approved these changes Aug 23, 2021

View reviewed changes

lsilva01 approved these changes Aug 23, 2021

View reviewed changes

ghost approved these changes Aug 24, 2021

View reviewed changes

ben-kaufman force-pushed the listtransactions-set-order branch from 3f9112c to 024439d Compare August 24, 2021 17:43

Add option to go from oldest to newest in listtransactions

d07577b

ben-kaufman force-pushed the listtransactions-set-order branch from 024439d to d07577b Compare August 24, 2021 17:47

benthecarman approved these changes Aug 24, 2021

View reviewed changes

This was referenced Aug 25, 2021

rpc: Add options argument to listtransactions, with paginatebypointer options #19443

Closed

RPC: Add universal options argument to listtransactions #22807

Closed

jamesob reviewed Aug 27, 2021

View reviewed changes

jonatack reviewed Sep 15, 2021

View reviewed changes

luke-jr suggested changes Sep 20, 2021

View reviewed changes

DrahtBot mentioned this pull request Nov 15, 2021

Refactor: Improve API design of ScriptToUniv, TxToUniv etc to return the UniValue instead of mutating a parameter/reference #23507

Closed

This was referenced Dec 2, 2021

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

Split up rpcwallet #23667

Merged

DrahtBot added the Needs rebase label Dec 8, 2021

maflcko closed this Jul 25, 2022

bitcoin locked and limited conversation to collaborators Jul 25, 2023

	{"ascending_order", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for ascending order (newest to oldest), false for descending order (oldest to newest), default is true"}, "The direction from which to start getting transactions"},
	{"ascending_order", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether to list transactions by ascending or descending order"},

rpc: Add option to list transactions from oldest to newest in listtransactions RPC command #22775

rpc: Add option to list transactions from oldest to newest in listtransactions RPC command #22775

Uh oh!

Conversation

ben-kaufman commented Aug 23, 2021

Uh oh!

S3RK commented Aug 23, 2021

Uh oh!

ben-kaufman commented Aug 23, 2021

Uh oh!

benthecarman left a comment

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!

ben-kaufman commented Aug 23, 2021

Uh oh!

benthecarman left a comment

Choose a reason for hiding this comment

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 24, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Aug 24, 2021

Uh oh!

ghost commented Aug 24, 2021

Uh oh!

ben-kaufman commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman left a comment

Choose a reason for hiding this comment

Uh oh!

benthecarman commented Aug 24, 2021

Uh oh!

S3RK commented Aug 25, 2021

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Dec 8, 2021

Uh oh!

DrahtBot commented Jan 3, 2022

Uh oh!

DrahtBot commented Jul 25, 2022

Uh oh!

maflcko commented Jul 25, 2022

Uh oh!

Uh oh!

rpc: Add option to list transactions from oldest to newest in `listtransactions` RPC command #22775

rpc: Add option to list transactions from oldest to newest in `listtransactions` RPC command #22775

ben-kaufman commented Aug 24, 2021 •

edited

Loading

kristapsk commented Aug 27, 2021 •

edited

Loading

DrahtBot commented Nov 15, 2021 •

edited

Loading