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

feat(zebra-checkpoints): make zebra-checkpoints work for zebrad backend #5894

Merged
merged 10 commits into from
Jan 23, 2023

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Dec 24, 2022

Motivation

Zebra has an utility tool to create checkpoints. Until now this tool only work if zcashd is used as the backend.

This PR make the changes to make it work with zebrad as the backend while zcashd is still supported.

Close #5852

Solution

When i started on this i was only expecting to make changes to the zebra-checkpoints binary, however i realized later we were going to need changes in the getblock rpc method.

The binary is expecting height, hash and size fields of a getblock call with verbosity = 1. Currently (and because ligthwalletd only make use of the tx field of the object response), we are not returning any additional data.

this PR updated the getblock rpc method to return the additional data needed to make the tool work.

Also, in the zebra-checkpoints we changed the use of getblockcount to the blocks field of getblockchaininfo call as suggested in the proposed changes section of the ticket.

Review

This is optional and low priority. It can wait until people get back. Anyone can 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

  • Update some docs mentioning this in zebra book or checkpoint README ?
  • Do manual tests of differences between zcashd and zebrad generated checkpoints.
  • Compare the speed of using zebrad against zcashd as the backend.

@oxarbitrage oxarbitrage requested a review from a team as a code owner December 24, 2022 00:18
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team December 24, 2022 00:18
@github-actions github-actions bot added the C-feature Category: New features label Dec 24, 2022
@oxarbitrage oxarbitrage self-assigned this Dec 24, 2022
@oxarbitrage oxarbitrage added P-Low ❄️ A-rpc Area: Remote Procedure Call interfaces no-review-reminders Turn off review reminders labels Dec 24, 2022
@oxarbitrage
Copy link
Contributor Author

I did some manual test for comparison and performance.

In regards to time, zebra started a bit faster but they both ended up taking pretty much the same amount of time to get a full list of up to date checkpoints in the Mainnet. Here what i did:

zebra (i have a synchronized chain at port 6666):

$ time ./target/debug/zebra-checkpoints --cli /media/zcash/zcash/src/zcash-cli  -- -rpcport=6666 > mainnet_checkpoints_zebrad.txt

real	381m56,356s
user	60m57,259s
sys	36m19,635s
$ 

zcashd (synchronized and running in default rpc port):

$ time ./target/debug/zebra-checkpoints --cli /media/zcash/zcash/src/zcash-cli -- -rpccookiefile="/media/chain/zcash/.cookie" > mainnet_checkpoints_zcashd.txt
real	399m10,090s
user	67m53,213s
sys	37m6,481s
$

A comparassion of this 2 generated files show that they are exactly the same.

$ diff --color mainnet_checkpoints_zebrad.txt mainnet_checkpoints_zcashd.txt 
$

@codecov
Copy link

codecov bot commented Dec 24, 2022

Codecov Report

Merging #5894 (3117bb7) into main (67194c4) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5894      +/-   ##
==========================================
- Coverage   78.01%   77.90%   -0.12%     
==========================================
  Files         312      312              
  Lines       38997    39022      +25     
==========================================
- Hits        30425    30401      -24     
- Misses       8572     8621      +49     

arya2
arya2 previously approved these changes Jan 2, 2023
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/tests/vectors.rs Outdated Show resolved Hide resolved
arya2
arya2 previously approved these changes Jan 2, 2023
mergify bot added a commit that referenced this pull request Jan 3, 2023
@arya2
Copy link
Contributor

arya2 commented Jan 3, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jan 3, 2023

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Jan 3, 2023
@arya2 arya2 requested a review from teor2345 January 3, 2023 22:03
@arya2 arya2 added the A-docs Area: Documentation label Jan 3, 2023
@teor2345 teor2345 added A-compatibility Area: Compatibility with other nodes or wallets, or standard rules C-security Category: Security issues labels Jan 3, 2023
@arya2 arya2 self-assigned this Jan 3, 2023
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.

This looks good, and I'd really like to merge it. But it seems to cause a performance issue with lightwalletd.

I spent some time trying to work out how to fix it, let me know what you think of my suggestions!

zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-consensus/src/checkpoint/README.md Show resolved Hide resolved
@teor2345 teor2345 removed the C-security Category: Security issues label Jan 4, 2023
@teor2345 teor2345 changed the title feat(zebra-checkpoint): make zebra-checkpoint work for zebrad backend feat(zebra-checkpoints): make zebra-checkpoints work for zebrad backend Jan 4, 2023
@teor2345 teor2345 added the S-waiting-on-author Status: waiting on the ticket or PR author label Jan 6, 2023
@mpguerra
Copy link
Contributor

If we can't get this merged with minimal changes, e.g timeboxed to 1 day + 1 more round of reviews then we should park it for now

@arya2 arya2 removed their assignment Jan 17, 2023
@oxarbitrage
Copy link
Contributor Author

If we can't get this merged with minimal changes, e.g timeboxed to 1 day + 1 more round of reviews then we should park it for now

I think this is easy enough for a fix in one day and another round of reviews. Working on it now.

@oxarbitrage oxarbitrage removed no-review-reminders Turn off review reminders S-waiting-on-author Status: waiting on the ticket or PR author labels Jan 19, 2023
@oxarbitrage oxarbitrage requested review from teor2345 and arya2 January 19, 2023 20:28
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thanks for the workaround to the performance issue!

zebra-utils/src/bin/zebra-checkpoints/args.rs Show resolved Hide resolved
@teor2345 teor2345 added the A-consensus Area: Consensus rule updates label Jan 20, 2023
@teor2345 teor2345 dismissed their stale review January 20, 2023 01:03

Performance issues were fixed

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.

Looks good, just doing some testing to make sure all the backends produce the same results.

@teor2345
Copy link
Contributor

Looks good, just doing some testing to make sure all the backends produce the same results.

All 3 backend/node combinations produce the same results for new checkpoints, I'll run a full set of checkpoints overnight to be sure. I think it's worth doing this test because checkpoints are consensus-critical.

@teor2345
Copy link
Contributor

The full checkpoint files were exactly the same as each other, and they didn't change any of the existing checkpoints.

mergify bot added a commit that referenced this pull request Jan 23, 2023
@mergify mergify bot merged commit d72211f into main Jan 23, 2023
@mergify mergify bot deleted the issue5852 branch January 23, 2023 04:50
teor2345 pushed a commit that referenced this pull request Feb 6, 2023
…kend (#5894) - theirs merge: git cherry-pick --strategy=ort --strategy-option=theirs

* make `zebra-checkpoint` util work with zebra as the backend

* update snapshots

* update documentation

* applies suggestions from code review

* irefactor zebra-checkpoints to work with zebra using deserialization of the raw block

* fix imports and derives

* rename mode to backend

* remove old stuff

* fix docs

Co-authored-by: arya2 <[email protected]>
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-consensus Area: Consensus rule updates A-docs Area: Documentation A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make zebra-checkpoints work with Zebra RPCs, as well as zcashd RPCs
5 participants