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(scan): Create a scanner storage database, but don't use it yet #8031

Merged
merged 11 commits into from
Nov 30, 2023

Conversation

teor2345
Copy link
Collaborator

@teor2345 teor2345 commented Nov 29, 2023

Motivation

This does the database creation and column family parts of #8019, but doesn't actually use the database.

There's still the actual data part of that ticket left for the next PR, and testing.

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?

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

Complex Code or Requirements

This seems to just work as expected.

Solution

  • Create an on-disk database in the scanner storage (but don't use it yet)
  • Create a column family
  • Use ephemeral storage in tests

Related changes:

  • Move populating storage into the new() method
  • Add a zebra_scan::init() method

Related fixes:

  • Fix a bug in the state where format tests weren't being skipped on shutdown even if the database was configured to skip them

Testing

The existing tests found one bug already, we'll add more in the next few PRs.

Review

This seemed like enough code to review for now. It's not urgent but it is on the critical path to completing the scanner MVP.

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

The rest of #8019:

  • Define key and result serialization
  • Put the keys in the database
  • Put the results in the database
  • Document the database format

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Medium ⚡ A-state Area: State / database changes A-blockchain-scanner Area: Blockchain scanner of shielded transactions labels Nov 29, 2023
@teor2345 teor2345 self-assigned this Nov 29, 2023
@teor2345 teor2345 requested a review from a team as a code owner November 29, 2023 05:41
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team November 29, 2023 05:41
@teor2345 teor2345 changed the title change(scan): Create an empty scanner storage database, don't use it yet change(scan): Create a scanner storage database, but don't use it yet Nov 29, 2023
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.

Looking great but i think the failing test need further investigation.

thread 'tests::scanning_fake_blocks_store_key_and_results' panicked at zebra-state/src/service/finalized_state/zebra_db/block.rs:65:66:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::scanning_fake_blocks_store_key_and_results ... FAILED

https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/finalized_state/zebra_db/block.rs#L65

It is clear that the test do not have a state but i don't see why and where tip() needs to be called.

We can just ignore the test to merge and deal with it later but i don't think that will be a good idea.

@teor2345
Copy link
Collaborator Author

It is clear that the test do not have a state but i don't see why and where tip() needs to be called.

It's being called from a format change, which should have been disabled in PR #8002, but I missed it:

thread 'tests::scanning_fake_blocks_store_key_and_results' panicked at zebra-state/src/service/finalized_state/zebra_db/block.rs:65:66:
called `Option::unwrap()` on a `None` value
stack backtrace:
...
   2: core::panicking::panic
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:127:5
   3: zebra_state::service::finalized_state::zebra_db::ZebraDb::spawn_format_change
   4: zebra_scan::storage::Storage::new
   5: zebra_scan::tests::scanning_fake_blocks_store_key_and_results
...

That's fixed by commits baaa738 and d57efb7

@teor2345 teor2345 requested a review from a team as a code owner November 29, 2023 20:34
oxarbitrage
oxarbitrage previously approved these changes Nov 29, 2023
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.

Looks great, thanks.

Base automatically changed from tests-indiv to main November 30, 2023 00:51
@mergify mergify bot dismissed oxarbitrage’s stale review November 30, 2023 00:51

The base branch was changed.

@mergify mergify bot merged commit 8c717c9 into main Nov 30, 2023
129 checks passed
@mergify mergify bot deleted the init-scanner-db branch November 30, 2023 12:59
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-state Area: State / database changes C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants