[Staking] Prevent potential negative out values during stake splitting

[CaveSpectre11]

On the rare occasion that a small UTXO wins a stake; and that user has (by lack of understanding, or by error) configured a very small StakeSplitThreshold; they can end up in a situation where their extremely lucky stake win fails at consensus because of the way the stake split is calculated.

The logic split the block reward between the two outputs; along with the input. However if that input is significantly small, and the block reward is significantly weighted to the masternode; then the potential for a negative (and thus very very large) output would be created in the second side of the split. This is due to the fact that the full masternode reward is taken out of the "previous" output (vout[i - 1]).

This is a rare, if not relatively impossible, scenario with PIVX at it's current state, but the potential grows much larger when coin parameters are re-arranged; e.g. small stakes with a heavy weight onto the masternode reward. i.e. a stake split of 5, with a block reward of 10, and 90% to the masternode. 15 ends up split into vout[1] and vout[2], both receiving 7.5; yet FillBlockPayee will subtract 9 from vout[2]; resulting in a negative wrapped to a large positive.

This logic simply puts the reward that will be deducted from into the second vout. So with the above numbers; vout[1] is split to 2.5, vout[2] is initially 12.5 and then after the unknown (at that time) masternode fee of 9, is reduced to 3.5; no longer going negative.

Of course this is an extreme example; but the potential still exists.

Alternatively, restrictions could be placed on setstakesplitthreshold to prevent the user from setting a threshold dangerously low; or a minstake could be setup to prevent a stake small enough to be a problem from being considered. However, this seems like the best solution to the problem; by handling it without adding a lot of logic to collect the chain parameters and set limits that make sense to whatever chosen configuration is used.

[random-zebra]

Concept ACK. Nice find!
However, with this fix the stake wouldn't be equally split between the outputs (the staker reward would be added only to the last one).
I wonder if it wouldn't make more sense to, instead, subtract the masternode payout "equally" (except for a reminder) from all coinstake outputs.

Something like this in CMasternodePayments::FillBlockPayee maybe?

//subtract mn payment from the stake reward
if (i > 2) {
    // stake has been split in (i-1) outputs
    CAmount quotient = masternodePayment / (i-1);
    CAmount reminder = masternodePayment - quotient * (i-1);
    for(unsigned int j=1; j<i; j++)
        txNew.vout[j].nValue -= quotient;
    txNew.vout[i-1].nValue -= reminder;

} else {
    txNew.vout[1].nValue -= masternodePayment;
}

[CaveSpectre11]

Something like this in CMasternodePayments::FillBlockPayee maybe?

Yea, I like that idea. Actually brings up an interesting question; since the FillBlockPayee logic is setup to handle an unknown number of outputs; whereas the CreateCoinStake() is specifically if outputs == 3 (a split into two outputs. So in this situation it doesn't -need- to handle more than distributing between the two.

However, the logic you describe leaves code that functionally will work; and won't require changes if we ever decide to have CreateTxOuts check how many multiples of nStakeSplitThreshold nTotal is, and add an output to split respectively (ok, now my mind is churning, I'll look at that this weekend. e.g. staking 10k w/ 2k threshold would result in ten 1k outputs, 9999 w/ 2k would result in nine 1111 outputs, etc)

[random-zebra]

Actually It's not even needed to use a conditional there.
As when i == 2 we have quotient = masternodePayment and reminder = 0.

[CaveSpectre11]

Actually It's not even needed to use a conditional there.
As when i == 2 we have quotient = masternodePayment and reminder = 0.

True; although the underlying efficiency process in my head sees the division and I would opt for the conditional to fast path the majority of the callers; sending only those that need special processing (those being split) into the mathematical slow path.

[CaveSpectre11]

Logic moved to FillBlockPayee() to get an even split across all the outputs (per @random-zebra suggestion). A few things to note:

1. While the common path (one output) could be run through the split scheme, with the division being by one, no remainder, and the for loop passed one time; that's an order of magnitude work added to the common path. By differentiating, the most common case only has one conditional added, rather than all the extra logic for the split case.
2. CreateTxOuts() currently only splits into two outputs, but the logic in FillBlockPayee() supports any number of outputs. Rather than restricting FillBlockPayee() to the limitation of CreateTxOuts(), the "any number of outputs" logic is preserved, for the future potential of changing CreateTxOuts() to split into several outputs (splitting once, rather than repetitive halving).

[CaveSpectre11]

Split off additional change to #967 & rebased

[random-zebra]

ACK 139d333

BEFORE:
https://testnet.pivx.link/tx/e0b6bef1f56f14557a5cc05b1e740f6bbe9ece1a43aeda189f835eeddb1a9a17

AFTER:
https://testnet.pivx.link/tx/ef5e381fb785a01159e8d8aa27a68fe9112210a435f8ecb7aadf7613b57731ab

[Warrows]

utACK 139d333

[Mrs-X]

utACK 139d333 and merging...