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

pull ben-kaufman wants to merge 1 commits into bitcoin:master from ben-kaufman:listtransactions-set-order changing 4 files +42 −5
  1. ben-kaufman commented at 2:50 am on August 23, 2021: contributor

    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:

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

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

    0bitcoin-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.

  2. fanquake added the label RPC/REST/ZMQ on Aug 23, 2021
  3. S3RK commented at 6:38 am on August 23, 2021: contributor

    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?

  4. ben-kaufman commented at 2:49 pm on August 23, 2021: contributor
    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.
  5. in src/wallet/rpcwallet.cpp:1417 in ddeff14d7d outdated 
    1413@@ -1414,6 +1414,7 @@ static RPCHelpMan listtransactions()
1414                     {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
1415                     {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
1416                     {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
1417+                    {"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"},
    benthecarman commented at 5:43 pm on August 23, 2021:
    Should probably say which is the default in the hint
    ben-kaufman commented at 8:05 pm on August 23, 2021:
    Updated
  6. benthecarman commented at 5:54 pm on August 23, 2021: contributor
    Needs a release note and should add a test
  7. ben-kaufman force-pushed on Aug 23, 2021
  8. ben-kaufman commented at 8:06 pm on August 23, 2021: contributor
    Added test and release note in 3f9112c6291cbc22a3e14bbc6f39f454b40032ca
  9. benthecarman approved
  10. benthecarman commented at 8:24 pm on August 23, 2021: contributor
    tACK 3f9112c6291cbc22a3e14bbc6f39f454b40032ca
  11. lsilva01 approved
  12. lsilva01 commented at 8:47 pm on August 23, 2021: contributor
    Tested ACK https://github.com/bitcoin/bitcoin/pull/22775/commits/3f9112c6291cbc22a3e14bbc6f39f454b40032ca on Ubuntu 20.04
  13. ghost commented at 9:15 am on August 24, 2021: none

    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)

  14. unknown approved
  16. kristapsk commented at 9:57 am on August 24, 2021: contributor
    Have you seen #19443? There skip is replaced with options, which could be used for various things, including reverse order.
  17. ghost commented at 10:46 am on August 24, 2021: none
    @kristapsk looks interesting. Will try today.
  18. ben-kaufman force-pushed on Aug 24, 2021
  19. ben-kaufman commented at 5:44 pm on August 24, 2021: contributor
    Updated with d07577b259aa1144e92e0572e4711e4d7c2fcdfe to minimise code duplication.
  20. Add option to go from oldest to newest in listtransactions d07577b259
  21. ben-kaufman force-pushed on Aug 24, 2021
  22. benthecarman approved
  23. benthecarman commented at 6:32 pm on August 24, 2021: contributor
    tACK d07577b259aa1144e92e0572e4711e4d7c2fcdfe
  24. benthecarman commented at 7:18 pm on August 24, 2021: contributor

    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

  25. S3RK commented at 6:27 am on August 25, 2021: contributor
    Concept ACK
  26. jamesob commented at 6:58 pm on August 27, 2021: member

    code ACK 3f9112c6291cbc22a3e14bbc6f39f454b40032ca, 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.

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3code 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))
 4
 5Code looks fine, tests were a little confusing but seem to be verifying the right thing. I built and ran functional tests locally.
 6
 7This seems like a fairly specific option relative to the functionality introduced in [#19443](/bitcoin-bitcoin/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.
 8
 9Since 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.
10-----BEGIN PGP SIGNATURE-----
11
12iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmEpNYsACgkQepNdrbLE
13TwWNBxAAjRm9Dq84h1dVLeWjB9QQUQHLC0MGh5YBRf033gP8oFqChyrQELvFHA0B
14QmT0bQtfq3JdRe+Y3XsgZ/SrcVhu4lQFFYbsY5DYAp14w9teW7VmZPNXrL23uXnn
157/D9Xg7kmggRBKnv896xKW5YIV+VSpvyQGw7lBGa8Tc/9yV8aUK6+/jTFo6j5kfo
16abLLOq2oqvhUkJ9s/akzpvONWXmhmfjgSuWLfa7TD6qnO1m2BhbWGkGeuPvONyPH
17vPs2vckxv4Mm6oBADTkb+Dsc9R8rDabETwnn4kvXwmMVOJpREfoQClxIvfMWfD47
18222DvQU4ClUVFxZzMWjN7v4nxNXf5VA5c1SVc/uTeAyCpG7NRb00C7hqC5JYtYut
19BEsoyPncH23X1y0JMWagUZkW/58xsvOjyvuM4tTTX5IfauPtoU+RadDzGRka5+qJ
201Fy/lrdq629oCrhvxDRI8lgFjVWRQpMBaRLYOCOPxTZhI04wm4PwCXjABBxCGWVt
21pApZ21KgJMa4h+oVy7mcVPany6XcaFQh+gKHm8zfg1Q//GINJhhChEWg52LxlMRa
22PUOgGv/TCar8pycY2ZXP5YDU2ON1yy4WXIc9Ygy7eu66pVZUJB9/2fvB/tTjD1SW
23pHX40BH63G6mnm4kaYuRi74c/+eSAAGSARafWX7qmz6fbT9tpIY=
24=89DY
25-----END PGP SIGNATURE-----

    0Tested on Linux-4.19.0-17-amd64-x86_64-with-glibc2.28
1
2Configured 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
3
4Compiled 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
5
6Compiler version: Debian clang version 11.1.0-++20210428103820+1fdec59bffc1-1~exp1~20210428204437.162
  27. kristapsk commented at 8:21 pm on August 27, 2021: contributor
    @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.
  28. in src/wallet/rpcwallet.cpp:1534 in d07577b259 
    1526@@ -1508,7 +1527,11 @@ static RPCHelpMan listtransactions()
1527 
1528     const std::vector<UniValue>& txs = ret.getValues();
1529     UniValue result{UniValue::VARR};
1530-    result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom }); // Return oldest to newest
1531+    if (ascending_order) {
1532+        result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom }); // Return oldest to newest
1533+    } else {
1534+        result.push_backV({ txs.cbegin() + nFrom, txs.cbegin() + nFrom + nCount }); // Return oldest to newest
1535+    }
    jonatack commented at 4:06 pm on September 15, 2021:
    • This comment should be removed
    0-    // ret is newest to oldest
1-
    • The second comment below seems incorrect (can probably drop them)
    • Use clang format
    0     if (ascending_order) {
1-        result.push_backV({ txs.rend() - nFrom - nCount, txs.rend() - nFrom }); // Return oldest to newest
2+        result.push_backV({txs.rend() - nFrom - nCount, txs.rend() - nFrom});
3     } else {
4-        result.push_backV({ txs.cbegin() + nFrom, txs.cbegin() + nFrom + nCount }); // Return oldest to newest
5+        result.push_backV({txs.cbegin() + nFrom, txs.cbegin() + nFrom + nCount});
6     }
  29. in src/wallet/rpcwallet.cpp:1417 in d07577b259 
    1413@@ -1414,6 +1414,7 @@ static RPCHelpMan listtransactions()
1414                     {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
1415                     {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
1416                     {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
1417+                    {"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"},
    jonatack commented at 4:08 pm on September 15, 2021:

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

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

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

    returns

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

    instead of

    05. 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
    luke-jr commented at 1:40 am on September 20, 2021:

    Maybe just a negative number for skip?

    At the very least, make it an options object…

  30. in src/wallet/rpcwallet.cpp:1492 in d07577b259 
    1483@@ -1483,19 +1484,37 @@ static RPCHelpMan listtransactions()
1484     if (nFrom < 0)
1485         throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative from");
1486 
1487+    // Default ascending_order to true if no value has been specified
1488+    bool ascending_order = true;
1489+
1490+    if (!request.params[4].isNull()) {
1491+        ascending_order = request.params[4].get_bool();
1492+    }
    jonatack commented at 4:10 pm on September 15, 2021:

    suggestion

    0-    // Default ascending_order to true if no value has been specified
1-    bool ascending_order = true;
2-
3-    if (!request.params[4].isNull()) {
4-        ascending_order = request.params[4].get_bool();
5-    }
6+    const bool ascending_order{request.params[4].isNull() ? true : request.params[4].get_bool()};
  31. in doc/release-notes-22775.md:5 in d07577b259 
    0@@ -0,0 +1,5 @@
1+New RPCs
2+--------
3+- A new optional ascending_order flag in the `listtransactions` RPC
4+will allow to go through the transactions list from the oldest to
5+the newest in addition to the current newest to oldest behavior. (#22775)
    jonatack commented at 4:55 pm on September 15, 2021:

    suggestion

    0 New RPCs
1 --------
2-- A new optional ascending_order flag in the `listtransactions` RPC
3-will allow to go through the transactions list from the oldest to
4-the newest in addition to the current newest to oldest behavior. (#22775)
5+
6+- RPC `listtransactions` has a new optional `ascending_order` argument that
7+  allows selecting whether to return transactions in the default order of
8+  ascending (oldest to newest) or descending (newest to oldest). (#22775)
  32. in test/functional/wallet_listtransactions.py:112 in d07577b259 
    107+        # The first transaction counting from the oldest (descending order) should be the last transaction counting from the newest (ascending order)
108+        assert_equal(self.nodes[0].listtransactions(count=1, ascending_order=False),
109+                     self.nodes[0].listtransactions(count=1, skip=len(self.nodes[0].listtransactions(count=10000)) - 1, ascending_order=True))
110+        # 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)
111+        assert_equal(self.nodes[0].listtransactions(count=2, skip=1, ascending_order=False),
112+                     self.nodes[0].listtransactions(count=2, skip=len(self.nodes[0].listtransactions(count=10000)) - 3, ascending_order=True))
    jonatack commented at 5:07 pm on September 15, 2021:

    suggestions

     0-        # The first transaction counting from the oldest (descending order) should be the last transaction counting from the newest (ascending order)
 1+        size = len(self.nodes[0].listtransactions(count=10000))  # get size of transactions list
 2+        # The first transaction in descending order should be the last transaction in ascending order.
 3         assert_equal(self.nodes[0].listtransactions(count=1, ascending_order=False),
 4-                     self.nodes[0].listtransactions(count=1, skip=len(self.nodes[0].listtransactions(count=10000)) - 1, ascending_order=True))
 5-        # 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)
 6+                     self.nodes[0].listtransactions(count=1, ascending_order=True, skip=size - 1))
 7+        # The second and third txns in descending order should be the 2 before the last txn in ascending order.
 8         assert_equal(self.nodes[0].listtransactions(count=2, skip=1, ascending_order=False),
 9-                     self.nodes[0].listtransactions(count=2, skip=len(self.nodes[0].listtransactions(count=10000)) - 3, ascending_order=True))
10+                     self.nodes[0].listtransactions(count=2, ascending_order=True, skip=size - 3))
  33. jonatack commented at 5:11 pm on September 15, 2021: contributor
    I’m not sure if this is the best way to do this, but some review suggestions follow.
  34. luke-jr changes_requested
  35. DrahtBot commented at 2:29 pm on November 15, 2021: contributor

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

    Conflicts

    No conflicts as of last run.

  36. DrahtBot added the label Needs rebase on Dec 8, 2021
  37. DrahtBot commented at 5:22 am on December 8, 2021: contributor

    🐙 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”.

  38. DrahtBot commented at 9:12 am on January 3, 2022: contributor
    • 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.
  39. DrahtBot commented at 7:03 am on July 25, 2022: contributor
    • 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.
  40. MarcoFalke commented at 7:16 am on July 25, 2022: member
    Closing for now. Let us know when you are working on this again and if it should be reopened.
  41. MarcoFalke closed this on Jul 25, 2022
  42. bitcoin locked this on Jul 25, 2023


ben-kaufman S3RK benthecarman lsilva01 ghost kristapsk jamesob jonatack luke-jr DrahtBot MarcoFalke

Labels
RPC/REST/ZMQ Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me