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.

Sign up for GitHub

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

Jump to bottom

add(scan): Handle RegisterKeys messages in scan task #8222

Merged
merged 21 commits into from
Feb 6, 2024
Merged

add(scan): Handle RegisterKeys messages in scan task #8222

merged 21 commits into from
Feb 6, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Feb 1, 2024 •
edited
Loading

Motivation

This PR handles RegisterKey messages in the scan task, adding the new keys to parsed_keys to be scanned, and spawning a task to scan past blocks for the new viewing keys.

Closes #8220.

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

Testing

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-concurrency Area: Async code, needs extra work to make it work properly. A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Feb 1, 2024
@arya2 arya2 self-assigned this Feb 1, 2024
upbqdn
upbqdn previously approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

utACK

Looks great. I think we can merge and refactor later on if we need to.

zebra-scan/src/service/scan_task/commands.rs Outdated Show resolved Hide resolved
@arya2 arya2 force-pushed the scan-task-register-keys branch from 1c4bf90 to 6a45930 Compare February 2, 2024 22:31
@arya2 arya2 marked this pull request as ready for review February 5, 2024 18:31
@arya2 arya2 requested a review from a team as a code owner February 5, 2024 18:31
@arya2 arya2 requested review from upbqdn and removed request for a team February 5, 2024 18:31
@arya2 arya2 requested a review from a team as a code owner February 5, 2024 19:09
@arya2 arya2 force-pushed the scan-task-register-keys branch from 2059397 to 64e682c Compare February 5, 2024 19:14
@arya2 arya2 removed the request for review from a team February 5, 2024 19:14
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 5, 2024
@arya2 arya2 mentioned this pull request Feb 5, 2024
Closed
@arya2 arya2 requested a review from oxarbitrage February 6, 2024 00:35
@upbqdn upbqdn mentioned this pull request Feb 6, 2024
Closed
oxarbitrage
oxarbitrage previously approved these changes Feb 6, 2024
View reviewed changes
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.

I think this looks really good, i sent a few suggestions and questions but they are all minors.

zebra-scan/src/init.rs Outdated Show resolved Hide resolved
.github/workflows/sub-build-lightwalletd.yml Outdated Show resolved Hide resolved
zebra-scan/src/service.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/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/scan_task/tests/vectors.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Show resolved Hide resolved
zebra-scan/src/service/scan_task/executor.rs Outdated Show resolved Hide resolved
arya2 added 12 commits February 6, 2024 12:00
@arya2
moves scan functions to scan_task module
ad39351
@arya2
moves ScanTaskCommand and related method impls to its own module
75989cf
@arya2
moves ScanTask::mock() test constructor to its own module
5c5f262
@arya2
creates add_keys mod and moves scan_until() function there
8b50b97
@arya2
moves scan task executor to its own module and adds ScanRangeTaskBuil…
9c778ec 
…der type
@arya2
renames add_keys to scan_range, moves executor to scan_task mod
feed994
@arya2
adds test for process_messages
79e0a6e
@arya2
updates scan_height_and_store_results() to skip last_scanned_height c…
177c84d 
…heck if there is no key height for a key in `parsed_keys`
@arya2
updates default_test_config()
221f826
@arya2
adds acceptance test for registering keys in ScanTask
c8e6d3d
@arya2
uses the right feature for the new acceptance test
3484c69
@arya2
Applies suggestions from code review
217fcda
@arya2 arya2 force-pushed the scan-task-register-keys branch from afb9a05 to 217fcda Compare February 6, 2024 17:17
@arya2 arya2 requested a review from oxarbitrage February 6, 2024 17:20
@mergify mergify bot merged commit 2a004ff into main Feb 6, 2024
133 checks passed
@mergify mergify bot deleted the scan-task-register-keys branch February 6, 2024 18:41
@arya2 arya2 mentioned this pull request Feb 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage oxarbitrage approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn upbqdn is a code owner automatically assigned from ZcashFoundation/general-rust-reviewers

Assignees

@arya2 arya2

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 C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-Medium ⚡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle RegisterKeys messages in the scanner task
3 participants
@arya2 @upbqdn @oxarbitrage