Skip to content

Optimise reindex #5268

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
Merged
zkbot merged 7 commits into from
Aug 10, 2021
Merged

Optimise reindex #5268

zkbot merged 7 commits into from
Aug 10, 2021

Conversation

@str4d
Copy link
Contributor

@str4d str4d commented Aug 9, 2021

Backports bitcoin/bitcoin#7917:

Several changes:

  • Make reindex/import use AcceptBlock rather than ProcessNewBlock,
    so activation of the resulting best chain is delayed.

  • That delayed activation is handled by the existing "Activating best
    chain" step in the startup process, which is moved to ThreadImport.

  • Optimize ActivateBestChain to not walk the entire chain to find the
    most work chain to connect for each block (O(n^2) -> O(n)).

This has several advantages:

  • As the actual activation is done after all blocks have been added
    back to the index, it gets to use the checkpoints information (better
    progress indicator, skipping of signature checks, ...).

  • Having a block index with many unactivated blocks (for example, after
    deleting the chainstate directory) becomes much faster

  • That case also runs in the background now (as it's simply treated as
    part of the importing process).

Downsides:

  • All blocks are read twice from disk during reindex (once to rebuild
    the index, once to activate).

sipa and others added 6 commits August 9, 2021 20:58
@str4d str4d added I-performance Problems and improvements with respect to performance C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. labels Aug 9, 2021
@str4d str4d added this to the Core Sprint 2021-30 milestone Aug 9, 2021
str4d
str4d commented Aug 9, 2021
View reviewed changes
qa/rpc-tests/reindex.py
Comment on lines +33 to +34
while self.nodes[0].getblockcount() < blockcount:
time.sleep(0.1)
Copy link
Contributor Author

@str4d str4d Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This incidental change should fix the race condition we've been seeing in CI.

This was referenced Aug 9, 2021
Open
Merged
@daira
Copy link
Contributor

daira commented Aug 9, 2021 •
edited
Loading

Upstream had a comment saying to add "reindex" to the list of debug categories, but that doesn't appear to have been done, either in the upstream PR or this one.

daira
daira approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@str4d
Copy link
Contributor Author

str4d commented Aug 9, 2021

Upstream had a comment saying to add "reindex" to the list of debug categories, but that doesn't appear to have been done, either in the upstream PR or this one.

We already have it there (probably accidentally backported in some previous PR).

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Aug 9, 2021

📌 Commit c173a26 has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Aug 9, 2021

⌛ Testing commit c173a26 with merge ab8ed50c4edb0040c587bbfe06db984a313ca92e...

@zkbot
Copy link
Contributor

zkbot commented Aug 9, 2021

💔 Test failed - pr-merge

@str4d
test: Flush wallet in WriteCryptedSaplingZkeyDirectToDb before reopening
01f66cd 
Hopefully this should reliably prevent the race condition we see in CI.
@str4d
Copy link
Contributor Author

str4d commented Aug 9, 2021

Hopefully I've fixed the transient CI failure.

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Aug 9, 2021

📌 Commit 01f66cd has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Aug 9, 2021

⌛ Testing commit 01f66cd with merge f721e3b...

nuttycom
nuttycom approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@zkbot
Copy link
Contributor

zkbot commented Aug 10, 2021

☀️ Test successful - pr-merge
Approved by: str4d
Pushing f721e3b to master...

@zkbot zkbot merged commit f721e3b into zcash:master Aug 10, 2021
@str4d str4d deleted the optimise-reindex branch August 10, 2021 00:22
@miodragpop miodragpop mentioned this pull request May 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nuttycom nuttycom nuttycom approved these changes

@daira daira daira approved these changes

Assignees

No one assigned

Labels

C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. I-performance Problems and improvements with respect to performance

Projects

None yet

Milestone

Core Sprint 2021-30

Development

Successfully merging this pull request may close these issues.

5 participants

@str4d @daira @zkbot @nuttycom @sipa