Improve miner status messages for ineligible staking balances #1447
Conversation
Also fixes gridcoin-community#1446 by clearing the miner status variables for GUI integration.
|
utACK. |
| //! | ||
| //! \return Always \false - suitable for returning from the call directly. | ||
| //! | ||
| bool BreakForNoCoins(CMinerStatus& status, const char* message) |
There was a problem hiding this comment.
This function looks like suitable for inclusion into CMinerStatus class.
There was a problem hiding this comment.
We could, but it's an implementation detail of the miner loop. I don't think we want code outside this file to call that function. It just makes the early return conditions in the miner loop more readable/maintainable.
There was a problem hiding this comment.
Let's leave it this way for now. We can fancy it up in Elizabeth if we want.
| @@ -417,16 +442,18 @@ bool CreateCoinStake( CBlock &blocknew, CKey &key, | |||
| int64_t nValueIn = 0; | |||
| //Request all the coins here, check reserve later | |||
|
|
|||
| if ( BalanceToStake<=0 | |||
| || !wallet.SelectCoinsForStaking(BalanceToStake*2, txnew.nTime, CoinsToStake, nValueIn) ) | |||
There was a problem hiding this comment.
How does this actually solve the issue? In the old code, SelectCoinsForStaking would return false, causing ReasonNotStaking to be set and false returned from CreateCoinStake. The BreakForNoCoins does essentially the same. Or is the fix only in calling status.Clear() ?
There was a problem hiding this comment.
The fix for the linked issue is just to call Clear() to set the status variables back to zero because the UI checks those numbers for the staking indicator. I noticed that we say "No coins" for three different conditions, so I separated them to provide more informative messages (I imagine that someone might be confused if the wallet says "No coins" even though they do have coins, but they're just on cooldown). The BreakForNoCoins() helper just does the same thing, but it wraps up the shared code so we can pass different messages without duplicating the logic.
There was a problem hiding this comment.
Thanks for clarifying that the Clear bit was the fix. I could not read it from the PR description (sorry). And I like how you separated the error cases!
|
I regenerated the Qt translation units because I added a couple of user-facing messages, but I'm not sure what the process is here for keeping those up to date, so I didn't commit them. Would you like for me to check those in? |
|
Tomas are you ok to go with this? |
|
I don't know on those. Let's do that in a separate PR from this one. Merging this one. |
Added: - Replace NeuralNetwork with portable C++ scraper #1387 (@jamescowens, @tomasbrod, @Cycy, @TheCharlatan, @denravonska). - Allow compile flags to be used for depends #1423 (@G-UK). - Add stake splitting and side staking info to getmininginfo #1424 (@jamescowens). - Add freedesktop.org desktop file and icon set #1438 (@a123b). Changed: - Disable Qt for windows Travis builds #1276 (@TheCharlatan). - Replace use of AppCache PROJECT section with strongly-typed structures #1415 (@cyrossignol). - Change dumpwallet to use appropriate data directory #1416 (@jamescowens). - Optimize ExtractXML() calls by avoiding unnecessary string copies #1419 (@cyrossignol). - Change signature of IsLockTimeWithinMinutes #1422 (@jamescowens). - Restore old poll output for getmininginfo RPC #1437 (@a123b). - Prevent segfault when using rpc savescraperfilemanifest #1439 (@jamescowens). - Improve miner status messages for ineligible staking balances #1447 (@cyrossignol). - Enhance scraper log archiving #1449 (@jamescowens). Fixed: - Re-enable full GUI 32-bit Windows builds - part of #1387 (@jamescowens). - Re-activate Windows Installer #1409 (@TheCharlatan). - Fix Depends and Travis build issues for ARM #1417 (@jamescowens). - Fix syncupdate icons #1421 (@jamescowens). - Fix potential BOINC crash when reading projects #1426 (@cyrossignol). - Fix freeze when unlocking wallet #1428 (@denravonska). - Fix RPC after high priority alert #1432 (@denravonska). - Fix missing poll in GUI when most recent poll expired #1455 (@cyrossignol). Removed: - Remove old, rudimentary side staking implementation #1381 (@denravonska). - Remove auto unlock #1402 (@denravonska). - Remove superblock forwarding #1430 (@denravonska).
Also fixes #1446 by clearing the miner status variables for GUI integration. I think we really just needed to call
MinerStatus.Clear()before returning, but maybe the more precise messages help to reduce confusion.