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

add(scan): Implement SubscribeResults request for scan service #8253

Merged
merged 22 commits into from
Feb 13, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Feb 8, 2024

Motivation

We want to be able to subscribe to new scan results to stream to RPC clients.

Closes #8206.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

  • Updates process_messages() function to return a list of subscribed keys and their result senders
  • Adds a watch channel for sending subscribed_keys to scan_range() tasks
  • Checks if there's a result_sender for a key and sends it new results if there are any

Testing

  • Updates test vector for process_messages() to check that it processes SubscribeResults messages correctly
  • Adds a test for the ScanService to see that it sends the message correctly
  • Adds an acceptance test for checking that the results channel receives new relevant transactions

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

@arya2 arya2 added C-feature Category: New features A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Feb 8, 2024
@arya2 arya2 self-assigned this Feb 8, 2024
@arya2 arya2 added the E-help-wanted Call for participation: Help is requested to fix this issue. label Feb 9, 2024
@arya2 arya2 removed the E-help-wanted Call for participation: Help is requested to fix this issue. label Feb 9, 2024
… executor and setting an appropriate start height
@arya2 arya2 marked this pull request as ready for review February 9, 2024 20:44
@arya2 arya2 requested review from a team as code owners February 9, 2024 20:44
@arya2 arya2 requested review from oxarbitrage and removed request for a team February 9, 2024 20:44
@arya2
Copy link
Contributor Author

arya2 commented Feb 9, 2024

The acceptance test should pass now, the test was blocking the async executor with a blocking .recv_timeout() call and there weren't any relevant transactions in the height range that was being scanned before the timeout.

@arya2 arya2 requested a review from upbqdn February 9, 2024 22:02
@arya2 arya2 added the A-concurrency Area: Async code, needs extra work to make it work properly. label Feb 9, 2024
zebra-scan/src/service/tests.rs Outdated Show resolved Hide resolved
zebra-node-services/src/scan_service/response.rs Outdated Show resolved Hide resolved
zebra-node-services/src/scan_service/request.rs Outdated Show resolved Hide resolved
zebra-node-services/src/scan_service/response.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/scan_task/scan.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

It looks good to me, i made some small suggestions and some questions.

zebra-node-services/src/scan_service/response.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/scan_task/commands.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/scan_task/commands.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/scan_task/commands.rs Show resolved Hide resolved
zebra-scan/src/service/scan_task/executor.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/scan_task/executor.rs Outdated Show resolved Hide resolved
zebrad/tests/common/shielded_scan/subscribe_results.rs Outdated Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Feb 12, 2024
@mergify mergify bot merged commit 3929a52 into main Feb 13, 2024
133 checks passed
@mergify mergify bot deleted the scan-subscribe-results branch February 13, 2024 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-concurrency Area: Async code, needs extra work to make it work properly. C-feature Category: New features P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement SubscribeResults scan service request to subscribe to any new results for a set of keys
3 participants