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): Refactor get block template RPC into stages #5837

Merged
merged 23 commits into from
Dec 13, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 9, 2022

Motivation

To implement long polling, we need to repeat some parts of the get block template RPC.

But currently the RPC function is very large, and in no specific order.

This refactor does not change any Zebra functionality, except that the long poll ID now tracks all mempool transactions.

Solution

Functional Changes:

Refactors:

Cleanups:

Review

This review is a high priority because the PR conflicts with most other RPC changes.

This PR contains a lot of code movement, it might be easier to review it using git diff --color-moved, or by reviewing each commit on GitHub.

I can do a video review after the engineering sync next week, if that would help!

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

Actually implement long polling.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-High 🔥 A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Dec 9, 2022
@teor2345 teor2345 requested review from a team as code owners December 9, 2022 06:16
@teor2345 teor2345 self-assigned this Dec 9, 2022
@teor2345 teor2345 requested review from oxarbitrage and arya2 and removed request for a team December 9, 2022 06:16
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #5837 (fa8ec61) into main (68d0c55) will decrease coverage by 0.10%.
The diff coverage is 25.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5837      +/-   ##
==========================================
- Coverage   78.62%   78.52%   -0.11%     
==========================================
  Files         308      308              
  Lines       38605    38706     +101     
==========================================
+ Hits        30355    30393      +38     
- Misses       8250     8313      +63     

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.

Thank you for the cleanup and getting rustfmt working in get_block_template_rpcs.rs again!

I left some optional comments but I'm happy with this PR as-is.

@teor2345 teor2345 added do-not-merge Tells Mergify not to merge this PR and removed C-enhancement Category: This is an improvement labels Dec 12, 2022
@teor2345
Copy link
Contributor Author

Alfredo is going to review this as well.

@oxarbitrage
Copy link
Contributor

@oxarbitrage
Copy link
Contributor

oxarbitrage commented Dec 13, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Dec 13, 2022

Sorry but I didn't understand the command. Please consult the commands documentation 📚.

@mergify
Copy link
Contributor

mergify bot commented Dec 13, 2022

update

✅ Branch has been successfully updated

@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Dec 13, 2022
@oxarbitrage oxarbitrage removed the do-not-merge Tells Mergify not to merge this PR label Dec 13, 2022
mergify bot added a commit that referenced this pull request Dec 13, 2022
@mergify mergify bot merged commit e9d6e97 into main Dec 13, 2022
@mergify mergify bot deleted the refactor-get-block-template branch December 13, 2022 21:25
@teor2345
Copy link
Contributor Author

https://github.com/ZcashFoundation/zebra/actions/runs/3681221203/jobs/6227680505

error

The underlying error was:

#28 pushing manifest for us-docker.pkg.dev/zealous-zebra/zebra/zebrad:main@sha256:2e7d33e1bd670fc14173ce5c782434901a29f0a87c094bce677c620c69f8374e 0.2s done
#28 ERROR: failed commit on ref "manifest-sha256:2e7d33e1bd670fc14173ce5c782434901a29f0a87c094bce677c620c69f8374e": cannot reuse body, request must be retried
------
 > exporting to image:
------

https://github.com/ZcashFoundation/zebra/actions/runs/3681221203/jobs/6227680505#step:9:410

But that link goes to PR #5839, so I think we're fine here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants