Move some functionality from miner to SelectCoinsForStaking + Respect the coin reserve setting + Randomize UTXO order #1525

iFoggz · 2019-09-09T21:56:49Z

Currently there is not guaranteed way to respect a reserve coin setting. After some discussion with jim and jims formula we have made a way that the reserve coin setting will be respected in all cases.

Added:

Use a formula of (S - R) >= value for determining if a utxo can stake based on the base and the reserve amount set. This is (Spendable - Reserve) >= utxo value. This also allows all utxos to still stake when the conditions are met.
Added bool to SelectCoinsForStaking to determine if the miner is requesting the data
SelectCoinsForStaking can send back a false in return now under various conditions.

Removed:

No longer use nValueIn as it is unused
No longer push a target value to SelectCoinsForStaking. The number sent in always *2.

Changed:

No longer check for coin age in miner as this is already done in AvailableCoinsForStaking and miner was a second unneeded check.
Move check for no coins to SelectCoinsForStaking as a more appropriate location
Move check to see if a reserve balance consumed more then the amount of coins in wallet to SelectCoinsForStaking
Refactor the pairing of coin and value in SelectCoinsForStaking to remove unnecessary extra variable in set pair for setCoinsRet
Add logging to SelectCoinsForStaking when debug2 enabled and miner calling the function

TODO:

In Future change the coin utxos sent to miner to be in random order since the wallet in mapped (sorted) in cases where a utxo has a high txid say starting with 'f' it can reduce the chance of that one staking since so many others that could have similar weights but lower txid.
Modify GetStakeWeight to remove the age check of coins since AvailableCoinsForStaking already does this

I'm sure @denravonska and @jamescowens will request changes. i've tested this on testnet for 3 days under various conditions without issues.

…eserve. Correct logging message to what the data actually shows. updated

jamescowens · 2019-09-10T14:55:10Z

Very happy to see these optimizations.

This by itself is leisure, because the selection of UTXO's to stake is a local node issue as long as they are validly spendable. I think it, along with the succeeding randomizer should go in Fern.

jamescowens

See comments for consideration

src/miner.cpp

src/wallet.cpp

cyrossignol

Looks good so far! To reduce confusion, we may also want to update the help text for the -reservebalance option, reservebalance RPC, and "Reserve" field in the UI under Settings -> Options... to make it clear for someone with only a few, large UTXOs that a reserve setting that appears reasonable may actually disqualify most of their coins from staking. For example, if I buy 50k GRC three times (150k) and set a reserve of 50%, I can only stake 50k each time. It makes sense, but it might confuse unfamiliar users.

src/wallet.cpp

src/miner.cpp

iFoggz · 2019-09-16T07:57:05Z

i believe reserve balance help should be updated. this allows all utxos to stake still unless there is only enough balance left to cover reserve.

jamescowens

Agreed.

…ctCoinsForStaking to have miner report the error to MinerStatus

… instead of being ordered with set by txid

src/wallet.cpp

jamescowens · 2019-09-16T23:40:02Z

I think it is good. Running this on two of my testnet nodes along with @cyrossignol's SuperblockValidator class.

jamescowens · 2019-09-17T02:18:59Z

Tests well for me on testnet. Changed reserve setting in the UI on the fly and the coinweight and tooltip changed correctly. When reserve set very high, coinweight went down to the correct total of the few eligible UTXO's. When reserve set >= balance, tootip correctly shows not staking, entire balance reserved. CPU use looks low and my testnet Windows wallet has several hundred UTXO's.

This reverts commit 516d986.

…false or vector coins is empty and returns false we return 0; even thou nWeight would technically already be zero

cyrossignol

Looks good!

cyrossignol · 2019-09-17T21:01:35Z

src/wallet.cpp

        return false;

-    if (setCoins.empty())
+    if (vCoins.empty())


Minor stuff: we can remove this check now that SelectCoinsForStaking() returns false when it finds no UTXOs.

yes we can its only a few extra cycles anyway

perhaps i should check if either are empty and send back the appropriate reason?

if (vCoinsRet.empty()) { if (fMiner) sError = vCoins.empty() ? _("No mature coins") : _("No utxos available due to reserve balance"); return false; }

lmk i think that would be better instead of no mature coins sending back no utxos available due to reserve

This vCoins in GetStakeWeight() is different from the one in SelectCoinsForStaking(). I don't think you need to do that because you're already returning false and setting the reason for vCoins.empty() above this:

Gridcoin-Research/src/wallet.cpp

Lines 1531 to 1537 in 5f24441

if (vCoins.empty())

{

if (fMiner)

sError = _("No mature coins");

return false;

}

That will happen before the function filters the UTXOs, so we should get the right message for both cases as you already wrote it.

oh ya so i'll put it back to way it was and remove that check in GetStakeWeight then, correct

Yep, that should do it. I think this is finished 🙂

@caraka

Added - Add testnet desktop launcher action for Linux #1516 (@caraka) - Shuffle vSideStakeAlloc if necessary to support sidestaking to more than 6 destinations #1532 (@jamescowens) - New Superblock format preparations for Fern #1526, #1542 (@jamescowens, @cyrossignol) - Multisigtools - Consolidate multisig unspent #1529 (@iFoggz) - Scanforunspent #1547 (@iFoggz) - consolidatemsunspent and scanforunspent bug fix #1561 (@iFoggz) - New banning misbehavior handling and Peers Tab on Debug Console #1537 (@jamescowens) - Reimplement getunconfirmedbalance rpc #1548 (@jamescowens) - Add CLI switch to display binary version #1553 (@cyrossignol) Changed - Select smallest coins for contracts #1519 (@iFoggz) - Move some functionality from miner to SelectCoinsForStaking + Respect the coin reserve setting + Randomize UTXO order #1525 (@iFoggz) - For voting - if url does not contain http then add it #1531 (@iFoggz) - Backport newer serialization facilities from Bitcoin #1535 (@cyrossignol) - Refactor ThreadSocketHandler2() Inactivity checks #1538 (@iFoggz) - Update outdated checkpoints #1539 (@barton2526) - Change needed to build Gridcoin for OSX using homebrew #1540 (@Git-Jiro) - Optimize scraper traffic for expiring manifests #1542 (@jamescowens) - Move legacy neural vote warnings to debug log level #1560 (@cyrossignol) - Change banlist save interval to 5 minutes #1564 (@jamescowens) - Change default rpcconsole.ui window size to better support new Peers tab #1566 (@jamescowens) Removed - Remove deprecated RSA weight and legacy kernel #1507 (@cyrossignol) Fixed - Clean up compiler warnings #1521 (@cyrossignol) - Handle missing external CPID in client_state.xml #1530 (@cyrossignol) - Support boost 1.70+ #1533 (@iFoggz) - Fix diagnostics failed to make connection to NTP server #1545 (@Git-Jiro) - Install manpages in correct system location #1546 (@Git-Jiro) - Fix ability to show help and version without a config file #1553 (@cyrossignol) - Refactor QT UI variable names to be more consistent, Fix Difficulty default #1563 (@barton2526) - Fix two regressions in previous UI refactor #1565 (@barton2526) - Fix "Owed" amount in output of "magnitude" RPC method #1569 (@cyrossignol)

Move more functionality to SelectCoinsForStaking and respect wallet r…

2f5d26e

…eserve. Correct logging message to what the data actually shows. updated

jamescowens requested review from cyrossignol, denravonska and jamescowens September 10, 2019 14:50

jamescowens assigned iFoggz Sep 10, 2019

jamescowens added this to the Fern milestone Sep 10, 2019

jamescowens reviewed Sep 15, 2019

View reviewed changes

src/miner.cpp Show resolved Hide resolved

src/wallet.cpp Outdated Show resolved Hide resolved

jamescowens removed the request for review from denravonska September 15, 2019 17:09

cyrossignol reviewed Sep 16, 2019

View reviewed changes

src/wallet.cpp Outdated Show resolved Hide resolved

src/miner.cpp Show resolved Hide resolved

jamescowens reviewed Sep 16, 2019

View reviewed changes

iFoggz added 3 commits September 16, 2019 11:22

Change BreakForNoCoins To ReturnMinerError. Pass error back from Sele…

0e10bd4

…ctCoinsForStaking to have miner report the error to MinerStatus

Randomize utxos sent to miner to keep PoS completely roll of the dice…

f4cc160

… instead of being ordered with set by txid

Update reserve balance notes

516d986

jamescowens reviewed Sep 16, 2019

View reviewed changes

src/wallet.cpp Outdated Show resolved Hide resolved

Change to another method of shuffle to avoid future deprecation

8ed9ae9

jamescowens approved these changes Sep 16, 2019

View reviewed changes

jamescowens changed the title ~~Move some functionality from miner to SelectCoinsForStaking + Respect the coin reserve setting~~ Move some functionality from miner to SelectCoinsForStaking + Respect the coin reserve setting + Randomize UTXO order Sep 16, 2019

jamescowens requested a review from cyrossignol September 17, 2019 02:19

iFoggz added 3 commits September 16, 2019 20:15

Revert "Update reserve balance notes"

10a94c2

This reverts commit 516d986.

change rpc/options form reserve balance information

b2dd29b

Move nWeight = 0 above Select Coins so in event select coins returns …

f7b270c

…false or vector coins is empty and returns false we return 0; even thou nWeight would technically already be zero

cyrossignol approved these changes Sep 17, 2019

View reviewed changes

this should be it

a0a58fb

iFoggz force-pushed the ChangeMinerBehaviour branch from 5f24441 to a0a58fb Compare September 17, 2019 21:45

jamescowens merged commit 89938c7 into gridcoin-community:development Sep 18, 2019

	if (vCoins.empty())
	{
	if (fMiner)
	sError = _("No mature coins");

	return false;
	}

Move some functionality from miner to SelectCoinsForStaking + Respect the coin reserve setting + Randomize UTXO order #1525

Move some functionality from miner to SelectCoinsForStaking + Respect the coin reserve setting + Randomize UTXO order #1525

Uh oh!

Conversation

iFoggz commented Sep 9, 2019

Uh oh!

jamescowens commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescowens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cyrossignol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iFoggz commented Sep 16, 2019

Uh oh!

jamescowens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamescowens commented Sep 16, 2019

Uh oh!

jamescowens commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyrossignol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamescowens commented Sep 10, 2019 •

edited

Loading

jamescowens commented Sep 17, 2019 •

edited

Loading