fix(rpc): Fix bugs and performance of getnetworksolps & getnetworkhashps RPCs

[teor2345]

Motivation

When getnetworksolps & getnetworkhashps are run over a large block range, they are extremely slow or produce incorrect results.

There are also some bugs, zcashd incompatibilities, and unsupported argument values in the RPC implementation.

This is a partial performance and bug fix for #6688, but it doesn't limit the height range.

Specifications

zcashd implements these RPCs using this function:
https://github.com/zcash/zcash/blob/482f120f635e342061005d5218d7cc18d1f342b5/src/rpc/mining.cpp#L48-L83

I re-wrote the implementation to match what the zcashd function does.

Complex Code or Requirements

We want to match zcashd's behaviour and performance. Some performance improvements require database changes (#7109), but they are out of scope for this ticket and PR.

To improve performance, I iterated over block headers, rather than full blocks. To do this, I re-wrote the chain iterator code so it was generic over the chain item. (Block, block header, etc.)

The zcashd code is fairly complicated, so I tried to follow it closely.

Solution

Compatibility fixes:

• Re-write the RPC and state implementations to match zcashd's code
• Accept the argument values that zcashd accepts, and handle them like zcashd

Bug fixes:

• Handle existing arguments exactly like zcashd
• Use min and max times, rather than first and last times
• Avoid dividing by a zero work duration (if the times are the same, which is valid)

Related changes:

• Simplify the RPC and state code, removing manual iteration
• Re-write the chain iterator so it is generic over the chain item
• Simplify the chain iterator code

Unrelated changes:

• Removed implemented TODO comments

Testing

Add extra test cases for RPC arguments.

I manually tested this change using zcash-rpc-diff, see the comments for details.

Review

This PR is ready for review.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

Follow Up Work

Test the performance of the new code to work out if we need to limit the height range.
Test for invalid answers when large heights are supplied.

[teor2345]

Block Range: Manual Performance and Compatibility Testing

This code produces identical results to zcashd. It is up to 20x faster than zcashd for small block ranges, but sometimes it is a few tens of milliseconds slower.

It becomes noticeably slower around 10,000 blocks. It takes 1 second for 100,000 blocks, 14 seconds for 1000,000 blocks, and 1 minute for the entire chain.

Since it's fast for the default range and typical ranges, and it finishes in a reasonable time for the whole chain, I don't think we need to do anything further here. If someone tells us the performance on large ranges is an issue, we can open a ticket then.

I used the following numbers of blocks:

# since this is larger than the chain length, it just uses the whole chain
zcash-rpc-diff 28232 getnetworksolps 10000000 2256077
zcash-rpc-diff 28232 getnetworksolps 1000000 2256077
zcash-rpc-diff 28232 getnetworksolps 100000 2256077
zcash-rpc-diff 28232 getnetworksolps 10000 2256077
zcash-rpc-diff 28232 getnetworksolps 1000 2256077
zcash-rpc-diff 28232 getnetworksolps 120 2256077
zcash-rpc-diff 28232 getnetworksolps -1 2256077
zcash-rpc-diff 28232 getnetworksolps -99 2256077
zcash-rpc-diff 28232 getnetworksolps 2 2256077
zcash-rpc-diff 28232 getnetworksolps 1 2256077
zcash-rpc-diff 28232 getnetworksolps 0 2256077

Here is a sample output:

$ zcash-rpc-diff 28232 getnetworksolps 120 2256077
Using 'Zcash RPC client version v5.7.0-ge08571476d8' via command 'zcash-cli'.
Using bash shell '5.1.16(1)-release' launched from '/nix/store/afc0n4afniiipmwz8asn8z16xlwkwjb4-bash-interactive-5.1-p16/bin/bash'.

Checking first node release info...
Checking second node release info...
Connected to zebrad v1.2.0+113.gbcce69b (port 28232) and zcashd v5.7.0-ge08571476d8 (zcash.conf port).

Checking zebrad v1.2.0+113.gbcce69b network and tip height...
Checking zcashd v5.7.0-ge08571476d8 network and tip height...

Request:
getnetworksolps 120 2256077

Querying zebrad v1.2.0+113.gbcce69b main chain at height >=2256080...

real    0m0.007s
user    0m0.004s
sys     0m0.003s

Querying zcashd v5.7.0-ge08571476d8 main chain at height >=2256080...

real    0m0.007s
user    0m0.003s
sys     0m0.004s


Response diff between zebrad v1.2.0+113.gbcce69b and zcashd v5.7.0-ge08571476d8 (limited to 40 lines):
RPC responses were identical

/run/user/1000/tmp.7Rsnl3dX4P.rpc-diff/zebrad-main-2256080-getnetworksolps.json, limited to 40 lines:
8626536292

[teor2345]

Initial Height: Compatibility Testing

I tried these heights and they produced the same results:

zcash-rpc-diff 28232 getnetworksolps 120 10
zcash-rpc-diff 28232 getnetworksolps -1 0
zcash-rpc-diff 28232 getnetworksolps 1 2
zcash-rpc-diff 28232 getnetworksolps 1 300000
zcash-rpc-diff 28232 getnetworksolps 120 300000
zcash-rpc-diff 28232 getnetworksolps -1 300000
zcash-rpc-diff 28232 getnetworksolps 120 5
zcash-rpc-diff 28232 getnetworksolps 120 -5
 zcash-rpc-diff 28232 getnetworksolps 120 20000000
zcash-rpc-diff 28232 getnetworksolps 0 0
zcash-rpc-diff 28232 getnetworksolps 0 -1
zcash-rpc-diff 28232 getnetworksolps -1 0
zcash-rpc-diff 28232 getnetworksolps -1 -1

[arya2]

This looks excellent, thank you for the doc cleanups.

[teor2345]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed