Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet-rpc: switch the order of transactions returned to most recent #475

Closed

Conversation

kilpatty
Copy link
Contributor

Re: discussion in the Telegram group. Looks like the intended behavior of this RPC call is to return the most recent transactions first, and then the "from/skip" parameter jumps ahead x number of txs.

@@ -1232,7 +1232,7 @@ class RPC extends RPCBase {
const valid = new Validator(args);
let name = valid.str(0);
const count = valid.u32(1, 10);
const from = valid.u32(2, 0);
const skip = valid.u32(2, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also change the argument name in the help message up on line 1228

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, adding this in

const to = Math.min(end, txs.length);
const start = txs.length - skip - 1;
const end = start - count;
const to = Math.max(end, -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer a 0 here and a >= below

@pinheadmz
Copy link
Member

pinheadmz commented Jul 22, 2020

See bcoin-org/bcoin#605 -- this rpc call is already problematic for two reasons:

  1. It's not paginated, so a wallet with a huge history is going to timeout anyway
  2. Transactions in the txdb aren't necessarily stored by time, this is a big part of what Wallet TX count and time indexing bcoin-org/bcoin#605 is fixing above.

So although I think this PR is useful and makes the API match Bitcoin Core rpc listtransactions it should be noted that its suboptimal.

Also, if we merge this we need to add a message to the CHANGELOG and update the api-docs.

@kilpatty
Copy link
Contributor Author

kilpatty commented Aug 7, 2020

Following up here, I think without pagination, even these sight edits are somewhat useless when it comes to a wallet with a large amount of transactions.

I'm currently seeing roughly 1-2 second response times for this RPC call, and so I think the next step would be to mimic bitcoin core and introduce some kind of pagination. I'll need to do some reading to see how they do it, and if it will require a change to how the transactions are indexed in the walletDB, but will report back here when I get the time to investigate.

@pinheadmz
Copy link
Member

Be sure to check out bcoin #605, Braydon wrote pagination stuff already, it just never got reviewed or merged.

@pinheadmz
Copy link
Member

@kilpatty should we close this and work on a deeper fix instead?

@kilpatty
Copy link
Contributor Author

kilpatty commented Aug 10, 2020

I might actually pull the edits from bcoin #605 into this PR and rename. This is a relatively high priority for me as I need it for some internal software, so I'll try to get on it tonight and/or tomorrow. If you want me to just pop it into a new one happy to do that as well.

EDIT: Also I noticed there are further improvements to pagination in bitcoin core recently so I will likely snag those in here as well.

@pinheadmz
Copy link
Member

Ok cool, up to you on which branch you push to :-)

@nodech nodech added quick review difficulty - easy wallet-rpc part of the codebase waiting for the author modifier labels Dec 9, 2021
@nodech nodech closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick review difficulty - easy waiting for the author modifier wallet-rpc part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants