Fix rpc::CoreTxIter logic.

[evanlinjin]

Description

This fixes a bug where CoreTxIter attempts to call listtransactions immediately after a tx result is filtered (instead of being returned), when in fact, the correct logic will be to pop another tx result.

The new logic also ensures that tx results are returned in chonological order. The test test_list_transactions verifies this. We also now ensure that page_size is between the range [0 to 1000] otherwise an error is returned.

Some needless cloning is removed from from_config as well as logging improvements.

Notes to the reviewers

This is an oversight by me (sorry) for PR #683

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

* [ ] This pull request breaks the existing API

• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[afilini]

Can you come up with an example that would break without this PR (i.e. without iterating in chronological order)? You don't have to write it in code right now, just describe it. We can add it later to the blockchain tests so that we can cover more "edge-cases".

[evanlinjin]

@afilini the "chronological order" is just a good to have.

The real fix is this:

There was also a bug where CoreTxIter will attempt to call listtransactions again when a tx result is filtered (the correct logic would be to pop another tx result).

Sorry I should have made it more clear.

It just so happened that the test I wrote for the iterator didn't work initially (and I didn't know why) but then I realized that with different page sizes results were returned in different orders so I decided to change the logic to make things more predictable.

[evanlinjin]

@afilini I just updated the PR description and commit message to better describe the intention.

[evanlinjin]

@afilini Right now the RpcBlockchain sync logic assumes that transactions returned by CoreTxIter may not in chronological order (this is why we need to calculate TransactionDetails::received in a separate loop. In the future we can change this to slightly improve performance (because we are using the new list_transactions function).

The chronological nature of transactions returned is more of an "implementation detail" for determining field values of TransactionDetails rather than a potential bug. As from my understanding, Database::iter_txs does not require transactions to be in chronological order.

[afilini]

Ah ok, that's fine.

The other bug was clear to me, instead of an if let it should have been a while let to iterate until you find a tx that is not filtered. I couldn't figure out why the chronological order was needed, thanks for the explanation!

[afilini]

ACK 74e2c47