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

add(rpc): Add extra getblock RPC fields used by some mining pools #6097

Merged
merged 7 commits into from
Feb 7, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 3, 2023

Motivation

This PR fixes a confusing error from s-nomp, when the block has actually been accepted by Zebra:

We thought a block was found but it was rejected by the daemon...

Specifications

https://zcash.github.io/rpc/getblock.html

Complex Code or Requirements

This RPC makes two separate state requests. But any possible combination of results must be accepted by clients, because they already have to handle chain forks. See the code comments in the PR for details.

Solution

Changes:

  • Add hash, height, and confirmations fields to getblock RPC
    • Some fields are only supplied when it is easy to get them, I added TODOs to provide them all the time
    • This is enough for s-nomp for now
  • Update snapshots, use new naming scheme
  • Remove a test that is already checked by snapshots

Related changes:

  • Document the performance requirements of the getblock RPC

Unrelated changes:

  • clippy: remove unnecessary return statement

Review

This isn't on the critical path, but the s-nomp error is confusing for users and developers.

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

  • always provide the hash, height, and confirmations fields for both hash and height arguments
    • make the Depth request support HashOrHeight

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Medium ⚡ I-usability Zebra is hard to understand or use A-rpc Area: Remote Procedure Call interfaces A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Feb 3, 2023
@teor2345 teor2345 requested a review from a team as a code owner February 3, 2023 02:42
@teor2345 teor2345 self-assigned this Feb 3, 2023
@teor2345 teor2345 requested review from arya2 and removed request for a team February 3, 2023 02:42
@github-actions github-actions bot added the C-feature Category: New features label Feb 3, 2023
@teor2345 teor2345 added the A-concurrency Area: Async code, needs extra work to make it work properly. label Feb 3, 2023
@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Merging #6097 (d184566) into main (0aab3df) will increase coverage by 0.06%.
The diff coverage is 86.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6097      +/-   ##
==========================================
+ Coverage   77.94%   78.01%   +0.06%     
==========================================
  Files         304      304              
  Lines       39035    39083      +48     
==========================================
+ Hits        30425    30489      +64     
+ Misses       8610     8594      -16     

@teor2345
Copy link
Contributor Author

teor2345 commented Feb 3, 2023

CI panicked in the getblocktemplate-rpcs snapshot code due to a test service timeout, but I didn't change those tests or that code at all:
https://github.com/ZcashFoundation/zebra/actions/runs/4080594241/jobs/7033201856#step:14:3070

It didn't happen in the macOS tests, and I can't reproduce it locally. So I'm going to assume it could be a very slow VM.

Also I got a test failure that is fixed by PR #6091:
https://github.com/ZcashFoundation/zebra/actions/runs/4080594267/jobs/7033830311#step:8:276

@teor2345
Copy link
Contributor Author

teor2345 commented Feb 5, 2023

Output comparisons:

Height:

$ zcash-rpc-diff 28232 getblock 1973877 1                                                                                                                                               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:                                                                                             
getblock 1973877 1
                                                  
Querying zebrad main chain at height >=1973877...
                                                  
real    0m0.007s                    
user    0m0.004s                      
sys     0m0.003s                
                                                  
Querying zcashd main chain at height >=1973877...
                                                  
real    0m0.007s       
user    0m0.003s         
sys     0m0.003s                     
                                                  
                                                  
Response diff between zebrad and zcashd:
--- /run/user/1000/tmp.n29EjDfk2D.rpc-diff/zebrad-main-1973877-getblock.json    2023-02-06 07:57:59.821727610 +1000
+++ /run/user/1000/tmp.n29EjDfk2D.rpc-diff/zcashd-main-1973877-getblock.json    2023-02-06 07:57:59.828727527 +1000
@@ -1,8 +1,52 @@       
 {                       
+  "hash": "00000000010927b96e49599efaecf1240ddcab9038497b4fcece1be9688309fa",
+  "confirmations": 1,                
+  "size": 2333,                
   "height": 1973877,    
+  "version": 4,                                                                                                                                                                                           
+  "merkleroot": "e9c5f7729846bbb16892dc8d817f391cbe121891536e851ac51957e570837dde",                                                                                                                       
+  "blockcommitments": "fcf47368e1782598534c4f2fb893e2474a4ac355bab3160d08e0ff807f02c1f1",                                                                                                                 
+  "authdataroot": "d4ac15b68ab2a886d18bb2e155bb5e2f8135201ca391a3ea03c61eb833c14fff",                                                                                                                     
+  "finalsaplingroot": "052f17f604954bfceb1b3e106d9c0fda48178edb24026f4ed8f21abf0a18d030",           
+  "finalorchardroot": "3a82f22051ab16b1da85e696fd00cc15fde299dbc717fce917553b3678441b12",
+  "chainhistoryroot": "98b59168f9b179f827681b2b009fd7bfd97c1f037b03163dfbe3fd3db6d62ee1",                                                                                                                 
   "tx": [
     "48297fe8d7e272efb53b5d0947eeb0b94b2f85baa49f47a3b69363012106990c",
     "fe7f0c301a2da277a8e871d3ff4cd1e9cb96986c6472456395ddce8d13d06f0b",
     "d4401d8988c787742357f72d9b70bd7ac982fde8128bb944887d1eaa5f58f832"
-  ]
+  ],
+  "time": 1675634225,
+  "nonce": "b1ad004000000000000000000010000000000000000000000000000000000000",
+  "solution": "...",
+  "bits": "1c014078",
+  "difficulty": 107216921.5625549,
+  "chainwork": "0000000000000000000000000000000000000000000000000c34f2d1790d9bec",
+  "anchor": "558ef286f07e698dd1795c1dd8b157920cda735b50ccc71cf199d55af63bd0c5",
+  "valuePools": [
+    {
+      "id": "sprout",
+      "monitored": true,
+      "chainValue": 26772.46532004,
+      "chainValueZat": 2677246532004,
+      "valueDelta": 0.00000000,
+      "valueDeltaZat": 0
+    },
+    {
+      "id": "sapling",
+      "monitored": true,
+      "chainValue": 998321.04957595,
+      "chainValueZat": 99832104957595,
+      "valueDelta": 0.00000000,
+      "valueDeltaZat": 0
+    },
+    {
+      "id": "orchard",
+      "monitored": true,
+      "chainValue": 34559.95906721,
+      "chainValueZat": 3455995906721,
+      "valueDelta": 0.00000000,
+      "valueDeltaZat": 0
+    }
+  ],
+  "previousblockhash": "0000000000bcdff0b66f5557060d515a7832b923fc2b9502e66d40d999a37075"
 }

Hash:

$ zcash-rpc-diff 28232 getblock 00000000010927b96e49599efaecf1240ddcab9038497b4fcece1be9688309fa 1                                                                          
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:                                                                                             
getblock 00000000010927b96e49599efaecf1240ddcab9038497b4fcece1be9688309fa 1
                                                  
Querying zebrad main chain at height >=1973884...
                                                  
real    0m0.007s                    
user    0m0.005s                      
sys     0m0.002s                
                                                  
Querying zcashd main chain at height >=1973884...
                                                  
real    0m0.007s       
user    0m0.003s         
sys     0m0.003s                     
                                                  
                                                  
Response diff between zebrad and zcashd:
--- /run/user/1000/tmp.DC9batDLY5.rpc-diff/zebrad-main-1973884-getblock.json    2023-02-06 08:10:15.756259394 +1000
+++ /run/user/1000/tmp.DC9batDLY5.rpc-diff/zcashd-main-1973884-getblock.json    2023-02-06 08:10:15.763259315 +1000
@@ -1,9 +1,53 @@       
 {                       
   "hash": "00000000010927b96e49599efaecf1240ddcab9038497b4fcece1be9688309fa",
   "confirmations": 8,                
+  "size": 2333,                
+  "height": 1973877,    
+  "version": 4,
+  "merkleroot": "e9c5f7729846bbb16892dc8d817f391cbe121891536e851ac51957e570837dde",
+  "blockcommitments": "fcf47368e1782598534c4f2fb893e2474a4ac355bab3160d08e0ff807f02c1f1", 
+  "authdataroot": "d4ac15b68ab2a886d18bb2e155bb5e2f8135201ca391a3ea03c61eb833c14fff",
+  "finalsaplingroot": "052f17f604954bfceb1b3e106d9c0fda48178edb24026f4ed8f21abf0a18d030",
+  "finalorchardroot": "3a82f22051ab16b1da85e696fd00cc15fde299dbc717fce917553b3678441b12",
+  "chainhistoryroot": "98b59168f9b179f827681b2b009fd7bfd97c1f037b03163dfbe3fd3db6d62ee1",
   "tx": [                       
     "48297fe8d7e272efb53b5d0947eeb0b94b2f85baa49f47a3b69363012106990c",
     "fe7f0c301a2da277a8e871d3ff4cd1e9cb96986c6472456395ddce8d13d06f0b",
     "d4401d8988c787742357f72d9b70bd7ac982fde8128bb944887d1eaa5f58f832"
-  ]
+  ],
+  "time": 1675634225,
+  "nonce": "b1ad004000000000000000000010000000000000000000000000000000000000",
+  "solution": "...",
+  "bits": "1c014078",
+  "difficulty": 107216921.5625549,
+  "chainwork": "0000000000000000000000000000000000000000000000000c34f2d1790d9bec",
+  "anchor": "558ef286f07e698dd1795c1dd8b157920cda735b50ccc71cf199d55af63bd0c5",
+  "valuePools": [
+    {
+      "id": "sprout",
+      "monitored": true,
+      "chainValue": 26772.46532004,
+      "chainValueZat": 2677246532004,
+      "valueDelta": 0.00000000,
+      "valueDeltaZat": 0
+    },
+    {
+      "id": "sapling",
+      "monitored": true,
+      "chainValue": 998321.04957595,
+      "chainValueZat": 99832104957595,
+      "valueDelta": 0.00000000,
+      "valueDeltaZat": 0
+    },
+    {
+      "id": "orchard",
+      "monitored": true,
+      "chainValue": 34559.95906721,
+      "chainValueZat": 3455995906721,
+      "valueDelta": 0.00000000,
+      "valueDeltaZat": 0
+    }
+  ],
+  "previousblockhash": "0000000000bcdff0b66f5557060d515a7832b923fc2b9502e66d40d999a37075",
+  "nextblockhash": "00000000009cc265164b783c9f0cc50757629a29537d64b5e41a37a716c38dac"
 }

The only differences are fields that are not present in Zebra.

@teor2345 teor2345 force-pushed the extra-get-block-fields branch from f32d1e8 to 42baa54 Compare February 5, 2023 22:13
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

This looks good, i just left a question.

zebra-rpc/src/methods.rs Show resolved Hide resolved
zebra-rpc/src/methods.rs Show resolved Hide resolved
zebra-rpc/src/methods.rs Show resolved Hide resolved
zebra-rpc/src/methods.rs Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Feb 7, 2023
@mergify mergify bot merged commit daba6d7 into main Feb 7, 2023
@mergify mergify bot deleted the extra-get-block-fields branch February 7, 2023 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-concurrency Area: Async code, needs extra work to make it work properly. A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement C-feature Category: New features I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants