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

change(scan): Refactor scanning tests #8047

Merged
merged 9 commits into from
Dec 6, 2023
Merged

change(scan): Refactor scanning tests #8047

merged 9 commits into from
Dec 6, 2023

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Dec 4, 2023 •
edited
Loading

Motivation

Address #7994 (comment).

This PR creates fake blocks in Zebra's format that are worth scanning and adjusts existing tests. We can also leverage this functionality in further tests. I expect we'll need to refine the API for fake blocks later on when we need more complicated tests.

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

  • Create fake Zebra blocks:
    • Use the already existing fake_compact_block function to create fake compact blocks.
    • Convert transactions from the compact format to Zebra's V4.
    • Create fake Zebra blocks using the converted transactions.
  • Refactor existing tests to use the fake Zebra's blocks.
  • Fix some nits in the existing tests.

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.

@upbqdn upbqdn added A-rust Area: Updates to Rust code P-Medium ⚡ A-blockchain-scanner Area: Blockchain scanner of shielded transactions labels Dec 4, 2023
@upbqdn upbqdn self-assigned this Dec 4, 2023
@upbqdn upbqdn requested review from a team as code owners December 4, 2023 16:24
@upbqdn upbqdn requested review from teor2345 and removed request for a team December 4, 2023 16:24
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Dec 4, 2023
@teor2345
Copy link
Contributor

teor2345 commented Dec 4, 2023

I think CI on this PR was impacted by the self-hosted runner changes, so I'm going to cancel and restart those jobs.

@teor2345
Copy link
Contributor

teor2345 commented Dec 4, 2023

Re-running failed jobs didn't work, I'm now trying re-running all jobs in the hung workflow.

Next step is to make a code change or update from main, so that we have a new GitHub merge commit, and everything runs fresh.

teor2345
teor2345 reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@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, looks great. Just a minor comment about Default and consensus-critical code.

zebra-chain/src/block/hash.rs Outdated Show resolved Hide resolved
@upbqdn
Put Default behind cfg_attr(test)
b0bdf17 
Rationale
---------

We avoid implementing `Default` on consensus-critical types because it's
easy to miss an incorrect use in a review. It's easy to hide a
`default()` in a call like `unwrap_or_default()` or even more subtle
methods.
@teor2345
Copy link
Contributor

teor2345 commented Dec 5, 2023

We appear to be having issues with runners again, pinging @gustavovalverde

teor2345
teor2345 previously approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@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 the fix!

@upbqdn upbqdn requested a review from teor2345 December 5, 2023 23:37
@upbqdn
Copy link
Member Author

upbqdn commented Dec 5, 2023

There was a merge conflict that required manual resolution.

teor2345
teor2345 approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

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

mergify bot added a commit that referenced this pull request Dec 6, 2023
@mergify
Merge of #8047
88bc02d
@mergify mergify bot mentioned this pull request Dec 6, 2023
Closed
@mergify mergify bot merged commit 7c6a0f8 into main Dec 6, 2023
128 checks passed
@mergify mergify bot deleted the scannin-tests-refactor branch December 6, 2023 01:57
@mpguerra mpguerra linked an issue Dec 11, 2023 that may be closed by this pull request
Create a scan_block function to use across scanning tasks #7947
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

Assignees

@upbqdn upbqdn

Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-rust Area: Updates to Rust code C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a scan_block function to use across scanning tasks
2 participants
@upbqdn @teor2345