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

feature(rpc): Add basic long polling support to the getblocktemplate RPC #5843

Merged
merged 3 commits into from
Dec 15, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 12, 2022

Motivation

This PR implements basic long polling support, checking for changes by re-fetching from the sync status, state and mempool.

Part of ticket #5720.

Complex Code or Requirements

We need to hold the RPC request open until the response will change. This means running a loop and a select!{} in a future.

Solution

Implement inefficient long polling by checking the state/mempool/syncer, waiting 5 seconds, then checking it again. Also wait for the max time.

Related changes:

  • Change impl From<LongPollInput> for LongPollId into a named method
  • Rename the long_poll_id field
  • Cleanup some comments

Review

This is ready for review, I'm doing some manual testing now. Automated testing will happen in the next PR.

Efficiency and refactors are out of scope for this PR, I'd like to focus on getting some kind of working implementation merged.

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

  • medium priority: check for state changes efficiently
  • low priority: check for sync status and mempool changes efficiently

@teor2345 teor2345 added P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-feature Category: New features A-concurrency Area: Async code, needs extra work to make it work properly. labels Dec 12, 2022
@teor2345 teor2345 self-assigned this Dec 12, 2022
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #5843 (5d1bba2) into main (e9d6e97) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 5d1bba2 differs from pull request most recent head 99f244f. Consider uploading reports for the commit 99f244f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5843      +/-   ##
==========================================
- Coverage   78.48%   78.44%   -0.05%     
==========================================
  Files         308      308              
  Lines       38706    38706              
==========================================
- Hits        30379    30361      -18     
- Misses       8327     8345      +18     

Base automatically changed from refactor-get-block-template to main December 13, 2022 21:25
@teor2345 teor2345 marked this pull request as ready for review December 13, 2022 21:30
@teor2345 teor2345 requested a review from a team as a code owner December 13, 2022 21:30
@teor2345 teor2345 requested review from dconnolly and removed request for a team December 13, 2022 21:30
@teor2345
Copy link
Contributor Author

This is ready for review, I'm doing some manual testing now. Automated testing will happen in the next PR.

@teor2345
Copy link
Contributor Author

A bunch of google cloud jobs seemed to have failed setup with:

ssh: connect to host 104.198.199.90 port 22: Connection timed out

https://github.com/ZcashFoundation/zebra/actions/runs/3689513976/jobs/6246166936#step:11:78

@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Dec 13, 2022

update

✅ Branch has been successfully updated

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.

This looks like it should work.

@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Dec 14, 2022
@teor2345
Copy link
Contributor Author

@arya2 just checking why you marked this PR as "do-not-merge"?

It would be easier for me if this was part of the main branch and I didn't have to rebase my changes for PR #5862.

I'm also happy to sort out tests and any fixes in the next PR.

@oxarbitrage
Copy link
Contributor

I think is in case someone else wanted to review as @dconnolly is in the list as well.

@oxarbitrage
Copy link
Contributor

I think is in case someone else wanted to review as @dconnolly is in the list as well.

But i think one approval is ok here so we can just remove the do not merge tag.

@arya2
Copy link
Contributor

arya2 commented Dec 14, 2022

@arya2 just checking why you marked this PR as "do-not-merge"?

Just in case we wanted a second reviewer. I'll remove the label.

@arya2 arya2 removed the do-not-merge Tells Mergify not to merge this PR label Dec 14, 2022
@teor2345 teor2345 removed the request for review from dconnolly December 14, 2022 22:49
@teor2345
Copy link
Contributor Author

@arya2 just checking why you marked this PR as "do-not-merge"?

Just in case we wanted a second reviewer. I'll remove the label.

Always happy to get a second review, but I try not to block unless someone has committed to doing a review soon.

mergify bot added a commit that referenced this pull request Dec 14, 2022
@mergify mergify bot merged commit f7011a9 into main Dec 15, 2022
@mergify mergify bot deleted the wait-on-long-poll branch December 15, 2022 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants