Automatic stake UTXO splitting to optimize staking efficiency #1244

jamescowens · 2018-08-03T21:43:08Z

This PR implements three conf file parameters:

minstakesplitvalue
enablestakesplit
stakingefficiency

enablestakesplit=1 will enable the automatic splitting of UTXO's in the coinstake transaction (stake outputs). Zero is the default (disabled).

stakingefficiency=xx is an integer that specifies the desired staking efficiency. This is constrained by the code to be between 75% and 98%, in case someone puts some nonsense value in there.

minstakesplitvalue=xxx is an integer that specifies the minimum UTXO size desired post split to provide a secondary control on UTXO size. If difficulty drops and a high efficiency is specified, the efficiency alone may split UTXO's into amounts smaller than the user desires. This will prevent that from occurring. If a user specifies less than 800 GRC, then the code uses 800 GRC. Note that the stake splitter uses a 160 block averaging interval for calculating the difficulty to smooth out the difficulty swings.

The code is implemented as a modification to the UTXO push at the end of the CreateCoinStake, and the addition of the Gridcoin reward to the UTXO(s) at the end of CreateGridcoinReward. Note that the code can handle an n-way UTXO split in the case of giant UTXO's that need to be split more than 2-way, but there is a current limit placed of 2-way because of the limitation of the number of Coinstake outputs in AcceptBlock. This may be changed in the future.

This addresses issue #1240.

This PR is already staking successfully on testnet with my testnet Windows node.

jamescowens · 2018-08-03T23:31:27Z

Here is the stake output...

08/03/18 22:46:26 CreateCoinStake: Found Kernel;
08/03/18 22:46:26 CreateCoinStake: nMinUTXOPostSplitValue = 800.000000
08/03/18 22:46:26 CreateCoinStake: dEfficiency = 0.950000
08/03/18 22:46:26 CreateCoinStake: nDesiredStakeUTXOValue = 800.000000
08/03/18 22:46:26 CreateCoinStake: nUTXOCount = 2
08/03/18 22:46:26 CreateCoinStake: nActualStakeUTXOValue = 1027.658856
08/03/18 22:46:26 CreateCoinStake: create stake UTXO 1 value 1027.658856
08/03/18 22:46:26 CreateCoinStake: create stake UTXO 2 value 1027.658856
08/03/18 22:46:26 CreateCoinStake: added kernel type=1 credit=2055.317712
08/03/18 22:46:26 StakeMiner: created rest of the block
08/03/18 22:46:26 CreateGridcoinReward: for bc0621a4ac4610ffa400a0d298c02e23 mint 35.986201 {RSAWeight 2920.000000} Research 35.986296, Interest 0.000000
08/03/18 22:46:26 CreateGridcoinReward: nUTXOCount = 2
08/03/18 22:46:26 CreateGridcoinReward: nActualStakeUTXOReward = 17.993101
08/03/18 22:46:26 CreateGridcoinReward: added reward to stake UTXO 1 value 17.993101
08/03/18 22:46:26 CreateGridcoinReward: added reward to stake UTXO 2 value 17.993101
08/03/18 22:46:26 StakeMiner: added gridcoin reward to coinstake
08/03/18 22:46:27 AddNeuralContractOrVote: not Needed
08/03/18 22:46:27 StakeMiner: signed boinchash, coinstake, wholeblock
08/03/18 22:46:27 {SBC} new best {4a5d4c796bbfcd00672992f706a62b0490b798434cd87fa7ea6813b130678caa 631293} ;
08/03/18 22:46:27 {PB}: ACC;
08/03/18 22:46:27 StakeMiner: block processed

tomasbrod

split into multiple functions
one that calculates how many outputs to split to
second that does the actual splitting
called from the main miner function

jamescowens · 2018-08-04T11:44:05Z

That makes it a lot harder Tomas and what does it get you? Those functions would be captive and never used by anything but the miner.

jamescowens · 2018-08-04T11:56:32Z

It will make it prettier... :). Before I go to the trouble to split off into separate functions, do you buy into my implementation concept?

jamescowens · 2018-08-04T20:03:32Z

Tomas. Looking at this more carefully. I agree with putting the number of UTXO's calculation into a helper funtion. It seems wrong to me to actually create another function to "split" the UTXO's, because they are not really being split. The calculation determines the number to "create" and then push is done to create them. This properly belongs in CreateCoinStake I think... Further, we add the reward to the same UTXO's later in CreateGridcoinReward.

jamescowens · 2018-08-04T22:07:44Z

Implemented unsigned int GetNumberofStakeOutputs(int64_t nValue). Also changed upper efficiency limit to 98%. 99% is too aggressive and unnecessary.

tomasbrod · 2018-08-05T18:21:54Z

This properly belongs in CreateCoinStake I think... Further, we add the reward to the same UTXO's later in CreateGridcoinReward.

This is the reason why i want it split. CreateGridcoinReward is called after CreateCoinStake and would only add reward to the 1nd output.

You are duplicating (not fully) the splitting code both in CreateCoinStake and CreateGridcoinReward. If the splitting was done after all reward manipulation, only once.

jamescowens · 2018-08-05T18:51:19Z

It adds rewards to both outputs because I put a loop in there. I agree it duplicates the loop. The problem is what it CreateCoinStake doing anymore if it doesn't actually create the UTXO's. I will discuss with you on Slack/Rocket.

jamescowens · 2018-08-06T17:35:59Z

Ok @tomasbrod. I created void SplitCoinStakeOutput(CBlock &blocknew). Please take a look again. I think this does it...

tomasbrod · 2018-08-06T21:25:13Z

src/miner.cpp

@@ -600,7 +600,6 @@ bool CreateCoinStake( CBlock &blocknew, CKey &key,

            txnew.vout.push_back(CTxOut(0, CScript())); // First Must be empty
            txnew.vout.push_back(CTxOut(nCredit, scriptPubKeyOut));
-            //txnew.vout.push_back(CTxOut(0, scriptPubKeyOut));


You left that code in there commented out, probably from the original write of V8. I figured it should go away.

tomasbrod · 2018-08-06T21:25:55Z

src/miner.cpp

+void SplitCoinStakeOutput(CBlock &blocknew)
+{
+    // When this function is called, CreateCoinStake and CreateGridcoinReward have already been called
+    // and there will be a single coinstake output (besides the empty one) that has the combined stake + research


great candidate for assert()ion

What are you suggesting to assert? There is no condition in any of that code that is expected to fail. It is a no-op if the number of UTXO's is 1, and GetNumberofStakeOutputs is clamped. If CreateCoinStake and CreateGridcoinReward have succeeded (because we have arrived at the call to SplitCoinStakeOutput), then CBlock StakeBlock already exists and is in the proper form.

assert(blockNew.vtx.size() == 2)

@tomasbrod I think you actually meant assert(blockNew.vtx[1].vout.size()==2)...

tomasbrod · 2018-08-06T21:33:01Z

src/kernel.h

@@ -69,4 +69,6 @@ int64_t CalculateStakeWeightV8(
    const CTransaction &CoinTx, unsigned CoinTxN,
    const MiningCPID &BoincData);

+unsigned int GetNumberofStakeOutputs(int64_t nValue);


Definition in miner but declaration in kernel.h? Make it consistent, please.

You don't even have a CreateCoinStake declaration in a header file @tomasbrod ... And for that matter, I didn't make a declaration for SplitCoinStakeOutput... so I need to fix that. I don't even know why the compiler is not barking about it, or about your original CreateCoinStake function that has no declaration. I will move the GetNumberOfStakeOutputs and also add SplitCoinStakeOutput to miner.h.

I do not have declaration, because the function is not used nowhere, but the file it is defined in. Yes, correctly is should have a declaration or be made static. I am commenting on the fact that you put the declaration to kernel.cpp

you do not need declaration if the function is used below it's definition (definition is a declaration)

It's actually good to leave it out so the user API becomes more clear. It does require to extern declare it in the test suite, so don't make it static. You could argue that you should test through the public API. While true, in this case, I suspect the actual unit test will become too hard to both read and write.

Yep. Really hard to unit test this. I moved the declarations to miner.h. Are you guys ok with that?

tomasbrod · 2018-08-06T21:34:00Z

src/miner.cpp

+    // Pull Minimum Post Stake UTXO Split Value from config or command line parameter.
+    // Do not allow it to be specified below 800 GRC.
+    nMinUTXOPostSplitValue = GetArg("-minimumstakeutxovalueforsplit", 800) * COIN;
+    nMinUTXOPostSplitValue = (nMinUTXOPostSplitValue < 800 * COIN) ? 800 * COIN : nMinUTXOPostSplitValue;


use std::max

Everything doesn't have to be done with methods @tomasbrod. I will do it, but the request to change is silly. You are lucky. std::max is defined in C++11. Its use here requires a cast of the 800 to int64_t because std::max does not like to compare with different integer types.

tomasbrod · 2018-08-06T21:36:04Z

src/miner.cpp

+    if (dEfficiency > 0.98)
+        dEfficiency = 0.98;
+    else if (dEfficiency < 0.75)
+        dEfficiency = 0.75;


use std::clamp( val, low, high )
include <algorithm> if not already declared

you can return fast if nValue < nMinUTXOPostSplitValue

You can't use std:;clamp. It is C++17 and is not available in many compilers.

I agree with the fast return and implemented it.

I hate that it took them this long to implement a clamp instead of the ugly std::max(lower, std::min(n, upper)); construct.

That construct is uglier to me than doing the clamp with straight if statements... I agree with you!

I forgot to change line 715 to a std::max. I will do it in the morning.

tomasbrod · 2018-08-06T21:38:18Z

src/miner.cpp

+    else if (dEfficiency < 0.75)
+        dEfficiency = 0.75;
+
+    LogPrintf("GetNumberofStakeOutputs: dEfficiency = %f", dEfficiency);


Please remove prints like this, or at least condition under fDebug2/fDebug

Then you should remove the original prints from CreateCoinStake, which are similar.

If we are going to be consistent with the LogPrintf's in CreateCoinStake (at 605 for example), then the ones at 663 and 671 should remain non-conditional. I have conditioned all others on fDebug2.

tomasbrod · 2018-08-06T21:41:29Z

src/miner.cpp

+    // Set Desired UTXO size post stake based on efficiency and difficulty, but do not allow to go below MinUTXOPostSplitValue.
+    // Note that we use GetAverageDifficulty over a 4 hour (160 block period) rather than StakeKernelDiff, because the block
+    // to block difficulty has too much scatter.
+    nDesiredStakeUTXOValue = 9942.2056 * GetAverageDifficulty(160) * (3.0 / 2.0) * (1 / dEfficiency  - 1) * COIN;


please link to explanation of the constants (file under doc/ or steemit)
move 9942.2056 constant somewhere up

That is a good point. I can do that, since I did all of these calculations... the problem is that the final blue paper is not done yet, only the initial draft. I will put a link to the initial draft paper.

tomasbrod · 2018-08-06T21:54:58Z

I should have done the requested changes instead of talking about them.

Anyway. Let's talk about the config options.
minimumstakeutxovalueforsplit is too long,
calling the other efficiency in config can evoke need for 100%, idk which name would be non biased
The enable switch could be merged with min output tunable:
0-> disabled, non zero -> enabled, >800 -> increases min output
@denravonska what do you think about the option names?

jamescowens · 2018-08-06T22:15:40Z

I don't like combining the tunables. It makes it more cryptic. We have ENOUGH cryptic code in this codebase. Option names should be descriptive of what they do, just like variable names/functions, and they should not be overloaded unless absolutely necessary. I will be happy to shorten them and will take suggestions as long as they are descriptive.

jamescowens · 2018-08-06T22:38:35Z

I should have done the requested changes instead of talking about them.

@tomasbrod That is insulting to me. Sorry to have wasted your time.

denravonska · 2018-08-07T04:07:48Z

src/miner.cpp

+
+    // Pull Minimum Post Stake UTXO Split Value from config or command line parameter.
+    // Default to 800 and do not allow it to be specified below 800 GRC.
+    nMinUTXOPostSplitValue = GetArg("-minimumstakeutxovalueforsplit", (int64_t)800) * COIN;


There's a small bug here where the lower cap is 800 Halfords, not coins, if min stake value is set to 0. You could also extract the constant to avoid someone changing one but not the other.

namespace { const uint64_t MIN_STAKE_SPLIT_VALUE_GRC = 800; } // ... const int64_t = std::max( GetArg("-minimumstakeutxovalueforsplit", MIN_STAKE_SPLIT_VALUE_GRC), MIN_STAKE_SPLIT_VALUE_GRC) * COIN;

Edit: MIN_STAKE_SPLIT_VALUE_GRC was a poor name as it's not in coins. Ignore my std::max construct as it made things less readable :D But the bug remains.

u are right. Good catch! I could fix it equivalently by changing line 692 to nMinUTXOPostSplitValue = max(nMinUTXOPostSplitValue, (int64_t)800 * COIN);

which do you think is better?

Do the small change and let's assume that the next author is a sensible adult.

I think we should define a system constant for the 800. Might as well do it now. And the 800 is effectively in GRC so I think your constant name is good! Now... where to put it....

I prefer your approach @denravonska. I am putting the constant in miner.h as

// Note the below constant controls the minimum value allowed for post
// split UTXO size. It is int64_t but in GRC so that it matches the entry in the config file.
// It will be converted to Halfords in GetNumberOfStakeOutputs by multiplying by COIN.
static const int64_t MIN_STAKE_SPLIT_VALUE_GRC = 800;

I have to do it this way to avoid a template error, because the GetArg is int64_t.

tomasbrod · 2018-08-07T07:59:46Z

I should have done the requested changes instead of talking about them.

@tomasbrod That is insulting to me. Sorry to have wasted your time.

I tried to applologise for wasting your time by ordering you around to change this and that. Sorry
i need to improve my language skills

jamescowens · 2018-08-07T11:35:09Z

Then I owe you an apology. I mistook your comment. Just know that I am the type person that likes to finish what I started...

jamescowens · 2018-08-07T15:22:49Z

Ok. I think everything is in there. The only thing left is to shorten the config params if you both don't like the longer names... :)

denravonska · 2018-08-07T15:23:58Z

I would prefer a shorter and more to the point name but I cannot come up with anything better.

jamescowens · 2018-08-07T15:31:52Z

Me either...

jamescowens · 2018-08-07T15:33:21Z

I think it is ready for general testnet testing... I have been running in testnet already in the previous guises, and it appears to work nicely.

iFoggz

utACK agree for testing if everyone comes to consensus on moving forward

jamescowens · 2018-08-08T02:33:51Z

Ok! @barton2526 gave me some inspiration... I have changed the config option names to...

minstakesplitvalue
enablestakesplit
stakingefficiency

Much better!

I think this is ready for testing now!

This modifies the miner to split stake outputs if necessary to optimize UTXO size.

denravonska · 2018-09-25T15:14:53Z

Closing in favor of #1265

Side staking (extends stake splitting PR #1244)

tomasbrod · 2018-09-25T21:01:31Z

merging this as well (no-op code-wise) would grant James bit of stats.

jamescowens · 2018-09-25T21:15:35Z

I didn't even think about that! :)

jamescowens force-pushed the StakeUTXOSplit branch 4 times, most recently from aa26c36 to 4e0134c Compare August 3, 2018 22:04

tomasbrod suggested changes Aug 4, 2018

View reviewed changes

jamescowens force-pushed the StakeUTXOSplit branch 2 times, most recently from 5517a32 to 36aee75 Compare August 4, 2018 22:05

jamescowens force-pushed the StakeUTXOSplit branch from 36aee75 to 19fe491 Compare August 6, 2018 17:30

tomasbrod suggested changes Aug 6, 2018

View reviewed changes

jamescowens force-pushed the StakeUTXOSplit branch from 19fe491 to d033f5d Compare August 7, 2018 00:32

denravonska added the enhancement label Aug 7, 2018

denravonska reviewed Aug 7, 2018

View reviewed changes

jamescowens force-pushed the StakeUTXOSplit branch from d033f5d to c4233b6 Compare August 7, 2018 15:20

denravonska added this to the Camilla milestone Aug 7, 2018

iFoggz reviewed Aug 7, 2018

View reviewed changes

jamescowens force-pushed the StakeUTXOSplit branch from c4233b6 to 4768154 Compare August 8, 2018 02:31

Stake UTXO split

6e85632

This modifies the miner to split stake outputs if necessary to optimize UTXO size.

jamescowens force-pushed the StakeUTXOSplit branch from 4768154 to 6e85632 Compare August 9, 2018 00:39

tomasbrod mentioned this pull request Aug 9, 2018

Allow more than two outputs on CoinStake transaction #1256

Closed

This was referenced Aug 12, 2018

Raise coinstake output count limit to 8 #1261

Merged

Side staking (extends stake splitting PR #1244) #1265

Merged

iFoggz mentioned this pull request Sep 10, 2018

Removal of 0123 0124 & implementation of GenerateType/MinedType #1295

Closed

denravonska closed this Sep 25, 2018

denravonska added a commit that referenced this pull request Sep 25, 2018

Merge pull request #1265 from jamescowens/SideStaking

b7611c8

Side staking (extends stake splitting PR #1244)

jring-o mentioned this pull request Sep 25, 2019

The Development Roadmap of the Gridcoin Network (September 2019) #1536

Open

jamescowens deleted the StakeUTXOSplit branch October 23, 2019 00:04

Automatic stake UTXO splitting to optimize staking efficiency #1244

Automatic stake UTXO splitting to optimize staking efficiency #1244

Uh oh!

Conversation

jamescowens commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescowens commented Aug 3, 2018

Uh oh!

tomasbrod left a comment

Choose a reason for hiding this comment

Uh oh!

jamescowens commented Aug 4, 2018

Uh oh!

jamescowens commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescowens commented Aug 4, 2018

Uh oh!

jamescowens commented Aug 4, 2018

Uh oh!

tomasbrod commented Aug 5, 2018

Uh oh!

jamescowens commented Aug 5, 2018

Uh oh!

jamescowens commented Aug 6, 2018

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!

jamescowens Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jamescowens Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

denravonska Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jamescowens Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasbrod Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

jamescowens commented Aug 3, 2018 •

edited

Loading

jamescowens commented Aug 4, 2018 •

edited

Loading

jamescowens Aug 6, 2018 •

edited

Loading

jamescowens Aug 6, 2018 •

edited

Loading

denravonska Aug 7, 2018 •

edited

Loading

jamescowens Aug 6, 2018 •

edited

Loading

tomasbrod Aug 6, 2018 •

edited

Loading

jamescowens commented Aug 6, 2018 •

edited

Loading

denravonska Aug 7, 2018 •

edited

Loading

jamescowens Aug 7, 2018 •

edited

Loading