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

2. change(state): Run AwaitUtxo read requests without shared mutable chain state #5107

Merged
merged 16 commits into from
Sep 16, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Sep 7, 2022

Motivation

This PR runs StateService::AwaitUtxo read requests without accessing shared mutable chain state.
As a useful side-effect, this makes all StateService read requests concurrent.

Closes #5102.

Designs

See #4937 and https://docs.google.com/drawings/d/1FXpAUlenDAjl8nkftrypdAPsj0jr-Ut9gZlSP57nuyc/edit

Solution

Don't access shared mutable chain state in StateService read requests:

  • Re-Implement AwaitUtxo without accessing shared mutable chain state, using StateService.pending_utxos and ReadStateService::AnyUtxo
    • Send the whole NonFinalizedState to the ReadStateService
    • Rename the ChainUtxo request to BestChainUtxo
    • Add an AnyChainUtxo request

Related documentation:

  • Update the state service module documentation
  • Explain how spent UTXOs are treated by the state
  • Clarify how cached Chains impact state read requests

Related Cleanups:

  • Move AwaitUtxos next to the other shared writeable state requests
  • Rename ReadResponse::Utxos to ReadResponse::AddressUtxos
  • Rename an out_point variable to outpoint for consistency
  • Rename transparent_utxos to address_utxos
  • Move the QueuedBlock type into the queued_blocks module

Testing

We should run a full sync on this before we merge, and make sure it is not significantly slower than the main branch.

Review

This PR is part of regular scheduled work.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

Actually create a separate task to commit blocks to the state.

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #5107 (019a75a) into main (806dd0f) will decrease coverage by 0.05%.
The diff coverage is 79.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5107      +/-   ##
==========================================
- Coverage   79.35%   79.29%   -0.06%     
==========================================
  Files         307      307              
  Lines       39259    39323      +64     
==========================================
+ Hits        31153    31181      +28     
- Misses       8106     8142      +36     

@teor2345

This comment was marked as outdated.

@mergify

This comment was marked as outdated.

@teor2345 teor2345 changed the title doc(state): Add TODOs for a separate block commit task change(state): Run StateService read requests without access shared mutable chain state Sep 8, 2022
@teor2345 teor2345 changed the title change(state): Run StateService read requests without access shared mutable chain state change(state): Run StateService read requests without shared mutable chain state Sep 8, 2022
@teor2345 teor2345 self-assigned this Sep 8, 2022
@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium ⚡ I-slow Problems with performance or responsiveness A-state Area: State / database changes labels Sep 8, 2022
@teor2345

This comment was marked as outdated.

@teor2345

This comment was marked as outdated.

@mergify

This comment was marked as outdated.

@teor2345
Copy link
Contributor Author

teor2345 commented Sep 8, 2022

I'm running a full sync and a checkpoint sync here:
https://github.com/ZcashFoundation/zebra/actions/runs/3018034862

@teor2345 teor2345 marked this pull request as ready for review September 8, 2022 20:31
@teor2345 teor2345 requested a review from a team as a code owner September 8, 2022 20:31
@teor2345 teor2345 requested review from oxarbitrage, upbqdn and arya2 and removed request for a team and oxarbitrage September 8, 2022 20:31
@teor2345
Copy link
Contributor Author

teor2345 commented Sep 8, 2022

I did a full sync and an update sync locally:

  • both succeeded on testnet
  • update sync succeeded on mainnet
  • full sync is still going on mainnet, it seems to be about the same speed, there are no warnings

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

I'm running a full sync here, but we don't need to wait for it before we merge this PR:
https://github.com/ZcashFoundation/zebra/actions/runs/3047965824

Base automatically changed from partial-state-concurrent to main September 14, 2022 00:35
@teor2345 teor2345 requested review from arya2 and upbqdn September 14, 2022 23:10
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.

All looks good. I left some minor comments.

zebra-state/src/service/read/block.rs Show resolved Hide resolved
zebra-state/src/service.rs Show resolved Hide resolved
zebra-state/src/service.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code A-state Area: State / database changes C-bug Category: This is a bug I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make StateService requests into concurrent ReadStateService requests
3 participants