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

[Qt] Show progress percent for zpiv reindex operations #612

Merged
merged 3 commits into from Jun 28, 2018

Conversation

Projects
None yet
4 participants
@Fuzzbawls
Copy link
Collaborator

Fuzzbawls commented May 12, 2018

-reindexaccumulators and -reindexzerocoin can take a considerable
time to complete depending on system hardware. Lets show a progress percent
similar to VerifyDB() on the splashscreen.

@Fuzzbawls Fuzzbawls added the GUI label May 12, 2018

@wafflebot wafflebot bot added the review label May 12, 2018

@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator Author

Fuzzbawls commented May 12, 2018

I'll rebase this when #609 is merged, but wanted to get the concept up.

@presstab

This comment has been minimized.

Copy link
Collaborator

presstab commented May 12, 2018

Concept ACK

[Qt] Show progress percent for zpiv reindex operations
`-reindexaccumulators` and `-reindexzerocoin` can take a considerable
time to complete depending on system hardware. Lets show a progress percent
 similar to `VerifyDB()` on the splashscreen.

@Fuzzbawls Fuzzbawls force-pushed the Fuzzbawls:2018_reindexz-progress branch to 48e502a May 22, 2018

@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator Author

Fuzzbawls commented May 22, 2018

Rebased now that #609 has been merged

@Mrs-X
Copy link
Collaborator

Mrs-X left a comment

if (!listMissingCheckpoints.empty() && chainActive.Height() >= Params().Zerocoin_StartHeight())

and

(pindex->nHeight - nZerocoinStart) / (double)(chainActive.Height() - nZerocoinStart)

When chainActive.Height() == Params().Zerocoin_StartHeight() we would have a division by zero.

@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator Author

Fuzzbawls commented May 27, 2018

looks like the if conditions weren't updated for v2 zPIV.

@presstab should the if instead read as

if (!listMissingCheckpoints.empty() && chainActive.Height() >= Params().Zerocoin_Block_V2_Start()) {

since we set pindex to Params().Zerocoin_Block_V2_Start() for looping just a few lines below (thus ignoring any block height prior to v2 start height)?

Guard against division by zero for reindex zerocoin/accumulators
in the event that the local chainActive is equal or below that of the
relevant zerocoin block heights, there is nothing to do, so bypass
the internal process.
@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator Author

Fuzzbawls commented Jun 9, 2018

adjusted some of the logic for these two reindex options, as well as added checks to see if they should even run based on the client's chainActive height. (no point in running either if the local chainActive is at a height below the respective zPIV activation values.

@Fuzzbawls Fuzzbawls requested a review from presstab Jun 11, 2018

@Warrows
Copy link
Collaborator

Warrows left a comment

ACK bed79e2

@Mrs-X
Copy link
Collaborator

Mrs-X left a comment

ACK but a little NIT:

  • I would prefer to have the call of ReindexAccumulators() in the if (chainActive.Height() > Params().Zerocoin_Block_V2_Start()) {block to make the fulfilled pre-condition obvious to the reader

For ReindexZerocoinDB) it's done already like this and was (at least for me) much easier to verify.

What do you think?

Move ReindexAccumulators() call to be inside parent conditional
Not much point in checking if the listAccCheckpointsNoDB list is empty
outside of when the wallet is started using `-reindexaccumulators`, as
it is always empty outside of that case.
@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator Author

Fuzzbawls commented Jun 26, 2018

@Mrs-X good point, moved it up a bit.

@Mrs-X

Mrs-X approved these changes Jun 28, 2018

Copy link
Collaborator

Mrs-X left a comment

ACK and merging...

@Mrs-X Mrs-X merged commit 5127486 into PIVX-Project:master Jun 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mrs-X added a commit that referenced this pull request Jun 28, 2018

Merge #612: [Qt] Show progress percent for zpiv reindex operations
5127486 Move ReindexAccumulators() call to be inside parent conditional (Fuzzbawls)
bed79e2 Guard against division by zero for reindex zerocoin/accumulators (Fuzzbawls)
48e502a [Qt] Show progress percent for zpiv reindex operations (Fuzzbawls)

Tree-SHA512: 2b2ac1444494b209a4d9825bf3caa516342c70fc8e55cd9f2ae7d34816cc8f7ea363c48aa30acb06f49c38933312f44525009eed0d838e258f8c3fd3476d92e5

@wafflebot wafflebot bot removed the review label Jun 28, 2018

@Fuzzbawls Fuzzbawls added this to the 3.1.1 milestone Jul 5, 2018

Fuzzbawls added a commit to Fuzzbawls/PIVX that referenced this pull request Jul 6, 2018

[Qt] Show progress percent for zpiv reindex operations
`-reindexaccumulators` and `-reindexzerocoin` can take a considerable
time to complete depending on system hardware. Lets show a progress percent
 similar to `VerifyDB()` on the splashscreen.

Github-Pull: PIVX-Project#612
Rebased-From: 48e502a

Fuzzbawls added a commit to Fuzzbawls/PIVX that referenced this pull request Jul 6, 2018

Guard against division by zero for reindex zerocoin/accumulators
in the event that the local chainActive is equal or below that of the
relevant zerocoin block heights, there is nothing to do, so bypass
the internal process.

Github-Pull: PIVX-Project#612
Rebased-From: bed79e2

Fuzzbawls added a commit to Fuzzbawls/PIVX that referenced this pull request Jul 6, 2018

Move ReindexAccumulators() call to be inside parent conditional
Not much point in checking if the listAccCheckpointsNoDB list is empty
outside of when the wallet is started using `-reindexaccumulators`, as
it is always empty outside of that case.

Github-Pull: PIVX-Project#612
Rebased-From: 5127486
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.