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(state): Check block and transaction Sprout anchors in parallel #5742

Merged
merged 4 commits into from
Dec 1, 2022
Merged

change(state): Check block and transaction Sprout anchors in parallel #5742

merged 4 commits into from
Dec 1, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Nov 29, 2022 •
edited
Loading

Motivation

We want to run these anchor verifications on a CPU-bound thread pool.

Solution

  • Use a parallel iterator when checking sprout_anchors_refer_to_treestates(..) for a block if sprout_final_treestates is not empty.
  • Use in_place_scope_fifo for running sprout anchors checks on an UnminedTx if the transaction has sprout joinsplit_data

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

@arya2 arya2 requested a review from a team as a code owner November 29, 2022 20:23
@arya2 arya2 requested review from oxarbitrage and teor2345 and removed request for a team November 29, 2022 20:23
@arya2 arya2 self-assigned this Nov 29, 2022
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Nov 29, 2022
@arya2 arya2 added A-rust Area: Updates to Rust code P-Medium ⚡ A-state Area: State / database changes and removed C-enhancement Category: This is an improvement labels Nov 29, 2022
@arya2 arya2 mentioned this pull request Nov 29, 2022
Merged
5 tasks
@arya2 arya2 changed the title change(state): check anchors in parallel change(state): Check anchors in parallel Nov 29, 2022
@arya2 arya2 force-pushed the check-anchors-in-parallel branch from 18f3d33 to 17aa356 Compare November 29, 2022 21:27
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Nov 29, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022 •
edited
Loading

Codecov Report

Merging #5742 (fb37958) into main (26ff1b4) will increase coverage by 0.03%.
The diff coverage is 86.20%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5742      +/-   ##
==========================================
+ Coverage   78.74%   78.77%   +0.03%     
==========================================
  Files         306      306              
  Lines       38687    38702      +15     
==========================================
+ Hits        30463    30488      +25     
+ Misses       8224     8214      -10

teor2345
teor2345 suggested changes Nov 29, 2022
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.

We try to only make performance-related changes where we have evidence of slow code or a denial of service risk.

Otherwise, we could spend a lot of time modifying code that's not performance critical, making it harder to maintain. And sometimes the setup costs exceed any performance gains.

zebra-state/src/service/check/anchors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/anchors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/anchors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/anchors.rs Show resolved Hide resolved
zebra-state/src/service/check/anchors.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

Failed due to:

Unable to find image 'us-docker.pkg.dev/zealous-zebra/zebra/zebrad-test:sha-e10d851' locally
docker: Error response from daemon: Get "https://us-docker.pkg.dev/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers).

https://github.com/ZcashFoundation/zebra/actions/runs/3578145576/jobs/6018860424#step:8:80

@gustavovalverde any idea why this is happening?

teor2345
teor2345 previously approved these changes Nov 30, 2022
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 good!

@teor2345 teor2345 changed the title change(state): Check anchors in parallel change(state): Check block and transaction Sprout anchors in parallel Nov 30, 2022
@teor2345 teor2345 added C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage labels Nov 30, 2022
@teor2345
Copy link
Contributor

Failed due to #5384

Base automatically changed from mempool-context-valid to main November 30, 2022 04:40
@mergify mergify bot requested a review from a team as a code owner November 30, 2022 04:40
@teor2345
Copy link
Contributor

This will need a rebase, anyone can re-approve once that's done.

@teor2345 teor2345 removed request for oxarbitrage and a team November 30, 2022 07:40
@arya2 arya2 force-pushed the check-anchors-in-parallel branch from b8d18d8 to fb37958 Compare November 30, 2022 18:20
@teor2345
Copy link
Contributor

Failed due to a temporary network error:

buildx failed with: ERROR: failed to solve: failed to compute cache key: failed to copy: read tcp 172.17.0.2:53916->142.250.114.82:443: read: connection timed out

https://github.com/ZcashFoundation/zebra/actions/runs/3586007091/jobs/6034687982#step:9:503

mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify
Merge of #5742
94ba668
@mergify mergify bot mentioned this pull request Dec 1, 2022
Closed
@arya2
Copy link
Contributor Author

arya2 commented Dec 1, 2022

Failed due to #5384 again.

mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify
Merge of #5742
283cf57
@mergify mergify bot mentioned this pull request Dec 1, 2022
Closed
mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify
Merge of #5742
033bcfb
@mergify mergify bot mentioned this pull request Dec 1, 2022
Closed
mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify
Merge of #5742
a85d67c
@mergify mergify bot mentioned this pull request Dec 1, 2022
Closed
@teor2345
Copy link
Contributor

teor2345 commented Dec 1, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Dec 1, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify
Merge of #5742
fc288f7
@mergify mergify bot mentioned this pull request Dec 1, 2022
Closed
@mergify mergify bot merged commit c838383 into main Dec 1, 2022
@mergify mergify bot deleted the check-anchors-in-parallel branch December 1, 2022 11:35
teor2345 pushed a commit that referenced this pull request Feb 6, 2023
@arya2 @teor2345
change(state): Check block and transaction Sprout anchors in parallel (…
fd7fdb9 
…#5742)

* parallelize anchors checks for blocks

* parallelize anchors checks for unmined_tx

* reverts par_iter in block_sapling_orchard_anchors_refer_to_final_treestates

* moves fetch_sprout_final_treestates out of rayon thread
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

@arya2 arya2

Labels
A-rust Area: Updates to Rust code A-state Area: State / database changes C-enhancement Category: This is an improvement C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arya2 @teor2345