Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Sep 25, 2018

Conversation

@jamescowens
Copy link
Member

jamescowens commented Aug 15, 2018

This PR extends the work I have done in PR #1244 and implements the ability to "side stake" or distribute the CBR and research rewards to valid addresses. These addresses may be in the same wallet or any other valid Gridcoin address. The potential uses for sidestaking range from a person insuring that all reward go into the same address (and key) for easier backup and recovery to someone donating CBR and rewards to a charity, or sending it to their grandma! :). This functionality is very similar to Pinkcoin.

This addresses #1240, and along with #1260, addresses #1173.

The PR adds the following additional conf file parameters on top of 1244...

enablesidestaking=0|1
sidestake=address,allocation_percentage

You can specify multiple sidetake entries, just like addnode or connect. Note that the total number of ouputs for the coinstake will be limited to 8 in V10, and vout[0] must be empty, so that gives 7 usable outputs. One must always be reserved for the actual coinstake output (return), so that leaves up to 6 usuable outputs for rewards distribution.

Note that the total of all of the percentages can add up to less than 100%, in which cases the leftover reward will be returned back on the coinstake(s).

The SplitCoinStakeOutput function in miner.cpp has good comments on how it all actually works, but I am including here for easy reference...

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 reward. This function does the following...

  1. Perform reward payment to specified addresses ("sidestaking") in the following manner...
    a. Check if both flags false and if so return with no action.
    b. Limit number of outputs based on bv. 3 for <=9 and 8 for >= 10.
    c. Pull the nValue from the original output and store locally. (nReward was passed in.)
    d. Pop the existing outputs.
    e. Validate each address provided for redirection in turn. If valid, create an output of the reward * the specified percentage at the validated address. Keep a running total of the reward redirected. If the percentages add up to less than 100%, then the running total will be less than the original specified reward after pushing up to two reward outputs. Check before each allocation that 100% will not be exceeded. Also check if the distribution is less than 1 CENT, and if so skip it. (This will cause the small distribution to flow to the coinstake instead.) If there is a residual reward left, then add (nReward - running total) back to the base coinstake. This also works beautifully if the redirection is disabled, because then no reward outputs will be created, the accumulated total of redirected rewards will be zero, and therefore the subtraction will put the entire rewards back on the base coinstake.
  2. Perform stake output splitting of remaining value.
    a. With the remainder value left which is now the original coinstake minus (rewards outputs pushed - up to two), call GGetNumberOfStakeOutputs(int64_t nValue, unsigned int nOutputsAlreadyPushed) to determine how many pieces to split into, limiting to 8 - OutputsAlreadyPushed.
    b. SplitCoinStakeOutput will then push the remaining value into the determined number of outputs of equal size using the original coinstake input address.
    c. The outputs will be in the wrong order, so add the empty vout to the end and then reverse vout.begin() to vout.end() to make sure the empty vout is in [0] position and a coinstake vout is in the [1] position. This is required by block validation.

Note that @tomasbrod has suggested that I might split SplitCoinStakeOutput into two functions, one to perform the reward sidestaking, and the other to do the coinstake output splitting (UTXO optimization), but as long as we have an overall limit over both of them in terms of the overall number of outputs, it couples the two together. Further, vout[0] has to be blank, and the reward distribution has to be done before the splitting, because the reward distribution outputs are determinate, but the stake splitting is highly variable... so splitting the function into two requires some real gymnastics with the vector element order.

I have moved the conf parameter parsing all into the top part of StakeMiner, so that it executes only once when the miner thread is created, and not every time the miner loop is executed. The functions are now fully parameterized. Further changes to the parsing location and scope of the flags may be required to hook in the UI part. I would love commentary on that.

One last thing. This can be used on testnet, but you have to be careful because if you enable both enablestakesplit and enablesidestaking and more than three outputs are formed, the staked block will be rejected. This is because testnet is on V10, but does not currently have the expansion of the coinstake outputs to 8, so the test I have at miner.cpp:670 doesn't work properly.

So if you try this out in testnet, do not include more than one address, percent entry, and do not simultaneously enable both enablestakesplit and enablesidestaking.

One other thing. I have not moved CreateRestOfTheBlock(StakeBlock,pindexPrev) to later after the SplitCoinStakeOutput. I tried it, but was having trouble with it, so moved it back.

@barton2526

This comment has been minimized.

Copy link
Member

barton2526 commented Aug 16, 2018

Is there consideration here for a minimum threshold that must be met for a split value? Say I stake 1 GRC (assuming bv9) and I designate 1% of my reward to be sent to an address. This would send 0.01 GRC to the designated address. Am I correct in the assumption that this is the minimum value allowed at present? With CBR at 10, would this minimum rise to 1% of 10, which is 0.1 GRC?

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 16, 2018

I thought about that earlier, and we need to put a dust preventer in there. What do you think we should use, and it is super easy. An allocation that results in less than the minimum would be zeroed out, and the residual created would flow back to the coinstake...

@barton2526

This comment has been minimized.

Copy link
Member

barton2526 commented Aug 16, 2018

We already have a tx dust threshold defined in the code. Why not just use the same value here?

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 16, 2018

I think there is a constant for that right, or is it hard coded?

@barton2526

This comment has been minimized.

Copy link
Member

barton2526 commented Aug 16, 2018

// To limit dust spam, require MIN_TX_FEE/MIN_RELAY_TX_FEE if any output is less than 0.01

@barton2526

This comment has been minimized.

Copy link
Member

barton2526 commented Aug 16, 2018

It is defined currently as CENT, which is equal to 1/100 of a GRC, or 0.01 GRC.

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 16, 2018

It is super easy....

I will define a local variable
int64_t nRewardDist = (nReward * iterSideStake->second < CENT) ? 0 : nReward * iterSideStake->second;

and then replace the two occurrences of nReward * iterSideStake->second with that local!

Nope, better to use a if test and then continue... see below and the code.

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 16, 2018

Ah, but you are right, it may never be triggered because I am not allowing fractional percents... at least at a CBR of 10. Because .01 * 10 * COIN>CENT. The check should still go in should we ever change CBR...

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 16, 2018

Do we want to allow fractional percents?

@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch from 4cec4b9 to 6744416 Aug 16, 2018
@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 16, 2018

I put a continue in the reward processing loop that will prevent distributions of less than one CENT. This will cause the small undistributed portion to flow back to the coinstake. Note that this will not actually activate for CBR at 10, but the check should be in there anyway. Changing the commentary at the top.

@barton2526

This comment has been minimized.

Copy link
Member

barton2526 commented Aug 16, 2018

Do we want to allow fractional percents?

I would vote no. That would get extremely confusing very fast.

Copy link
Member

iFoggz left a comment

have some concerns/questions. 👍 but looks good so far

}
}

for(auto iter : vSideStakeAlloc)

This comment has been minimized.

Copy link
@iFoggz

iFoggz Aug 16, 2018

Member

do we have to go through this loop? could the logging not be in the mapMultiArgs["-sidestake"] area before the push_back? (yes u prob would make a floating point var for ease of this). probably wouldn't be a significant impact anyway. if it stays there then const as no need to copy. also if there is no sidestake settings we would hit this also even thou it would pan out to nothing if im correct.

Correct me if i'm wrong ofcourse. but it is shaping up pretty nice for a feature

This comment has been minimized.

Copy link
@jamescowens

jamescowens Aug 16, 2018

Author Member

Yes. I wanted to make sure stuff was making it into the vector < pair <>> properly, so it is an artifact of testing. I like at least the parsing read out into the debug but maybe it should all be debug2. I favor at least the highlights being straight LogPrintf. (I am the type person that likes to see what is going on...) The loops can be combined. The whole thing is conditioned upon the side stake flag, although someone could put 1 for the flag and then include no allocations... but that is ok. It will hit the for and then move on...

for (auto const& sSubParam : mapMultiArgs["-sidestake"])
{
ParseString(sSubParam, ',', vSubParam);
vSideStakeAlloc.push_back(std::pair<std::string, double>(vSubParam[0], atof(vSubParam[1].c_str()) / 100.0));

This comment has been minimized.

Copy link
@iFoggz

iFoggz Aug 16, 2018

Member

Leary about any ato* without a catch. people do make mistakes in config files, etc. we should handle such cases as better then a seg fault

This comment has been minimized.

Copy link
@jamescowens

jamescowens Aug 16, 2018

Author Member

I agree. I probably need a try catch in some other places too.

@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch 3 times, most recently from 30e676c to 5cd7a10 Aug 16, 2018
ParseString(sSubParam, ',', vSubParam);
sAddress = vSubParam[0];

try

This comment has been minimized.

Copy link
@iFoggz

iFoggz Aug 18, 2018

Member

I have concern here. with the try failures; It will continue the loop and continue allocations.

With a case where one is a bad allocation; is there a fall back plan so that the remainder of the side stake split that has not been allocated is not lost or not used? not my area of expertise but thought id ask what would happen if there was a failure of an allocation.

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 18, 2018

@iFoggz

This comment has been minimized.

Copy link
Member

iFoggz commented Aug 18, 2018

excellent !. base is covered!

// If side staking is enabled, parse destinations and allocations.
if (fEnableSideStaking)
{
if (mapArgs.count("-sidestake") && mapMultiArgs["-sidestake"].size() > 0)

This comment has been minimized.

Copy link
@iFoggz

iFoggz Aug 18, 2018

Member

if SideStaking is enabled but no -sidestake is set at all in config, we should log a warning message to state that side staking is enabled however no sidestake parameters have been set. lets be vocal where we can to assist a user who may of forgotten or not set the config options to support sidestaking.

This comment has been minimized.

Copy link
@jamescowens

jamescowens Aug 18, 2018

Author Member

I agree!

This comment has been minimized.

Copy link
@jamescowens

jamescowens Aug 18, 2018

Author Member

I added a comment at 1061 where the parsing is to ensure people know that skipped allocations result in the rewards going back into the coinstake... and I added a check for dSumAllocation == 0 at 727 and if it is, prints a warning message in debug.log.

@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch 2 times, most recently from 07d6c72 to 70b8e13 Aug 18, 2018
@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 19, 2018

I just tested address,74.5 and atof carries the decimal. So it carries through as 0.745 and the code DOES allow decimal percentages. I am glad I put in the dust limiter...

@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch 3 times, most recently from 54858ec to e3fc827 Aug 19, 2018
@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 19, 2018

Did a round turn on the validations in the parameter parsing to toughen it up...

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 19, 2018

I noticed that POR stakes are not displayed correctly when sidestaking is active. I think it is because transactionrecord.cpp has this stuff at line 95...

				if (wtx.vout.size()==2)
				{
					//Standard POR CoinStake
					sub.type = TransactionRecord::Generated;
					sub.credit = nNet > 0 ? nNet : wtx.GetValueOut() - nDebit;
					hashPrev = hash;
				}
				else
				{
					//CryptoLottery - CoinStake - 4-3-2015
					sub.type = TransactionRecord::Generated;
					if (nDebit == 0)
					{
						sub.credit = GetMyValueOut(wallet,wtx);
						sub.RemoteFlag = 1;
					}
					else
					{
						sub.credit = nNet > 0 ? nNet : GetMyValueOut(wallet,wtx) - nDebit;
					}

Looks like it should be yanked...

Comments?

@barton2526

This comment has been minimized.

Copy link
Member

barton2526 commented Aug 19, 2018

@jamescowens According to this reddit post (https://www.reddit.com/r/gridcoin/comments/3hpaae/gridcoin_research_3481_recommended_update/) CryptoLottery was removed in version 3.4.8.1 (released 20 Aug 2015). I think it's safe to remove lol.

@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch from e3fc827 to d0f7ca0 Aug 19, 2018
This is a small modification to ensure that if a sidestake is to
an output that happens to have the same address as the coinstake
address, which is entirely possible if the sidestake is local to
the staking wallet, it will be suppressed. The reward that would
be distributed will instead return in the coinstake outputs. This
makes more sense than unnecessarily separating them, and makes the
problem of displaying the right type for the stake in the UI easier.
@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch from d5b1b53 to 7ad0527 Aug 26, 2018
@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 26, 2018

Rebased onto current development branch...


try
{
dAllocation = atof(vSubParam[1].c_str()) / 100.0;

This comment has been minimized.

Copy link
@iFoggz

iFoggz Aug 26, 2018

Member

I made a whoopsie here in catching exceptions. we should not be using atof for some reason I thought we were using std::stof.

atof does not throw exceptions and can lead to undefined behaviour in some cases as mentioned by references

std::stof does the job and allows catching of exceptions std::invalid_argument std::out_of_range

i'm personally against a catch (...) even thou i consider this catch to be reasonable and within the bounds of what we need so leave the catch unless someone else requests that to be different

// * add gridcoin reward to coinstake
if( !CreateGridcoinReward(StakeBlock,BoincData,StakeCoinAge,pindexPrev) )
// * add gridcoin reward to coinstake, fill-in nReward
int64_t nReward;

This comment has been minimized.

Copy link
@iFoggz

iFoggz Aug 26, 2018

Member

= 0 please. avoid possible warnings :)

Copy link
Member

iFoggz left a comment

good work on the new commits, mentioned some concerns/changes :)

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 26, 2018

@iFoggz

This comment has been minimized.

Copy link
Member

iFoggz commented Aug 26, 2018

it does as long as they dont have inf or nan as the result it will be 0
and as long as they dont do a out of range number it wont result in inf

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Aug 26, 2018

You think I should change to stof to be safer? Probably, right? Then the catch would do its job.

@iFoggz

This comment has been minimized.

Copy link
Member

iFoggz commented Aug 26, 2018

as i mentioned on slack if its set to inf or nan it is undefined behavior.

nan + 100 = nan

nan is neither > 0, <0 nor == 0 :) thats the undefined i was meaning. so yes stof is the safe route since we deal with numbers and currency imo

@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch from 80f1106 to 6d3965c Aug 26, 2018
@iFoggz
iFoggz approved these changes Aug 26, 2018
Copy link
Member

iFoggz left a comment

utACK; I believe this PR is as far as it can go without direct testing on testnet. Approving to move to the testing stage.

@denravonska denravonska added this to the Camilla milestone Aug 27, 2018
@jamescowens jamescowens mentioned this pull request Sep 4, 2018
0 of 2 tasks complete
@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch from 9ec26ad to 0404074 Sep 9, 2018
This cleans up the string to float conversion on the parameter
parsing and fixes a corner case where an incomplete sidestake
entry is specified.
@jamescowens jamescowens force-pushed the jamescowens:SideStaking branch from 0404074 to f3dc430 Sep 9, 2018
@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Sep 9, 2018

I made some additional changes to the small cleanup commit to also check with the vSubParam vector is not 2 elements because of an incomplete sidestake entry in the config. This will prevent the dAllocation = stof(vSubParam[1]) / 100.0 from generating a segfault when it tried to reference an element that does not exist...

@iFoggz

This comment has been minimized.

Copy link
Member

iFoggz commented Sep 9, 2018

thank u for the prompt fix :)

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Sep 16, 2018

I have one more thing I want to do with parsing. It is possible that someone could have two identical sidestake addresses in the config file. I need to detect this. The question of what to do... merge the two entries together into one as long as the total doesn’t raise the percentage above 100% along with a warning, or simply ignore the second entry with a warning. Thoughts?

@iFoggz

This comment has been minimized.

Copy link
Member

iFoggz commented Sep 16, 2018

i would ignore it and not accept it before hand. maybe some comparator or something that does not the duplicate entry. would kind of be a pain to re run just to calc that one out.

@tomasbrod

This comment has been minimized.

Copy link
Member

tomasbrod commented Sep 16, 2018

I would not care. Treat the address as black box and if someone enters same address two times, send the money to the address two times. KISS

@jamescowens

This comment has been minimized.

Copy link
Member Author

jamescowens commented Sep 16, 2018

@denravonska denravonska merged commit b7611c8 into gridcoin-community:development Sep 25, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
denravonska added a commit that referenced this pull request Apr 3, 2019
Added:
 - Add `rainbymagnitude` RPC command #1235 (@Foggyx420).
 - Add stake splitting and side staking #1265 (@jamescowens).
 - Detect and block Windows shutdown so wallet can exit cleanly #1309
   (@jamescowens).
 - Add message support to sendfrom and sendtoaddress #1400 (@denravonska).

Changed:
 - Configuration options are now case insensitive #294 (@Foggyx420).
 - Update command in beaconstatus help message #1312 (@chrstphrchvz).
 - Improve synchronization speeds:
   - Refactor superblock pack/unpack #1194 (@denravonska).
   - Optimize neuralsecurity calculations #1255 (@denravonska).
   - Reduce hash calculations when checking blocks #1206 (@denravonska).
 - Make display of private key in beaconstatus OPT-IN only #1275 (@Foggyx420).
 - Store Beacon keys in Wallet #1088 (@tomasbrod).
 - Use default colors for pie chart #1333 (@chrstphrchvz).
 - Show hand cursor when hovering clickable labels #1332 (@chrstphrchvz).
 - Update README.md #1337 (@Peppernrino).
 - Fix integer overflow with displayed nonce #1297 (@personthingman2).
 - Improve application cache performance #1317 (@denravonska).
 - Improve reorg speeds #1263 (@denravonska).
 - Update Polish translation #1375 (@michalkania).

Fixed:
 - Remove expired polls from overview page #1250 (@personthingman2).
 - Fix plural text on block age #1304 (@scribblemaniac).
 - Fix researcher staking issue if your chain head was staked by you,
   #1299 (@denravonska).
 - Fix incorrect address to grcpool node #1314 (@wilkart).
 - Do not replace underscores by spaces in Qt Poll URLs #1327 (@tomasbrod).
 - Fix scraper SSL issues #1330 (@Foggyx420).

Removed:
 - Remove or merged several RPC commands #1228 (@Foggyx420):
    - `newburnaddress`, removed.
    - `burn2`: Removed.
    - `cpid`: Merged into `projects`.
    - `mymagnitude`: Merged into `magnitude`.
    - `rsa`: Removed, use `magnitude`.
    - `rsaweight`: Removed, use `magnitude`.
    - `proveownership`: Removed.
    - `encrypt`: Removed.
 - Remove obsolete POW fields from RPC responses #1358 (@jamescowens).
 - Remove obsolete netsoft fields for slight RAM requirement reduction
   #1336 (@denravonska).
 - Remove unused attachment functionality #1345 (@denravonska).
@jamescowens jamescowens deleted the jamescowens:SideStaking branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.