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

Shows reindex progress in metrics screen #4368

Merged
merged 4 commits into from Mar 12, 2020
Merged

Shows reindex progress in metrics screen #4368

merged 4 commits into from Mar 12, 2020

Conversation

gladcow
Copy link
Contributor

@gladcow gladcow commented Feb 20, 2020 •
edited by str4d

Resolves issue #3813.

The "Downloading blocks" message is changed to "Reindexing blocks" during reindex, after reindex is completed the text is reverted back to the first variant.

Reindex progress is shown as a sum of processed file size (we can't use reindexed block number as a progress because we can't predict how many blocks to process at all, we don't know the size of the block before we process it), the result looks like

Reindexing blocks | 22.64 MiB / 336.00 MiB (6%, 13583 blocks)

@gladcow gladcow requested a review from daira February 20, 2020 16:18
@r3ld3v r3ld3v added this to the Core Sprint 2020-09 milestone Feb 24, 2020
@str4d str4d added the A-metrics-screen Area: Metrics screen label Feb 28, 2020
str4d
str4d requested changes Feb 29, 2020
View reviewed changes
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Concept ACK. Yes, I would like formatting for the byte sizes; something akin to DisplayDuration() would be great (except without the FULL vs REDUCED format distinction; simply auto-scaling from e.g. kiB -> MiB with one or two decimal places would be ideal).

src/main.cpp Outdated
Comment on lines 70 to 71
std::atomic<size_t> nSizeReindexed(0); // valid only during reindex
std::atomic<size_t> nFullSizeToReindex(1); // valid only during reindex
Copy link
Contributor

Choose a reason for hiding this comment

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

As these are only consumed by the metrics screen, define these in metrics.cpp alongside the other metrics globals. That also makes it easier to refactor the metrics screen later on to reduce globals.

src/main.h Outdated
Comment on lines 144 to 145
extern std::atomic<size_t> nSizeReindexed; // valid only during reindex
extern std::atomic<size_t> nFullSizeToReindex; // valid only during reindex
Copy link
Contributor

Choose a reason for hiding this comment

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

These then move to metrics.h.

@gladcow gladcow force-pushed the issue3813_reindex_in_metrics branch from 72fc4be to 8fd575a Compare March 5, 2020 13:40
@gladcow
Copy link
Contributor Author

gladcow commented Mar 5, 2020

@str4d , thank you for your review. I've addressed your comments and have rebased this PR to current master, re-review it, please.

daira
daira previously requested changes Mar 5, 2020
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.

Please fix the race condition on fReindex.

src/metrics.cpp Show resolved Hide resolved
@daira
Copy link
Contributor

daira commented Mar 5, 2020 •
edited

I'm generally skeptical about the style of using atomics; we do use them elsewhere but very sparingly, since they're quite error-prone. Is there an applicable existing lock we can use instead (e.g. cs_main if that doesn't introduce too much contention)?

@gladcow
Copy link
Contributor Author

gladcow commented Mar 5, 2020

@daira , thank you for review. I'll try to use other way instead of atomics.

@daira
Copy link
Contributor

daira commented Mar 5, 2020 •
edited

Actually upstream makes fReindex atomic: bitcoin/bitcoin#11107
So they might as well all be atomic. I suggest pulling in those upstream changes (except for the last commit, because we fixed the race conditions on fUseCrypto in #3932) as a separate PR, and then making this one dependent on it.

@gladcow gladcow mentioned this pull request Mar 7, 2020
Merged
@gladcow
Copy link
Contributor Author

gladcow commented Mar 7, 2020

I've created backporting PR, but I don't know how to make this PR dependent on it. Just rebase this branch on its branch?

@gladcow gladcow requested review from daira and str4d March 7, 2020 08:15
@daira
Copy link
Contributor

daira commented Mar 8, 2020

@gladcow wrote:

I've created backporting PR, but I don't know how to make this PR dependent on it. Just rebase this branch on its branch?

Yes.

@gladcow gladcow force-pushed the issue3813_reindex_in_metrics branch from 8fd575a to 0579123 Compare March 8, 2020 13:30
@gladcow
Copy link
Contributor Author

gladcow commented Mar 8, 2020

I've rebased this PR on #4387

zkbot added a commit that referenced this pull request Mar 9, 2020
@zkbot
Auto merge of #4387 - gladcow:issue3813_backport_upstream_11107, r=daira
8c48786 
Fix races in AppInitMain and others with lock and atomic bools (Bitcoin backport)

Backport bitcoin bitcoin/bitcoin#11107 (excluding the last commit,  it was fixed in Zcash before).

This functionality is required for #4368 (details #4368 (comment)) as part of the issue #3813 resolution.
@str4d
Copy link
Contributor

str4d commented Mar 10, 2020

Please rebase this on master so that this PR only contains the relevant commits.

@gladcow gladcow force-pushed the issue3813_reindex_in_metrics branch from 0579123 to fc2501b Compare March 10, 2020 06:50
@gladcow
Copy link
Contributor Author

gladcow commented Mar 10, 2020

@str4d , done.)

str4d
str4d approved these changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@str4d str4d 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 str4d dismissed daira’s stale review March 12, 2020 06:49

Comment addressed.

@str4d
Copy link
Contributor

str4d commented Mar 12, 2020

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Mar 12, 2020

📌 Commit fc2501b has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Mar 12, 2020

⌛ Testing commit fc2501b with merge c0802cc...

@zkbot
Copy link
Contributor

zkbot commented Mar 12, 2020

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

@zkbot zkbot merged commit c0802cc into zcash:master Mar 12, 2020
@daira daira mentioned this pull request Mar 16, 2020
Closed
daira
daira reviewed Mar 16, 2020
View reviewed changes
src/init.cpp
nFile++;
fullSize += boost::filesystem::file_size(blkFile);
}
nFullSizeToReindex = fullSize;
Copy link
Contributor

@daira daira Mar 16, 2020 •
edited

Choose a reason for hiding this comment

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

I missed a potential divide-by-zero. If all block files are zero-length, then https://github.com/zcash/zcash/pull/4368/files#diff-9bee7abc3e254b85a1ce1ed330601054R291 can divide by zero.

daira added a commit to daira/zcash that referenced this pull request Mar 16, 2020
@daira
Avoid a theoretical possibility of division-by-zero introduced in zca…
b49cdee 
…sh#4368.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
zkbot added a commit that referenced this pull request Mar 16, 2020
@zkbot
Auto merge of #4404 - daira:fix-reindex-divby0, r=ebfull
fc9d0fa 
Avoid a theoretical possibility of division-by-zero introduced in #4368

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daira daira daira left review comments

@str4d str4d str4d approved these changes

Assignees
No one assigned
Labels
A-metrics-screen Area: Metrics screen
Projects
None yet
Milestone
Core Sprint 2020-09
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gladcow @daira @str4d @zkbot @r3ld3v