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(scanner): Restart scanning where left #8080

Merged
merged 24 commits into from
Dec 13, 2023
Merged

feat(scanner): Restart scanning where left #8080

merged 24 commits into from
Dec 13, 2023

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Dec 9, 2023

Motivation

We want the scanner to have the ability of resume the scanning where it was left after a restart.

Close #8022

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?

Solution

I am pushing for a solution that involves adding a control empty result entry to the database for each key every X blocks. This is because a very probable scenario is that the key do not have any real transaction close to sapling (all new accounts).

Testing

The functionality is tested by adding an acceptance test that needs a cache state close to tip.

Review

@teor2345 had been working with me in this.

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

This PR does not include the cases of new keys/deleted keys in the config that are part of #8022

It will be done in a separated PR.

@oxarbitrage oxarbitrage added P-Medium ⚡ A-blockchain-scanner Area: Blockchain scanner of shielded transactions labels Dec 9, 2023
@github-actions github-actions bot added the C-feature Category: New features label Dec 9, 2023
Copy link
Contributor Author

@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.

A few comments and doubts i have.

zebra-scan/src/scan.rs Outdated Show resolved Hide resolved
zebra-scan/src/storage.rs Show resolved Hide resolved
zebra-scan/src/storage.rs Outdated Show resolved Hide resolved
zebra-scan/src/storage.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Show resolved Hide resolved
Copy link
Collaborator

@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.

Most of this looks good, but I think we need to count our keys and heights carefully. This might need some unit tests to be sure.

Let's do a video review and talk through the database reading loop?

zebra-scan/src/storage.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Show resolved Hide resolved
zebra-scan/src/storage/db/sapling.rs Outdated Show resolved Hide resolved
zebra-scan/src/storage/db/sapling.rs Outdated Show resolved Hide resolved
zebra-scan/src/storage/db.rs Show resolved Hide resolved
zebra-scan/src/scan.rs Show resolved Hide resolved
zebra-scan/src/storage/db/sapling.rs Outdated Show resolved Hide resolved
zebra-scan/src/storage/db/sapling.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Collaborator

I left some TODOs in PR #8075 and #8083 about testing this code using snapshots. So let's open another ticket to do that once they all merge?

I think that would cover most of the testing for this PR, except maybe for some unit tests for skipped keys and skipped heights.

@mpguerra mpguerra linked an issue Dec 11, 2023 that may be closed by this pull request
7 tasks
@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Dec 11, 2023
zebra-scan/src/scan.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 removed the do-not-merge Tells Mergify not to merge this PR label Dec 12, 2023
Copy link
Collaborator

@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.

Looking good now, let me know if you want help with the height we start fetching blocks.

@oxarbitrage oxarbitrage marked this pull request as ready for review December 13, 2023 19:01
@oxarbitrage oxarbitrage requested a review from a team as a code owner December 13, 2023 19:01
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team December 13, 2023 19:01
Copy link
Collaborator

@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.

Thanks for working through this, let's see how it goes!

mergify bot added a commit that referenced this pull request Dec 13, 2023
@mergify mergify bot merged commit 92758a0 into main Dec 13, 2023
124 checks passed
@mergify mergify bot deleted the issue8022 branch December 13, 2023 20:16
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 C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(scanner): Start scanning where it left off last time
3 participants