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

change(rpc): Adds getmininginfo, getnetworksolps and getnetworkhashps methods #5808

Merged
merged 15 commits into from Dec 8, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Dec 7, 2022

Motivation

We want to support the getnetworksolps, getnetworkhashps, and getmininginfo RPCs.

Solution

  • Adds a state request for getting the chain solution rate
    • Sums up total work for a range of blocks
    • Divides it by number of seconds elapsed between the first and last block in the range
  • Adds RPC getmininginfo, getnetworksolps and getnetworkhashps RPCs

Review

Part of regular scheduled work.

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?
  • How do you know it works? Does it have tests?

Follow Up Work

  • Populate other fields for the getmininginfo response

@arya2 arya2 added A-rust Area: Updates to Rust code P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes labels Dec 7, 2022
@arya2 arya2 self-assigned this Dec 7, 2022
@github-actions github-actions bot added C-enhancement Category: This is an improvement C-feature Category: New features labels Dec 7, 2022
@arya2 arya2 marked this pull request as ready for review December 7, 2022 02:33
@arya2 arya2 requested a review from a team as a code owner December 7, 2022 02:33
@arya2 arya2 requested review from dconnolly and removed request for a team December 7, 2022 02:33
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

I can see some places where the argument handling and result format doesn't seem to match the RPC reference. But I understand the reference might be different from what zcashd actually does.

Can you document each new argument and field with its description from the reference, and document any changes from zcashd or special processing that we do?

I'd also like to see the manual test results from zcash-rpc-diff, that will show up any differences.

zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-state/src/request.rs Outdated Show resolved Hide resolved
zebra-state/src/service.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/difficulty.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/difficulty.rs Show resolved Hide resolved
zebra-state/src/service/read/difficulty.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/difficulty.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #5808 (94db8f2) into main (678c519) will increase coverage by 0.07%.
The diff coverage is 25.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5808      +/-   ##
==========================================
+ Coverage   78.82%   78.90%   +0.07%     
==========================================
  Files         307      308       +1     
  Lines       38737    38685      -52     
==========================================
- Hits        30536    30523      -13     
+ Misses       8201     8162      -39     

@teor2345
Copy link
Contributor

teor2345 commented Dec 7, 2022

I did a zcash-rpc-diff, and it seemed to be the same, good work!

But since it's a manual test, we'll need to do it again before we merge.

- SolutionRate state request

- getnetworksolps, getnetworkhashps, & getmininginfo RPCs

- vectors tests
updates ReadRequest::SolutionRate doc link
moves snapshot tests up where it can use the populated state service
@arya2 arya2 removed the request for review from dconnolly December 8, 2022 00:23
@teor2345
Copy link
Contributor

teor2345 commented Dec 8, 2022

It looks like we've hit a GitHub API rate limit, causing CI jobs across multiple PRs to fail:
https://github.com/ZcashFoundation/zebra/actions/runs/3644298974/jobs/6153406794#step:3:13

it should reset within an hour or so.

I'll ask Gustavo to look into this next week. But until then, please push your commits in batches, rather than one at a time. (Each push runs about 50 jobs.)

Adds "arbitrary_precision" feature to serde_json in zebra-rpc
@arya2
Copy link
Contributor Author

arya2 commented Dec 8, 2022

It looks like we've hit a GitHub API rate limit, causing CI jobs across multiple PRs to fail:
https://github.com/ZcashFoundation/zebra/actions/runs/3644298974/jobs/6153406794#step:3:13

it should reset within an hour or so.

I'll ask Gustavo to look into this next week. But until then, please push your commits in batches, rather than one at a time. (Each push runs about 50 jobs.)

🤦 Whoops. Okay, noted.

Response diffs:

WARNING: comparing RPC responses from different heights:
zcashd is at: 107837
zebrad is at: 1905342

Request: getnetworkhashps

Querying zebrad main chain at height >=1905342...
time: real 0m0.005s, user 0m0.002s, sys 0m0.000s

Querying zcashd main chain at height >=107837...
time: real 0m0.003s, user 0m0.003s, sys 0m0.000s

Response diff between zcashd and zebrad:
-12139905448
+56199849
WARNING: comparing RPC responses from different heights:
zcashd is at: 146135
zebrad is at: 1905349

Request:
getnetworksolps

Querying zebrad main chain at height >=1905349...
time: real 0m0.005s, user 0m0.002s, sys 0m0.000s

Querying zcashd main chain at height >=146135...
time: real 0m0.063s, user 0m0.003s, sys 0m0.000s

Response diff between zcashd and zebrad:
-12224410071
+201076157
WARNING: comparing RPC responses from different heights:
zcashd is at: 108810
zebrad is at: 1905347

Request: getmininginfo

Querying zebrad main chain at height >=1905347...
time: real 0m0.006s, user 0m0.003s, sys 0m0.000s

Querying zcashd main chain at height >=108810...
time: real 0m0.002s, user 0m0.002s, sys 0m0.000s

Response diff between zcashd and zebrad:
{
-  "networksolps": 12413336220,
-  "networkhashps": 12413336220,
+  "blocks": 108810,
+  "difficulty": 1348528.045183577,
+  "errors": "Your computer's date and time may be behind the rest of the network! If your clock is wrong Zcash will not work properly.",
+  "errorstimestamp": 1670465885,
+  "genproclimit": 1,
+  "localsolps": 0,
+  "networksolps": 81163088,
+  "networkhashps": 81163088,
+  "pooledtx": 0,
+  "testnet": false,
   "chain": "main",
-  "testnet": false
+  "generate": false
 }

I'll update these diffs when the zcashd node finishes syncing.

@arya2 arya2 requested a review from teor2345 December 8, 2022 03:19
@teor2345
Copy link
Contributor

teor2345 commented Dec 8, 2022

WARNING: comparing RPC responses from different heights:
zcashd is at: 107837
zebrad is at: 1905342

The different heights are going to make some fields different, even if the calculations in Zebra and zcashd are exactly the same.

Here it is with them both synced:

$ zcash-rpc-diff 28232 getnetworkhashps
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 28232) and zcashd (zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getnetworkhashps

Querying zebrad main chain at height >=1905507...

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

Querying zcashd main chain at height >=1905507...

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


Response diff between zebrad and zcashd:
RPC responses were identical

/run/user/1000/tmp.50psuVx1Eh.rpc-diff/zebrad-main-1905507-getnetworkhashps.json:
11061724042
$ zcash-rpc-diff 28232 getnetworksolps
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 28232) and zcashd (zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getnetworksolps

Querying zebrad main chain at height >=1905507...

real    0m0.009s
user    0m0.005s
sys     0m0.002s

Querying zcashd main chain at height >=1905507...

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


Response diff between zebrad and zcashd:
RPC responses were identical

/run/user/1000/tmp.Xn2Omwyuvf.rpc-diff/zebrad-main-1905507-getnetworksolps.json:
11061724042
$ zcash-rpc-diff 28232 getmininginfo
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 28232) and zcashd (zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getmininginfo

Querying zebrad main chain at height >=1905507...

real    0m0.009s
user    0m0.005s
sys     0m0.002s

Querying zcashd main chain at height >=1905507...

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


Response diff between zebrad and zcashd:
--- /run/user/1000/tmp.bzO2k7ujrN.rpc-diff/zebrad-main-1905507-getmininginfo.json       2022-12-08 16:30:55.620302894 +1000
+++ /run/user/1000/tmp.bzO2k7ujrN.rpc-diff/zcashd-main-1905507-getmininginfo.json       2022-12-08 16:30:55.628302836 +1000
@@ -1,6 +1,16 @@
 {
+  "blocks": 1905507,
+  "currentblocksize": 2128,
+  "currentblocktx": 1,
+  "difficulty": 97409482.22582503,
+  "errors": "",
+  "errorstimestamp": 1670481055,
+  "genproclimit": 0,
+  "localsolps": 0,
   "networksolps": 11061724042,
   "networkhashps": 11061724042,
+  "pooledtx": 7,
+  "testnet": false,
   "chain": "main",
-  "testnet": false
+  "generate": false
 }

In this last one, all the differences are due to extra fields (including the line deletion for the trailing comma).

Great work, thanks for making this all match up!

@teor2345
Copy link
Contributor

teor2345 commented Dec 8, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Dec 8, 2022

refresh

✅ Pull request refreshed

@arya2
Copy link
Contributor Author

arya2 commented Dec 8, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Dec 8, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Dec 8, 2022
@mergify mergify bot merged commit 77b85cf into main Dec 8, 2022
@mergify mergify bot deleted the getmininginfo branch December 8, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces A-rust Area: Updates to Rust code A-state Area: State / database changes C-enhancement Category: This is an improvement C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement getnetworksolps and getnetworkhashps RPC Add basic support for getmininginfo RPC call
2 participants