zebra
zebra copied to clipboard
feat(rpc): introduce `getblocktemplate-rpcs` feature
Motivation
We want to support mining rpc calls but as an isolated rust feature. Can close https://github.com/ZcashFoundation/zebra/issues/5305 with some more work. It also implement partially get_block_count
(https://github.com/ZcashFoundation/zebra/issues/5303) so we can have something to test with.
Solution
PR attempts to do the supertrait suggested implementation. It looks good and we can make tests but something is missing. When calling the rpc with curl:
$ curl --silent --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getblockcount", "params": [] }' -H 'Content-type: application/json' http://127.0.0.1:8232/ | jq
{
"error": {
"code": -32601,
"message": "Method not found"
},
"id": "curltest"
}
$
Review
I will like @teor2345 to take a look
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
New features are blocked until we tag the first release candidate.
Codecov Report
Merging #5357 (3891e77) into main (7207f9d) will decrease coverage by
0.13%
. The diff coverage is100.00%
.
Additional details and impacted files
@@ Coverage Diff @@
## main #5357 +/- ##
==========================================
- Coverage 79.19% 79.06% -0.14%
==========================================
Files 308 308
Lines 39560 39289 -271
==========================================
- Hits 31329 31062 -267
+ Misses 8231 8227 -4
Why the optional tag was added if as far as i know it was decided we want to have the mining calls in a feature ?
I think the optional tag discourage developers to keep working on a pull request when used unilaterally as it has been done here (and pretty much everywhere else).
Why the optional tag was added if as far as i know it was decided we want to have the mining calls in a feature ?
I added the optional tag because this work isn't scheduled for this sprint, and we have the release candidate and getblocktemplate analysis work as a higher priority.
I added the "do not merge" tag because we can't merge new features until we've tagged the release candidate.
Sorry I didn't explain that to start with, I've been busy trying to fix CI and get the release candidate working.
It looks good and we can make tests but something is missing.
When the getblocktemplate-rpcs
feature is activated, we need to start the server using the GetBlockTemplateRpc
implementation:
https://github.com/ZcashFoundation/zebra/blob/ee0edef8571d3ed82848303b142c7ae72e7135fc/zebra-rpc/src/server.rs#L68
I'm guessing you did this already, but we also need to compile zebrad
with --features getblocktemplate-rpcs
.
I'm guessing you did this already, but we also need to compile
zebrad
with--features getblocktemplate-rpcs
.
This might need a getblocktemplate-rpcs
feature on zebrad
as well, here's how you set it up:
https://github.com/ZcashFoundation/zebra/blob/ee0edef8571d3ed82848303b142c7ae72e7135fc/zebrad/Cargo.toml#L46
The do not merge
is enough here for knowing we can't merge into main
for the release candidate. I removed the optional
because i think it is not.
We tagged the release candidate, so new features can merge now.
Can you add a priority label to this ticket?
This new RPC needs a snapshot test and a manual zcash-rpc-diff
test.
Can you post the results of zcash-rpc-diff
as a comment on this PR?
I highligthed the issue a bit more in https://github.com/ZcashFoundation/zebra/pull/5357/commits/b1a6918987ac09d44550350ef9be22511ba18a3e and added comments about it at https://github.com/ZcashFoundation/zebra/pull/5357#pullrequestreview-1141555004
Diffs are different because zcashd and zebrad are at different heights locally.
$ ./zcash-rpc-diff 6666 getblockcount
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 6666) and zcashd (zcash-cli zcash.conf port).
Checking zebrad network and tip height...
Checking zcashd network and tip height...
WARNING: comparing RPC responses from different heights:
zcashd is at: 15784
zebrad is at: 88152
Request:
getblockcount
Querying zebrad main chain at height >=88152...
real 0m0,002s
user 0m0,001s
sys 0m0,000s
Querying zcashd main chain at height >=15784...
real 0m0,002s
user 0m0,001s
sys 0m0,000s
Response diff between zcashd and zebrad:
--- /tmp/tmp.bWNHZfXNvz.rpc-diff/zebrad-main-88152-getblockcount.json 2022-10-17 17:12:54.993989666 -0300
+++ /tmp/tmp.bWNHZfXNvz.rpc-diff/zcashd-main-15784-getblockcount.json 2022-10-17 17:12:54.993989666 -0300
@@ -1 +1 @@
-88152
+15784
$
Output of zebra when quering the getblockcount
method in a build with no getblocktemplate-rpcs
feature:
$ curl --silent --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getblockcount", "params": [] }' -H 'Content-type: application/json' http://127.0.0.1:8232/ | jq
{
"error": {
"code": 0,
"message": "RPC method is not implemented for zebra builds without `getblocktemplate-rpcs` feature"
},
"id": "curltest"
}
The same call but in a build with getblocktemplate-rpcs
feature:
$ curl --silent --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getblockcount", "params": [] }' -H 'Content-type: application/json' http://127.0.0.1:8232/ | jq
{
"result": 84896,
"id": "curltest"
}
Thanks @arya2 , i added the suggested typo fix. Can you re approve ? Thanks.