[Staking][Zerocoin][GUI] Show number of confirmations until staking becomes active

[Rock-N-Troll]

Issue Reference: #889

New method GetConfirmationsRemainingForStaking() in ztracker.cpp

• Returns -2 if the ztracker is not fully initialized
• Returns -1 if there are no zerocoin in the wallet (which will display typical You need some zerocoin text)
• Returns 0 if there is at least 1 zerocoin mature enough for staking in the wallet.
• Otherwise returns the number of confirmations remaining until the most mature zerocoin in the wallet is mature enough to stake.

[Rock-N-Troll]

It might be best to just display "Staking Enabled" whenever the value is zero confirmations remaining as it would prevent the wallet from shifting back and forth. It would also be most accurate.

[WetOne]

Looking at what was line 66 in veilstatusbar.cpp, fstakingactive is set depending on if enough blocks passed to allow for staking to be active. If I'm reading this right, the new code checks to ensure mapHashedBlocks is not empty. This could lead to a discontinuity between the slider indicating that staking is active and staking actually being active. See issue #880 for reference about what I'm on about.

Please squash you commits.

[Rock-N-Troll]

I added a line of code to clear the mapHashedBlocks map in miner.cpp line 901 when staking is not enabled so there shouldn't be a discontinuity between actively staking and otherwise.

The reason I went with this approach is because I have periodically noticed the wallet showing "Enabling..." many times when it was "Staking Active" for a long time. Additionally, the map for hashing calculations would be a more concrete way of assuring staking is happening, or not, than looking at time between hashings as is related to the issue you mentioned in pull request #880:

Once synchronization is complete, the staking slider will be changed to enabled with the state of "Enabling .... When finishing synchronization testing showed some thrashing between the states of "Enabling..." and "Staking Enabled" until a few blocks have gone by. This is due to the way active staking is calculated.

At a higher level, if the mapHashedBlocks map is not empty, then the wallet must be staking to add values to it.
Also, assuming the line I added to clear the mapHashedBlocks when staking is not enabled (which happens before the next round of hashing anyway) is accurate, it too will be indicative of the wallet not staking. I essentially tried to turn the mapHashedBlocks.empty() check into a flag of staking vs not.

Thank you for your input. I will squash the commits after performing some fix ups.

I also think there might be a more elegant way to handle the confirmations than looping through the list of zerocoins in the wallet to get the minimum number of remaining confirmations before staking can begin. Just noticed this can minimally be handled with code leveraged from MintableCoins() in wallet.cpp. Will revisit.

[WetOne]

If I'm understanding you correctly, staking would then be considered active one block after the user has enabled staking.

I should have been more clear regarding the discontinuity. The discontinuity I was referring to was between the cli and GUI (the genesis of issue 880). The code being removed was lifted from the cli to allow users to have the same information from both interfaces (humorously, the issue I'm referring to here is #504 authored by you, ha :) ). If you are changing how active staking is being confirmed through the GUI, then you should also change the same in the cli.

If @codeofalltrades and @CaveSpectre11 are OK with staking being dependent on a single block passing after user enables staking, then I am as well.

[Rock-N-Troll]

If you have mature zerocoin in your wallet, the wallet will be staking. That logic has not been changed. The logic to detect if the wallet is actively or not actively staking has changed. The bools in veilstatusbar.cpp are strictly for display purposes anyway (local to the veilstatusbar.cpp code checks), if I'm understanding your point correctly.

[Rock-N-Troll]

If you are changing how active staking is being confirmed through the GUI, then you should also change the same in the cli.

Yes I will need to change that as well. Good catch

[Rock-N-Troll]

squashed, but still need to apply changes to CLI across the board for stakingactive indicator

[Rock-N-Troll]

Note: The icon sizes for the toggle have shrunk a little bit, but I've noticed there was clipping in many screenshots before and if desired the svgs for the vector images should be more identical than previous (which was off-center and more top heavy as a result if you scroll up to prior version).

New Example:

[Rock-N-Troll]

I'm going to do some additional testing and perform the pull request writeup as the next step for anyone who wants to give additional feedback, particularly in regards to the logic WetOne mentions.

-added isStakingActive() bool to wallet.cpp which is now also used for staking_active in getWalletInfo() RPC call as well as part of the blue toggle switch checks in veilstatusbar.cpp
-code now clears mapHashedBlocks on staking thread exit or sleep.
-code also leverages existing check for stakable, mature zerocoin via SelectStakeCoins method when determining overall wallet staking status.
-getConfirmationsRemainingForStaking() is now only called if staking is enabled and staking_active is false.

[CaveSpectre11]

This would be a tough test to time; but maybe it would be best tested on testnet or maybe even in regtest. I would like to see the coin(s) mature, it go to Staking Enabled, and then return to showing the number of confirmations needed after they staked. Not sure the best way to simply test it without a closed network.

[Rock-N-Troll]

@CaveSpectre11 additionally can have staking enabled (with mature zerocoin), then spend the coins while also minting new ones, or some combination of those events.

[Rock-N-Troll]

Added a timer to check the staking status more frequently (every 5 seconds until the status changes) when staking is Enabling... or Disabling... . The reason being it previously only updated when a new block comes in or some other GUI trigger of the veilstatusbar which isn't very often.

[Rock-N-Troll]

with the latest commit, you should be able to toggle the status bar to your heart's content and it will always be accurate :)

[Rock-N-Troll]

I would like to add a feature where clicking the "Unlock wallet for staking" toggle bottom left of the screen brings up the pop-up to unlock the wallet for staking only as is depicted.

Current State: Only the toast dialogue shown below displays (no form pop-up). You still have to go to the settings right side tab to unlock wallet for staking only.
Suggested State: Now both toast dialogue and the form pop-up to unlock wallet for staking will display as shown on click, as the user's attention is to stake and should be taken to the action needed to do so (code is all ready).

Looking for feedback about this change. It touches the same code I've already had to modify as part of this pull request as well. Thanks

[CaveSpectre11]

Looking for feedback about this change. It touches the same code I've already had to modify as part of this pull request as well. Thanks

Please try to keep PRs autonomous. Separate PRs can touch the same code areas but unless they're actually dependent on each other, they should be separate PRs; otherwise the PR discussion gets overly confusing and code reviews and testing becomes even more complicated. Even if a PR is dependent on another PR; that second PR can be noted that it includes and is a follow on to another PR (and you can cherry pick the previous PR's commit into the new PR).

Please make this an issue if you'd like discussion on it, and if it's completed PR ends up with conflicts after this merges, the conflicts can be easily addressed when rebased with the master branch.

[Rock-N-Troll]

Created pull request #924 in response to my latest feature addition question.

[Rock-N-Troll]

After some extra thought, I think I might want to add an additional mapHashedBlocks.clear() whenever the selectStakeCoins method determines there's no stakable zerocoins. Just to cover the edge case where the wallet sends out the only eligible mature stakable zerocoin while looping in the staking loop (which won't clear out the mapHashedBlocks).

UPDATE: Added

[codeofalltrades]

Is this ready for QA?

[Rock-N-Troll]

Ready for QA

[CaveSpectre11]

Indenting issues in miner.cpp, veilstatusbar.cpp and ztracker.cpp are extensive enough that it goes beyond what I can let slide on a NIT. Definitely need those addressed.

I can accept 2 space indents on tight code, but standard should be 4 space indents for sections.

"4 space indentation (no tabs) for every block except namespaces."
(from https://github.com/Veil-Project/veil/blob/master/doc/developer-notes.md#coding-style-general)

[CaveSpectre11]

NIT fix the indentation

[CaveSpectre11]

This indentation is not only off the standard style, but it's off the local code indenting as well

[Rock-N-Troll]

Would like functionality checks if anyone has the capacity to check. Will address the indenting if functionality is correct or when I go back to fix-up the functionality, whichever comes first

[CaveSpectre11]

has conflicts to resolve as well.