rpc listtransactions new argument options (paginatebypointer impl)

hosseinzoda commented at 11:54 am on December 8, 2018: none

Added fifth param to listtransactions named options, options may be an object containing paginatebypointer (boolean default: false) and nextpagepointer (string default: OMITTED).

With paginatebypointer output will have the following changes.

Return transactions is ordered by most recent transactions. Though the default does reverse the order after transactions are fetched and clipped.
skip argument has no effect. Instead nextpagepointer will be used for pagination.
Return value is an object containing, records (array of txs) and nextpagepointer (string)

fanquake added the label Wallet on Dec 8, 2018

fanquake added the label RPC/REST/ZMQ on Dec 8, 2018

in src/wallet/rpcwallet.cpp:1651 in 0796ebfdf9 outdated

1643+            "5. include_watchonly (bool, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')\n"
1644+            "\nResult:\n"
1645+            "[\n"
1646+            "  {\n"
1647+            "    \"address\":\"address\",    (string) The bitcoin address of the transaction.\n"
1648+            "    \"category\":\"send|receive\", (string) The transaction category.\n"

andrewtoth commented at 4:14 pm on December 8, 2018:

There are more categories than just send and receive. See https://github.com/bitcoin/bitcoin/pull/14653

hosseinzoda commented at 5:36 pm on December 8, 2018:

I was using branch 0.17 , and when i upgraded to master. help file was from the old one. will fix

hosseinzoda commented at 5:42 pm on December 8, 2018:

I see original lsittransactions only contains send|receive. @andrewtoth Please inform me. What are the other values. I guess there’s internal.

meshcollider commented at 7:53 pm on December 8, 2018:

The other 3 are generate, immature and orphan.

hosseinzoda commented at 8:37 pm on December 8, 2018:

okay. I have made the change. i will add it after review.

hosseinzoda force-pushed on Dec 8, 2018

hosseinzoda force-pushed on Dec 9, 2018

promag commented at 6:27 pm on December 9, 2018: member

Is there a strong reason to not improve existing listtransactions without being a breaking change?

hosseinzoda commented at 8:51 pm on December 9, 2018: none

@promag listtransactions has been used in many tests. I don’t know what will happen. If change number one gets added to it.

Return transactions is ordered by most recent transactions. Though the original rpc does reverse the order after transactions are fetched and clipped.

There’s an odd reverse call at listtransactions which is not in this one. https://github.com/bitcoin/bitcoin/blob/fc1710fbf3ce840f0b647913be8b821089b00f5e/src/wallet/rpcwallet.cpp#L1494

Other than that adding nextpagepointer will break compatibility with listtransactions params. Only if one accepts it as interger and string and keeps the behaviour the same.

jonasschnelli commented at 8:56 pm on December 9, 2018: contributor

I general I like the idea of a pointer (seems more reliable) and the sort. Not sure about cloning the call.

kristapsk commented at 9:04 pm on December 9, 2018: contributor

@promag listtransactions has been used in many tests. I don’t know what will happen. If change number one gets added to it.

I would not worry about tests, you should anyway run the functional tests yourself if you change something in RPC. Sort order could be added as an additional optional boolean parameter IMO. As well as other changes.

hosseinzoda commented at 9:09 pm on December 9, 2018: none

Who thinks this line should get removed from listtransactions?

https://github.com/bitcoin/bitcoin/blob/fc1710fbf3ce840f0b647913be8b821089b00f5e/src/wallet/rpcwallet.cpp#L1494

sipa commented at 10:12 pm on December 9, 2018: member

@hosseinamin Well we can’t remove that line; there are likely end users who rely on its behavior, even if it is suboptimal. We can make it optional by adding an extra flag, though.

kristapsk commented at 10:22 pm on December 9, 2018: contributor

Breaking userspace is bad, so changing default behaviour of any RPC should be avoided.

hosseinzoda commented at 10:47 pm on December 9, 2018: none

That’s why i did cloned the rpc call. This is how new functionality can come in without messing with userspace.

jonasschnelli commented at 10:53 pm on December 9, 2018: contributor

That’s why i did cloned the rpc call. This is how new functionality can come in without messing with userspace.

Cloning is one way, backward compatible parameterising would be another (see how the account system was removed).

hosseinzoda commented at 10:56 pm on December 9, 2018: none

Okay. How’s this?

skip param can morph to nextpagepointer when string is given.
adding nonlegacy parameter at the end to avoid messing with userspace.

hosseinzoda commented at 10:59 pm on December 9, 2018: none

I think skip accepts string as input. It will convert to integer. A bit more work is needed to detect between the two.

hosseinzoda commented at 0:22 am on December 10, 2018: none

It’s also seems like changes on ListTransactions2 is trivial. ListTransactions with no change works.

hosseinzoda force-pushed on Dec 10, 2018

hosseinzoda commented at 4:08 am on December 10, 2018: none

@jonasschnelli Removed listransactions2, And did parametrize it. I will try later to write some tests for it.

hosseinzoda force-pushed on Dec 10, 2018

in src/wallet/rpcwallet.cpp:1510 in d788aae9c5 outdated

1510+                    }
1511+                    nextHash = uint256S(s);
1512+                } else if (i == 1) {
1513+                    try {
1514+                        nextVOut = std::stoul(s);
1515+                    } catch (std::invalid_argument &e) {

practicalswift commented at 7:13 pm on December 10, 2018:

Drop &e? Applies to the three cases below as well :-)

kristapsk commented at 9:16 pm on December 10, 2018: contributor

I’m not sure a single parameter that changes multiple things is a right way to go. Wouln’t be better to be able to specify sorting order and paging behaviour independently? Besides I don’t like “old behaviour vs new behaviour” boolean also because in future there could be a different one “new new behaviour”.

in src/rpc/client.cpp:60 in d788aae9c5 outdated

56@@ -57,7 +57,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
57     { "waitfornewblock", 0, "timeout" },
58     { "listtransactions", 1, "count" },
59     { "listtransactions", 2, "skip" },
60+    { "listtransactions", 2, "nextpagepointer" },

practicalswift commented at 9:27 pm on December 10, 2018:

Nit: Isn’t this typically called an index rather than a pointer?

in src/rpc/client.cpp:62 in d788aae9c5 outdated

56@@ -57,7 +57,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
57     { "waitfornewblock", 0, "timeout" },
58     { "listtransactions", 1, "count" },
59     { "listtransactions", 2, "skip" },
60+    { "listtransactions", 2, "nextpagepointer" },
61     { "listtransactions", 3, "include_watchonly" },
62+    { "listtransactions", 4, "newversion" },

practicalswift commented at 9:28 pm on December 10, 2018:

Nit: The newversion won’t be new forever :-) Can you come up with a time independent name?

DrahtBot commented at 1:28 am on December 11, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16240 (JSONRPCRequest-aware RPCHelpMan by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hosseinzoda commented at 11:10 am on December 11, 2018: none

Okay. I agree. Want to change newversion to flags. Though don’t have time to do it now.

in src/wallet/rpcwallet.cpp:1308 in d788aae9c5 outdated

1293@@ -1294,8 +1294,13 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
1294  * @param  ret            The UniValue into which the result is stored.
1295  * @param  filter_ismine  The "is mine" filter flags.
1296  * @param  filter_label   Optional label string to filter incoming transactions.
1297+ * @param  afterVOut      start adding after afterVOut

practicalswift commented at 7:52 am on December 12, 2018:

afterVOut is not part of the declaration?

hosseinzoda commented at 6:47 pm on December 13, 2018:

Should get removed. Left out from some dead coding.

laanwj commented at 1:10 pm on December 13, 2018: member

@hosseinamin Can you please update the PR title and opening post to cover the current changes?

hosseinzoda renamed this:
~~new wallet rpc listtransactions2~~
nextpagepointer & list ordering options for listtransactions
on Dec 13, 2018

hosseinzoda commented at 6:45 pm on December 13, 2018: none

@hosseinamin Can you please update the PR title and opening post to cover the current changes? @laanwj Where can i open a post? (i did edit the current description)

laanwj commented at 11:36 am on January 16, 2019: member

@laanwj Where can i open a post? (i did edit the current description)

Thanks. That’s what I meant, with “opening post” I mean the first post.

laanwj commented at 11:40 am on January 16, 2019: member

Travis error:

0The locale dependent function stoul(...) appears to be used:
1src/wallet/rpcwallet.cpp:                        nextVOut = std::stoul(s);
2src/wallet/rpcwallet.cpp:                        nextIsSend = std::stoul(s) == 1;

PLease use our own functions ParseInt32 or ParseInt64 instead.

hosseinzoda force-pushed on Jan 21, 2019

hosseinzoda commented at 9:25 pm on January 21, 2019: none

I did add some test transaction which did catch a bug in my code.

hosseinzoda force-pushed on Jan 21, 2019

hosseinzoda renamed this:
~~nextpagepointer & list ordering options for listtransactions~~
nextpagepointer flag for listtransactions
on Jan 22, 2019

laanwj commented at 12:45 pm on January 22, 2019: member

Linter error from travis:

0This diff appears to have added new lines with trailing whitespace.
1The following changes were suspected:
2diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
3@@ -1455,2 +1492,113 @@ UniValue listtransactions(const JSONRPCRequest& request)
4+
5+
6+
7+
8+
9^---- failure generated from test/lint/lint-whitespace.sh

hosseinzoda force-pushed on Jan 22, 2019

hosseinzoda commented at 9:45 pm on January 23, 2019: none

This PR needs review.

DrahtBot added the label Needs rebase on Jan 29, 2019

hosseinzoda force-pushed on Jan 30, 2019

DrahtBot removed the label Needs rebase on Jan 30, 2019

in src/wallet/rpcwallet.cpp:1439 in 52a493b104 outdated

1433@@ -1403,10 +1434,15 @@ UniValue listtransactions(const JSONRPCRequest& request)
1434                     {"label", RPCArg::Type::STR, /* opt */ true, /* default_val */ "null", "If set, should be a valid label name to return only incoming transactions\n"
1435             "              with the specified label, or \"*\" to disable filtering and return all transactions."},
1436                     {"count", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "10", "The number of transactions to return"},
1437-                    {"skip", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0", "The number of transactions to skip"},
1438+                    {"skip", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0", "The number of transactions to skip, In case of flags contain nextpagepointer, fThe value of it is a string given by listtransactions."},
1439                     {"include_watchonly", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "Include transactions to watch-only addresses (see 'importaddress')"},
1440+                    {"flags", RPCArg::Type::STR, /* opt */ true, /* default_val */ "", "list of flags separated by | which they are [ nextpagepointer ]"},

MarcoFalke commented at 8:55 pm on February 7, 2019:

nit: I think we either use a json object called options or a named argument directly. A specially serialized flags seems inappropriate given that we have all json types and named arguments available.

DrahtBot added the label Needs rebase on Feb 8, 2019

hosseinzoda force-pushed on May 22, 2019

hosseinzoda renamed this:
~~nextpagepointer flag for listtransactions~~
rpc listtransactions new argument options (paginatebypointer impl)
on May 22, 2019

hosseinzoda force-pushed on May 22, 2019

DrahtBot removed the label Needs rebase on May 22, 2019

hosseinzoda force-pushed on May 22, 2019

hosseinzoda commented at 8:51 pm on May 22, 2019: none

Hey guys. This commit is ready for merge.

in src/wallet/rpcwallet.cpp:1434 in b3df008386 outdated

1430@@ -1400,10 +1431,20 @@ UniValue listtransactions(const JSONRPCRequest& request)
1431                     {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
1432             "              with the specified label, or \"*\" to disable filtering and return all transactions."},
1433                     {"count", RPCArg::Type::NUM, /* default */ "10", "The number of transactions to return"},
1434-                    {"skip", RPCArg::Type::NUM, /* default */ "0", "The number of transactions to skip"},
1435+                    {"skip", RPCArg::Type::NUM, /* default */ "0", "The number of transactions to skip, If \"usesnextpagepointer\" option is true, skip wont have an effect."},

practicalswift commented at 7:41 am on May 23, 2019:

“Wont” whould be “won’t” :-)

hosseinzoda force-pushed on May 24, 2019

DrahtBot added the label Needs rebase on May 24, 2019

rpc listtransactions new argument options (paginatebypointer impl)

Added fifth param to `listtransactions` named `options`, `options` may be an object containing `paginatebypointer` (boolean default: false) and `nextpagepointer` (string default: OMITTED).

With `paginatebypointer` output will have the following changes.

1. Return transactions is ordered by most recent transactions. Though the default does reverse the order after transactions are fetched and clipped.
2. `skip` argument has no effect. Instead `nextpagepointer` will be used for pagination.
3. Return value is an object containing, records (array of txs) and nextpagepointer (string)

8934f247c6

hosseinzoda force-pushed on May 24, 2019

DrahtBot removed the label Needs rebase on May 24, 2019

DrahtBot commented at 11:41 pm on July 9, 2019: member

DrahtBot added the label Needs rebase on Jul 9, 2019

adamjonas commented at 1:19 am on May 19, 2020: member

Checking in on this. It’s been a while since there was a rebase and there doesn’t appear to be strong conviction from the reviewers on the concept/approach.

hosseinzoda commented at 7:59 am on May 19, 2020: none

I wasted my time on this pull-request. No need for the merge anymore. Lets dump it in garbage.

hosseinzoda closed this on May 19, 2020

kristapsk commented at 12:10 pm on May 19, 2020: contributor

I still think this would have been a good functionality conceptually.

DrahtBot locked this on Feb 15, 2022

rpc listtransactions new argument options (paginatebypointer impl) #14898

Conflicts