change(state): Write non-finalized blocks to the state in a separate thread, to avoid network and RPC hangs #5257

arya2 · 2022-09-26T18:05:03Z

Motivation

This PR sends queued non-finalized blocks down a channel to the writer task, which makes state block writes concurrent

Part of #4937.

Designs

Solution

Use block write task to:

Commit non-finalized blocks.
Propagate errors from queued parent blocks to queued children and skip validate_and_commit calls.
Finalize blocks on best chain past MAX_REORG_HEIGHT.

Related cleanups:

Removes unused chain_tip_sender and non_finalized_state_sender fields of StateService.

Review

This PR is part of regular scheduled work.

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?

Follow Up Work

Avoid removing and re-adding known_utxos by nesting the SentHashes and QueuedBlocks inside another struct that holds that tracks the known_utxos
Avoid now-redundant checks against the chains in the Request::AwaitUtxo call by skipping to db.utxo(&outpoint).map(|utxo| utxo.utxo) instead of routing it to the ReadRequest after checking the queue and sent hashes on StateService.
Organize block write task into an object.

arya2 · 2022-09-26T18:10:56Z

I'm still looking into why value_pool_is_updated fails and chain_tip_sender_is_updated hangs

zebra-state/src/service.rs

teor2345

This looks great, it's a good design, and it seems like it will work.

I've marked a few reviews as a "Required Fix". They are probably the bugs causing the test failures. It might help to focus on them first.

I also made some comments about refactors that would simplify the code or improve cleanup. But they aren't as important.

Feel free to ignore the nitpicks until the sync and tests are working, some of them might get deleted or moved.

Let me know if you need help with any of these changes, or if you'd like me to push a PR for any change (or all of them).

zebra-state/src/service/write.rs

zebra-state/src/service.rs

teor2345

(Just marking this so people know it has been reviewed.)

…sses

teor2345 · 2022-10-10T23:57:00Z

I compared the full sync before my sync tweaks:
https://github.com/ZcashFoundation/zebra/actions/runs/3190092327

With the full sync after the tweaks:
https://github.com/ZcashFoundation/zebra/actions/runs/3190092327

The sync after is about 25 minutes faster, and the jobs for recent blocks are faster than they used to be. So I think I'll keep most of the changes. (They should also reduce RAM usage.)

…ck UTXO spends

* Remove some verbose block write channel logs * Only warn about tracing endpoint if the address is actually set * Use CloneError instead of formatting a non-cloneable error Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

teor2345 · 2022-10-11T01:06:31Z

I added a workaround for #5125 in commit 74b29be.

That should be it for this PR, we just need to merge the checkpoint PR and get the full sync working.

…-channel

teor2345 · 2022-10-11T01:09:57Z

I'm running a full sync with the new checkpoints here:
https://github.com/ZcashFoundation/zebra/actions/runs/3223410431

If any of the sync jobs time out, we can fix the workflows, but that doesn't need to block this PR merging.
(The timeouts could be from PR #5360 or this PR, but we want to wait to fix them until we have the final times from both.)

I'm also running a full mainnet and testnet sync locally.

teor2345

I think the code is complete here, and we can do any other fixes in separate PRs.

zebrad/src/components/sync/downloads.rs

arya2 · 2022-10-11T17:52:38Z

Failed to build zebra-test in the merge queue: https://github.com/ZcashFoundation/zebra/actions/runs/3228683588/jobs/5285115640

arya2 · 2022-10-11T17:53:18Z

@Mergifyio update

mergify · 2022-10-11T17:53:56Z

update

✅ Branch has been successfully updated

arya2 requested review from teor2345 and upbqdn September 26, 2022 18:05

arya2 added C-bug Category: This is a bug P-Medium ⚡ I-slow Problems with performance or responsiveness A-state Area: State / database changes labels Sep 26, 2022

teor2345 assigned arya2 Sep 27, 2022

teor2345 reviewed Sep 27, 2022

View reviewed changes

zebra-state/src/service.rs Outdated Show resolved Hide resolved

teor2345 reviewed Sep 27, 2022

View reviewed changes

teor2345 suggested changes Sep 27, 2022

View reviewed changes

teor2345 added 19 commits September 27, 2022 13:02

Add a new block commit task and channels, that don't do anything yet

0d43b69

Add last_block_hash_sent to the state service, to avoid database acce…

63486a1

…sses

Update last_block_hash_sent regardless of commit errors

c819fb2

Rename a field to StateService.max_queued_finalized_height

5c8af25

Commit finalized blocks to the state in a separate task

5ea30f7

Check for panics in the block write task

1ba6e5b

Wait for the block commit task in tests, and check for errors

a34032d

Always run a proptest that sleeps once

2479be0

Add extra debugging to state shutdowns

725adec

Work around a RocksDB shutdown bug

9dd5e07

Close the finalized block channel when we're finished with it

bbd44b8

Only reset state queue once per error

71f4a59

Update some TODOs

932d9b4

Add a module doc comment

d8c63d3

Drop channels and check for closed channels in the block commit task

d051464

Close state channels and tasks on drop

1d5455e

Remove some duplicate fields across StateService and ReadStateService

de6ff83

Try tweaking the shutdown steps

406fcc2

Update and clarify some comments

01a4a79

arya2 changed the base branch from show-gcp-test-logs to main October 10, 2022 23:27

teor2345 mentioned this pull request Oct 10, 2022

change(state) Shares the known_utxos fields of SentBlocks and QueuedBlocks to avoid unnecessary removals and re-additions #5339

Closed

5 tasks

teor2345 removed a link to an issue Oct 10, 2022

Avoid temporary failures verifying the first non-finalized block #5125

Closed

teor2345 and others added 8 commits October 11, 2022 10:25

Fix some state service init arguments

29e765f

Allow more lookahead in the downloader, but less lookahead in the syncer

2e3eaef

Add the latest config to the tests, and fix the latest config check

987d21d

Increase the number of finalized blocks checked for non-finalized blo…

a1fcf41

…ck UTXO spends

Increase block verify timeout

8eea77a

Merge branch 'main' into non-finalized-block-commit-channel

7a926b1

Work around a known block timeout bug by using a shorter timeout

74b29be

teor2345 force-pushed the non-finalized-block-commit-channel branch from acd61c5 to 74b29be Compare October 11, 2022 01:05

Merge branch 'checkpoints-2022-10-07' into non-finalized-block-commit…

a0a65b6

…-channel

teor2345 approved these changes Oct 11, 2022

View reviewed changes

zebrad/src/components/sync/downloads.rs Show resolved Hide resolved

mergify bot added a commit that referenced this pull request Oct 11, 2022

Merge of #5257

cdd40b6

mergify bot mentioned this pull request Oct 11, 2022

merge-queue: embarking main (036a982) and #5257 together #5382

Closed

Merge branch 'main' into non-finalized-block-commit-channel

e9a5dbd

arya2 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 11, 2022

mergify bot merged commit a28350e into main Oct 11, 2022

mergify bot deleted the non-finalized-block-commit-channel branch October 11, 2022 19:25

arya2 mentioned this pull request Oct 11, 2022

Refactor QueuedBlocks and SentHashes fields on StateService to avoid unnecessary utxo removals and re-additions #5385

Closed

teor2345 mentioned this pull request Oct 12, 2022

Revert note commitment and history trees when forking non-finalized chains #4794

Closed

6 tasks

teor2345 mentioned this pull request Oct 19, 2022

ci(sync): increase the height of blocks for some full sync jobs #5391

Merged

5 tasks

DeckerSU mentioned this pull request Jan 5, 2023

Unclear implementation of block "timestamp against prev" (time-too-old) consensus rule #5914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(state): Write non-finalized blocks to the state in a separate thread, to avoid network and RPC hangs #5257

change(state): Write non-finalized blocks to the state in a separate thread, to avoid network and RPC hangs #5257

arya2 commented Sep 26, 2022 •

edited by teor2345

Loading

arya2 commented Sep 26, 2022

teor2345 left a comment

teor2345 left a comment

teor2345 commented Oct 10, 2022

teor2345 commented Oct 11, 2022

teor2345 commented Oct 11, 2022 •

edited

Loading

teor2345 left a comment

arya2 commented Oct 11, 2022

arya2 commented Oct 11, 2022

mergify bot commented Oct 11, 2022

change(state): Write non-finalized blocks to the state in a separate thread, to avoid network and RPC hangs #5257

change(state): Write non-finalized blocks to the state in a separate thread, to avoid network and RPC hangs #5257

Conversation

arya2 commented Sep 26, 2022 • edited by teor2345 Loading

Motivation

Designs

Solution

Review

Reviewer Checklist

Follow Up Work

arya2 commented Sep 26, 2022

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Oct 10, 2022

teor2345 commented Oct 11, 2022

teor2345 commented Oct 11, 2022 • edited Loading

teor2345 left a comment

Choose a reason for hiding this comment

arya2 commented Oct 11, 2022

arya2 commented Oct 11, 2022

mergify bot commented Oct 11, 2022

✅ Branch has been successfully updated

arya2 commented Sep 26, 2022 •

edited by teor2345

Loading

teor2345 commented Oct 11, 2022 •

edited

Loading