feat(rpc): introduce getblocktemplate-rpcs feature

[oxarbitrage]

Motivation

We want to support mining rpc calls but as an isolated rust feature. Can close #5305 with some more work. It also implement partially get_block_count (#5303) so we can have something to test with.

Also closes #5303.

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

[teor2345]

New features are blocked until we tag the first release candidate.

[codecov]

Codecov Report

Merging #5357 (3891e77) into main (7207f9d) will decrease coverage by 0.13%.
The diff coverage is 100.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     

[oxarbitrage]

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).

[teor2345]

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.

[teor2345]

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:

zebra/zebra-rpc/src/server.rs

Line 68 in ee0edef

let (rpc_impl, rpc_tx_queue_task_handle) = RpcImpl::new(

I'm guessing you did this already, but we also need to compile zebrad with --features getblocktemplate-rpcs.

[teor2345]

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:

zebra/zebrad/Cargo.toml

Line 46 in ee0edef

proptest-impl = ["proptest", "proptest-derive", "zebra-chain/proptest-impl", "zebra-state/proptest-impl", "zebra-consensus/proptest-impl", "zebra-network/proptest-impl"]

[oxarbitrage]

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.

[teor2345]

We tagged the release candidate, so new features can merge now.

Can you add a priority label to this ticket?

[teor2345]

Looks good, looking forward to getting it working!

[teor2345]

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?

[oxarbitrage]

I highligthed the issue a bit more in b1a6918 and added comments about it at #5357 (review)

[oxarbitrage]

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
$ 

[oxarbitrage]

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"
}

[arya2]

This looks great, thank you!

It may be worth initializing a second object with the fields we need in order to separate the new code into a second trait definition at some point, but I think we can move forward with this for now.

I suggested a fix to a small typo, and we should promptly follow-up with #5405 because there are sometimes issues in CI that we don't see locally.

[oxarbitrage]

Thanks @arya2 , i added the suggested typo fix. Can you re approve ? Thanks.