[GUI] Update Staking Slider to be reflective of status #880

WetOne · 2020-12-06T17:07:58Z

Problem

The staking slider does not always indicate the present state of staking. There is a delay between when staking is enabled/disabled and when the actual enabling/disabling takes place.

Root Cause

The Qt indicates the state that the wallet is in. Some amount of time is required to ensure that the state change has taken affect. Additionally, staking is not performed while syncing. When the user initially opens the user interface, staking is displayed as active while syncing. Staking is initially active but will not be performed until syncing is complete.

Solution

Text for the staking slider now shows the current state of actual staking (enabled, disable, disabled for sync, enabling ..., disabling...).
The staking slider is disabled (cannot be changed) until syncing is complete.
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.
RPC getwalletinfo updated to show the actual state of staking and the wallet status
Popup entering into staking disabled has been updated to indicate that some time is required prior to the state change taking effect.

Issues Addressed

245: #245
504: #504

Bounty Payment Address

sv1qqphsvuwhk29xcn2q9gdth9x73qpm8kcktmuxyd8szl50sgt2nt47egpqwnxr83hfj7s6fnec2jr0cv5yc7r6s7nvxhd9969qrgy4cgnznwewqqqauut5m

CaveSpectre11 · 2020-12-06T21:09:34Z

src/qt/veil/veilstatusbar.cpp

+        }
+        bool fStakingActive = false;
+        if (nTimeLastHashing)
+            fStakingActive = GetAdjustedTime() + MAX_FUTURE_BLOCK_TIME - nTimeLastHashing < 70;


Define 70 as a static const, with a reflective name and description; then use it here and in the toast dialog further down

I'm OK with using it within the time check.

I suggest changing the toast to something along the lines of: "This may take a few minutes" as

The display changing in the Qt is a function of when staking is active and the calls are made (there is no periodic task that performs this check, only when new blocks come in). In my testing I've had a few minutes pass before the display is updated though calls through the command line show active already. This has to do with how the display is updated (when the command line is used, the calculation is done on the spot).

The 70 was copied from other code and I have no knowledge of what the actual high water mark is.

The comment is merely to adjust away from using magic numbers. Instead of "70" hardcoded in two places [the code and the message], make it
static const STAKING_DELAY = 70

or something to that effect; so that it's defined; and if adjusted anytime in the future you don't have the message (stating 70 seconds) and the code diverging.

Updated the message to be less definitive of the timeframe (as the user won't see the change real-time). Added the constant and used it in the check.

Did you take care of the "enabling..." thing while it's synching?

Yup. Works like it should now. Gave it a quick test too.

seanPhill · 2020-12-10T09:40:01Z

Looking good so far. Status while syncing a testnet wallet on a ancient MacBook that was a month behind.
"Staking Disabled while Syncing"

codeofalltrades

NACK 7556f9b

codeofalltrades · 2020-12-11T03:05:59Z

src/qt/veil/veilstatusbar.cpp

+	if (fStakingActive) {
+            ui->checkStaking->setText("Staking Enabled");
+	}else{
+	    ui->checkStaking->setText("Enabling...");


I think we need to checked for a locked wallet here. If the wallet is locked, display "unlock wallet for staking"

More of NIT, when the wallet is unlocked and there are no zerocoins, the text stays at Enabling...

WetOne · 2020-12-11T20:18:35Z

Updated to specify that the wallet needs to be unlocked to stake
when trying to stake with not zerocoins the text will notify the user that zerocoins are needed

seanPhill · 2020-12-12T23:05:46Z

Got some more successful screenshots. Tried while already unlocked and with sufficient zerocoins.

And now locked.

codeofalltrades

ACK 7c927b5

CaveSpectre11 · 2020-12-13T16:17:24Z

@WetOne one obscure use case that still has a problem. This came up due to a discussion with Hort regarding a problem he's had for a while. So we set out to see if this fixes his problem. First, the problem we found:
staking=0 in your veil.conf file. Start veil-qt. It doesn't properly recognize that staking=0 was set and it sits at "Enabling...", with 'staking_enabled: true and staking_active: false'.

After taking a look:

bool MnemonicWalletInit::Open() const
[...]
        if (gArgs.GetBoolArg("-exchangesandservicesmode", false))
            pwallet->SetStakingEnabled(false);

So a check on -staking needs to be put in that code as well to set staking disabled on startup when -staking=0. A quick test with 'exchangesandservicesmode' confirms it comes up "staking disabled", so that should be the right place.

(if you're feeling really randy and want to play with veild and confirm that it works there too... look in init.cpp and you'll find:

        //Start staking thread last
        if (!gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET) && gArgs.GetBoolArg("-staking", true) && !gArgs.GetBoolArg("-exchangesandservicesmode", false))
            threadGroupStaking.create_thread(&ThreadStakeMiner);

which probably would work with a simple "else set staking enabled false".

The initial problem we were looking at [not necessarily with your code, but with the current release]

staking=0
wallet locked
start veil-qt
unlock wallet
staking turned on automatically

I think we've determined your code handles that and doesn't turn on staking; but only if 'staking=0' is "fixed".

seanPhill · 2020-12-14T01:26:26Z

I like this implementation on the whole, but another potential annoyance is that if the wallet is locked and you click the slider where it says "Unlock wallet for staking" it then tells you to unlock the wallet, and when you click over on the padlock icon and type your password, unlocking it, then the slider activates staking. This might annoy some people, in that clicking "unlock wallet for staking" doesn't actually unlock the wallet, as you have to go to another control for that.

CaveSpectre11 · 2020-12-14T01:35:26Z

where it says "Unlock wallet for staking" it then tells you to unlock the wallet, and when you click over on the padlock icon and type your password, unlocking it, then the slider activates staking.

I'll have to play around with it when I get a chance. You're saying that it doesn't activate staking when you unlock for staking; only when you unlock the wallet as a whole? Yea, that would be an issue. It should start staking [assuming staking is enabled underneath] when it's unlocked for staking or when it's fully unlocked.

this might annoy some people, in that clicking "unlock wallet for staking" doesn't actually unlock the wallet

Well, that should be the expected behavior; unlock for staking.... turns on staking. unlock for staking shouldn't need a full unlock for spending to work.

seanPhill · 2020-12-14T02:59:24Z

To test, I locked it again with the padlock icon and then unlocked for staking from the other slider in the Settings page.
The bottom slider unlocked as expected. (Currently saying "You need some zerocoin" on this wallet, only because I need to mint another 1000 denom in order to mature the others I already minted all at once.)

WetOne · 2020-12-17T12:36:39Z

@CaveSpectre11, @codeofalltrades

Quick update: I modified line 75 of mnemonicwalletinit.cpp to be

if (gArgs.GetBoolArg("-exchangesandservicesmode", false) || gArgs.GetBoolArg("-staking",false))

and then ran "veil-qt -staking=0". The GUI starts with "Unlock wallet for staking". OK. After unlocking the wallet it sits at "Enabling..." and doesn't transition to enabled. Using the CLI shows that staking is enabled but staking doesn't go active. I'm going to investigate this further when I get the chance later today. I don't know what is keeping the enabled and the actual staking in two different states.

I did put instrumentation to verify that the staking flag when starting the GUI was being accepted.

Bottom line: either I put the one line change in the wrong spot (or the change is wrong, should probably default staking to true) or this is more than a one line change.

CaveSpectre11 · 2020-12-17T13:43:54Z

Bottom line: either I put the one line change in the wrong spot (or the change is wrong, should probably default staking to true) or this is more than a one line change.

The two flags are inverted logic. But that exchanges mode might be wrong? I would think it should be "if exchangesandservicesmode, true || staking false. But it could very well be that exchangesandservicesmode is "working" due to some code elsewhere; since it would imply that code would pass through and disable staking if exchangesandservices mode is not set. So maybe that code path isn't exercised.

Maybe throw some debug logging in and see if that code is called at all; and if not, find where exchangesandservices mode disables staking.

WetOne · 2020-12-17T15:39:42Z

I fixed the inverted logic and verified that SetStakingEnabled(false) being called in mnemonic wallet. Then ran the qt with -staking=0, unlocked my wallet (staking stayed disabled), and hit the slider. The text went to Enabling... but never got to staking enabled. through the CLI staking enabled is set to true but staking active doesn't go to true. I'm going to look through what is holding back the activity.

CaveSpectre11 · 2020-12-17T19:31:02Z

if (gArgs.GetBoolArg("-exchangesandservicesmode", false) || gArgs.GetBoolArg("-staking",false))

Right place, wrong interpretation of GetBoolArg(). After a bit of confusion... the parameter to GetBoolArg is not what you're checking against, it's what the 'default' is if it's not present.

so gArgs.GetBoolArg("-exchangesandservicesmode", false) returns "false" if exchangesandservicesmode is not set. Likewise gArgs.GetBoolArg("-staking", false) returns "false" if set to 0, or not set at all.

So the check should be

if (gArgs.GetBoolArg("-exchangesandservicesmode", false) // if exchanges and services mode is set
|| !gArgs.GetBoolArg("-staking",true)) // if staking is specified, and set to false (0)

WetOne · 2020-12-18T00:02:47Z

@CaveSpectre11, @codeofalltrades

Made the change in mnemonicalletinit.cpp to include the check for staking being disabled.

There is still an issue when launching with staking disabled, unlocking, and then trying to enable staking.

CaveSpectre11 · 2020-12-18T00:20:01Z

There is still an issue when launching with staking disabled, unlocking, and then trying to enable staking.

Just to help explain what we realized. @codeofalltrades; so the problem is that, like so many other places, args are checked in the middle of the code, rather than on startup. In this case what's happening rests right here:

//Start staking thread last
if (!gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET) && gArgs.GetBoolArg("-staking", true) && !gArgs.GetBoolArg("-exchangesandservicesmode", false))
    threadGroupStaking.create_thread(&ThreadStakeMiner);

If we come up with staking=0, the staking thread doesn't get started. So when we later enable staking in the wallet, fStakingEnabled gets turned on, yet the thread isn't there lying dormant waiting to be staked:

So on a separate PR; that will all be fixed. It's out of scope for this as starting with 'staking=0' has never been able to start staking without restarting the wallet.

That second PR will move the check for '-staking=0' to the init, but take it off the check to start the staking thread. That way the staking thread will start regardless if staking is enabled or not. That will allow the thread to lay dormant until staking is enabled. The dormancy is here:

if (!pwallet || !pwallet->IsStakingEnabled() || (pwallet->IsLocked() && !pwallet->IsUnlockedForStakingOnly())) {
    MilliSleep(5000);
    continue;
}

This will also allow for a cli command to be able to turn staking on and off as well, and no longer require the wallet to be restarted. You can set your desired default, and then enable or disable when you chose to.

codeofalltrades

ACK cad2120

CaveSpectre11

ACK cad2120

CaveSpectre11 · 2020-12-19T19:59:27Z

Bounty Paid: 6200 + 100 for using PR form

CaveSpectre11 assigned WetOne Dec 6, 2020

CaveSpectre11 requested review from CaveSpectre11 and codeofalltrades December 6, 2020 21:01

CaveSpectre11 reviewed Dec 6, 2020

View reviewed changes

WetOne force-pushed the stakingSlider branch from 73fe46f to 7556f9b Compare December 8, 2020 01:07

codeofalltrades requested changes Dec 11, 2020

View reviewed changes

codeofalltrades added Tag: Waiting For Developer and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 11, 2020

WetOne force-pushed the stakingSlider branch from 7556f9b to 7c927b5 Compare December 11, 2020 20:16

CaveSpectre11 requested a review from codeofalltrades December 11, 2020 20:51

codeofalltrades approved these changes Dec 13, 2020

View reviewed changes

CaveSpectre11 added this to the v1.1.1 milestone Dec 17, 2020

[GUI] Update Staking Slider to be reflective of status

cad2120

WetOne force-pushed the stakingSlider branch from 7c927b5 to cad2120 Compare December 17, 2020 23:59

CaveSpectre11 added Tag: Waiting For Code Review Waiting for code review from a core developer and removed Tag: Waiting For Developer labels Dec 18, 2020

CaveSpectre11 requested a review from codeofalltrades December 18, 2020 00:21

codeofalltrades approved these changes Dec 18, 2020

View reviewed changes

CaveSpectre11 approved these changes Dec 18, 2020

View reviewed changes

CaveSpectre11 added Code Review: Passed Tag: Waiting For QA A pull review is waiting for QA to test the pull request and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 18, 2020

CaveSpectre11 merged commit 83bd2bf into Veil-Project:master Dec 19, 2020

CaveSpectre11 added QA: Passed This has passed QA testing and can be merged to master Dev Status: Merged Issue is completely finished. and removed Tag: Waiting For QA A pull review is waiting for QA to test the pull request labels Dec 19, 2020

WetOne mentioned this pull request Apr 2, 2021

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GUI] Update Staking Slider to be reflective of status #880

[GUI] Update Staking Slider to be reflective of status #880

WetOne commented Dec 6, 2020

CaveSpectre11 Dec 6, 2020

WetOne Dec 6, 2020

CaveSpectre11 Dec 7, 2020

WetOne Dec 8, 2020

CaveSpectre11 Dec 8, 2020

WetOne Dec 8, 2020 •

edited

Loading

seanPhill commented Dec 10, 2020

codeofalltrades left a comment

codeofalltrades Dec 11, 2020

WetOne commented Dec 11, 2020

seanPhill commented Dec 12, 2020 •

edited

Loading

codeofalltrades left a comment

CaveSpectre11 commented Dec 13, 2020 •

edited

Loading

seanPhill commented Dec 14, 2020

CaveSpectre11 commented Dec 14, 2020

seanPhill commented Dec 14, 2020

WetOne commented Dec 17, 2020 •

edited

Loading

CaveSpectre11 commented Dec 17, 2020

WetOne commented Dec 17, 2020

CaveSpectre11 commented Dec 17, 2020

WetOne commented Dec 18, 2020

CaveSpectre11 commented Dec 18, 2020

codeofalltrades left a comment

CaveSpectre11 left a comment

CaveSpectre11 commented Dec 19, 2020

[GUI] Update Staking Slider to be reflective of status #880

[GUI] Update Staking Slider to be reflective of status #880

Conversation

WetOne commented Dec 6, 2020

Problem

Root Cause

Solution

Issues Addressed

Bounty Payment Address

CaveSpectre11 Dec 6, 2020

Choose a reason for hiding this comment

WetOne Dec 6, 2020

Choose a reason for hiding this comment

CaveSpectre11 Dec 7, 2020

Choose a reason for hiding this comment

WetOne Dec 8, 2020

Choose a reason for hiding this comment

CaveSpectre11 Dec 8, 2020

Choose a reason for hiding this comment

WetOne Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

seanPhill commented Dec 10, 2020

codeofalltrades left a comment

Choose a reason for hiding this comment

codeofalltrades Dec 11, 2020

Choose a reason for hiding this comment

WetOne commented Dec 11, 2020

seanPhill commented Dec 12, 2020 • edited Loading

codeofalltrades left a comment

Choose a reason for hiding this comment

CaveSpectre11 commented Dec 13, 2020 • edited Loading

seanPhill commented Dec 14, 2020

CaveSpectre11 commented Dec 14, 2020

seanPhill commented Dec 14, 2020

WetOne commented Dec 17, 2020 • edited Loading

CaveSpectre11 commented Dec 17, 2020

WetOne commented Dec 17, 2020

CaveSpectre11 commented Dec 17, 2020

WetOne commented Dec 18, 2020

CaveSpectre11 commented Dec 18, 2020

codeofalltrades left a comment

Choose a reason for hiding this comment

CaveSpectre11 left a comment

Choose a reason for hiding this comment

CaveSpectre11 commented Dec 19, 2020

WetOne Dec 8, 2020 •

edited

Loading

seanPhill commented Dec 12, 2020 •

edited

Loading

CaveSpectre11 commented Dec 13, 2020 •

edited

Loading

WetOne commented Dec 17, 2020 •

edited

Loading