Skip to content

Conversation

@CaveSpectre11
Copy link

(When trying to rebase and clean up the commits in #923 ; the PR automatically closed during the cleanup, and I can't seem to get it back open.)

Problem:
A problem was introduced with #518 when a UTXO smaller than 10% of the threshold is added to the collection of UTXOs to combine, and that UTXO is enough to increase the total amount being combined above the threshold, or when the total amount of dust in the wallet exceeds the threshold by less than 10%.

What occurs is two fold. First, the nTotalRewardsValue > nAutoCombineThreshold will break it out of the for loop; but the "safety margin" will split it up into two UTXOs; one within 10% of the threshold, and then one for the change. When the wallet comes back through on it's dust collection, it can pick up those two UTXOs and repeat until the fees whittle the two combined transactions to a total below the threshold.

If there is another UTXO to add, which takes the logic far enough above the threshold to remain above the threshold after the 10% reduction; the check in the loop is functional. However there is still one other problem case: If the total amount being collected falls into the "within 10% of the threshold" situation, and the for loop can't make another pass... we exit the for loop normally, and find our way into the zero fee check. However the zero fee check sees we are over the threshold, but not that the transaction amount will be over the threshold. So we need to account for the 10% there as well, by using the actual amount (vsecSend[0].second) rather than nTotalRewardsValue to determine if we should continue only if free.

Even with this fix, AutoCombineRewards was still hardly usable, as wallet scans on wallets with many active addresses was consuming significant resources when running AutoCombineDust() on every block. In order to make this feature more useful and usable, further features were folded in; The ability to configure how often to run the scan, and to configure it to combine as much as possible when it executes.

Improvement 1: Max Threshold:
Users trying to clean up wallets that have collected a lot of small transactions may find it more convenient to combine as much as possible with one sweep. Rather than guessing what amount would be best, and having to do multiple passes; setting a threshold of "0" will autocombine up to the max it is capable of. This is also even more useful for users that have several transactions across each of many addresses,, as they would need to get each wallet down to a very few transactions before sweeping the funds into a single large transaction across many input addresses. This allows for autocombine to quickly and easily combine to very few UTXOs in each wallet, and allowing a sweep across many wallet addresses to be much more convenient. This would function very much like a "defragment" routine.

Improvement 2: Frequency:
An old stale "fix" was floating around in internet, implementing an AutoCombineRewardsThresholdTime concept; I was unable to locate the originator. This implementation had allowed you to configure the number of minutes between dust collection. That old algorithm was commented out and replaced with a 5 second wait. This concept also was flawed, in that it was going to be doing a sleep within the ProcessNewBlock() code. The first attempt was abandoned because it likely was blocking ProcessNewBlock for 15 minutes at a time, and no way to not block, if using the feature, for under a minute. The workaround (5 seconds) still wasn't desirable, as it would still lock up block ProcessNewBlock's execution for 6 seconds.

This new method changes that design from ThresholdTime, to Block Frequency, and defaults to 15 blocks. Time can be adjusted per implementation, to get a desired minute time based on the block frequency of the coin parameters. If AutoCombineDust is enabled, it will only check and attempt to combine dust if the block height is a multiple of the Block Frequency. e.g. if set to 10, then every 10th block it will be executed. 100; then every 100th block.

This can now be tailored by the user, based on their desired dust cleanup threshold, and their expectation of frequency that they will need to clean.

Improvement 3: One Shot:
The concept of a "one shot" dust cleanup was added when the block frequency is set to "0". This will run the automatic rewards combining on the next block, and then it will disable itself until the next time the wallet is started up. This feature assumes that someone would be running their wallet infrequently, and therefore is likely to want the one shot to run when they start the wallet again. As such, the enabled=true and frequency=0 is saved in the database, but after it runs, it will be shut off for the duration of the wallet run.

To run it for a one time only execution, the user would have to autocombinerewards false after the one shot is executed.

The feature, in it's entirety:

  • Defaults to false
  • When "true", requires a threshold (in coin count). Threshold of "0" will combine up to the maximum transaction size.
  • Block frequency defaults to every 15 blocks. This can changed to every block (resource intensive) or however many blocks one expects their threshold to hit.
  • Block frequency of "0" will run the sweep on the next block, and then turn it off until they run it again, or restart their wallet.
  • To prevent the one shot to run on startup, they simply need to turn it off after the sweep is complete (when getautocombineinfo returns "on startup" rather than "on next block")

Examples:

Basic configuration for frequency use

autocombinerewards true 5 250
{
  "enabled": "on",
  "threshold": 5,
  "frequency": 250
  "comment": "Enabled for repeat on frequency"
}

Will run every time the block count is a multiple of 250, and combine if it finds multiple UTXOs. It will collect inputs until the total exceeds 5.5 PIVX. If the total is below the threshold + 10%, it will only submit the transaction if the fee is considered free.

Configuring for frequency use, maximum combine

autocombinerewards true 0 250
{
  "enabled": "on",
  "threshold": 0,
  "frequency": 250
  "comment": "Enabled for maximum combine, repeat on frequency"
}

Will run every time the block count is a multiple of 250, and combine if it finds multiple UTXOs. It will collect as many inputs as possible, up to the maximum bytes allowed per transaction (100kb).

Configuring One Shot

autocombinerewards true 5 0
{
  "enabled": true,
  "threshold": 5,
  "frequency": 0
  "comment": "Enabled for one shot on next block"
}

Before the run

getautocombineinfo
{
  "enabled": true,
  "threshold": 5,
  "frequency": 0
  "comment": "Enabled for one shot on next block"
}

After the run

getautocombineinfo
{
  "enabled": false,
  "threshold": 5,
  "frequency": 0
  "comment": "Enabled for one shot on startup"
}

Please Note: The underlying functionality has been tested on two altcoins, and the RPC logic has been tested standalone on PIVX testnet.

@Warrows
Copy link

Warrows commented Jul 19, 2019

It might be nice to check that the result when the threshold is set to zero is not > to the setstakesplitthreshold (if it is set). Otherwise the two functionalities are gonna clash.

@Warrows Warrows self-requested a review July 19, 2019 12:35
@Warrows Warrows added this to the 4.0.0 milestone Jul 19, 2019
@CaveSpectre11
Copy link
Author

It might be nice to check that the result when the threshold is set to zero is not > to the setstakesplitthreshold (if it is set). Otherwise the two functionalities are gonna clash.

Ahh yes; excellent point. For the primary use of "0" (cleaning up a wallet with a ton of masternode rewards), the user probably won't want it to stop at the stakesplitthreshold; so I definitely don't want to limit it. However I think it's fair to warn them if (threshold == 0) || (threshold > stakesplit) && frequency != 0) (If max combine or threshold could create a utxo greater than stakesplit, and it's not a one shot... warn). Also in stakesplit.. if stakesplit < threshold and autocombine is enabled....

Let me look through the code to see if there's any standard to warning messages that plays well with rpc outputs.

@CaveSpectre11
Copy link
Author

CaveSpectre11 commented Jul 21, 2019

Added checks between autocombinerewards and stakesplitthreshold. One thing of note; stakesplitthreshold isn't quite up to the common standard in the code (getstakesplitthreshold doesn't return JSON format; help isn't the same format). Those should be addressed in a separate PR.

spectre@Fortune:~/git-repos/PIVX/src$ ./pivx-cli autocombinerewards true 3000 1000
{
  "enabled": true,
  "threshold": 3000,
  "frequency": 1000,
  "comment": "Enabled for repeat on frequency",
  "warning": "***Warning: Threshold level may conflict with stakesplitthreshold!***"
spectre@Fortune:~/git-repos/PIVX/src$ ./pivx-cli getautocombineinfo
{
  "enabled": true,
  "threshold": 3000,
  "frequency": 1000,
  "comment": "Enabled for repeat on frequency",
  "warning": "***Warning: Threshold level may conflict with stakesplitthreshold!***"
spectre@Fortune:~/git-repos/PIVX/src$ ./pivx-cli setstakesplitthreshold 4000
{
  "threshold": 4000,
  "saved": "true",
  "warning": "(none)"
}
spectre@Fortune:~/git-repos/PIVX/src$ ./pivx-cli getautocombineinfo
{
  "enabled": true,
  "threshold": 3000,
  "frequency": 1000,
  "comment": "Enabled for repeat on frequency",
  "warning": "(none)"
}
spectre@Fortune:~/git-repos/PIVX/src$ ./pivx-cli setstakesplitthreshold 2000
{
  "threshold": 2000,
  "saved": "true",
  "warning": "***Warning: Threshold level may conflict with autocombinerewards threshold!***"
spectre@Fortune:~/git-repos/PIVX/src$ ./pivx-cli autocombinerewards true 1000 1000
{
  "enabled": true,
  "threshold": 1000,
  "frequency": 1000,
  "comment": "Enabled for repeat on frequency",
  "warning": "(none)"
}

Warrows
Warrows previously approved these changes Jul 21, 2019
Copy link

@Warrows Warrows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dfec18b

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of positioning nits

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rebase

@CaveSpectre11
Copy link
Author

Needs rebase

done

Warrows
Warrows previously approved these changes Aug 3, 2019
Copy link

@Warrows Warrows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 09acb52

random-zebra added a commit that referenced this pull request Sep 17, 2019
…output splitting

2e41db5 [Staking] Revise stakesplit to multi-split (Cave Spectre)

Pull request description:

  ### **Release notes**
  - [Staking] Staking inputs are now split into 2 or more outputs based on the stake split threshold

  ### **Note**
  For ease of rebasing; and for cleaner testing; this PR is built on top of PR #952 and will need adjustments if PR #952 has changes.

  ### **Background**
  The common function of `stakesplitthreshold` is to break a stake that has grown large into two outputs, splitting them in half.  It is assumed the size of the split is based on the users amount of coins they are staking, and they desire their staking UTXOs to be sized between their stake split threshold and 2 times that stake split threshold.

  This design falters when a stake UTXO is a multiple of the stake split threshold; causing repetitive halving until all outputs reach below twice the stake split threshold.

  For example, with a stake split threshold of 2000; a UTXO will grow until it hits 4000 coins, at which point it will split to 2000 coins.  However if the UTXO was 10,000 coins; it would split to two 5000 coin UTXOs, which would then split on the next stake for each of them; to 2500 coins.

  This becomes more problematic when considering the impact to the coin owners staking odds.  After a split; the coins will not stake until they mature.  So when that 10k splits into two 5k blocks, they won't stake for another 101 transactions.   After 101 transactions, the 10k is now being staked, until the next split; at which point only 5k is staking for the next 101 transactions.

  This issue comes up in a variety of scenarios;  A large transaction from an exchange or a payment received, Unlocking a masternode for staking, auto combine rewards (especially with PR #953) sweeping up a lot of dust bunnies into a big UTXO; just to name a few.

  ### **Multi-Split**
  Multi-Split will now use the stake split threshold to determine how many multiples of the threshold is contained in the input transaction; and split accordingly.  In the theoretical example above; that first stake of 10k will now be split into 5 outputs of 2000.4.  When one of those outputs receives a stake, instead of only the other 5k being staked for the 101 transactions to maturity; now 8k will remain staked for the maturity period of the UTXO (post split) being unavailable for staking.

  For a real example, with a stake split threshold of 1500, and a UTXO of 4708.1557; the current stake split algorithm would break that into two outputs of approximately 2355.07785.  With this new logic; it will be broken into 3 outputs instead of two; each sized 1570.0519 (4708.1557 input + 2 stake = 4710.1557 / 3 outputs = 1570.0519.

  _See this test transaction in [testnet block 1171496](https://testnet.pivx.link/block/1171496)_

  In order to limit the potential size growth of the staking transaction; the number of splits is limited.  This limit is a calculation based on the MAX_STANDARD_TX_SIZE, and is meant to limit the staking transaction to less than 10% of that size.  With an assumed per vout size of 190, rounded up, and converted to a bit shift for speed; MAX_STANDARD_TX_SIZE is shifted right 11 bits.  In simpler math:
   The limit is effectively MAX_STANDARD_TX_SIZE divided by 2048, creating a maximum limit of outputs for the stake split to 48 (with the current MAX_STANDARD_TX_SIZE of 100k).

  An example of this test can be seen in [testblock 1171785](https://testnet.pivx.link/block/1171785), where a UTXO of 13114.83882369 coins was used for a stake; and the stake split threshold was set to 100.  The logic would have split this into 131 outputs of 131.116 and change, however the maximum threshold is reached, and thus it is split into 47 outputs of 273.26747549, with the 48th output having the .17 uPiv remainder added and resulting in that final output of 273.26747566.

  As the stake splitting is contained within the wallet software itself; there is **no protocol change with this PR**; the consensus is already built to look at the last vout for the masternode payment, and look at the total value of a variable number of outputs to confirm that an overmint is not occurring.

ACKs for top commit:
  random-zebra:
    ACK 2e41db5
  Fuzzbawls:
    utACK 2e41db5
  furszy:
    utACK [2e41db5](2e41db5)

Tree-SHA512: 35f612a5042de964bd3fda040ff2e3ea24de81300ecdc7d9d322757471df45cc4c7cb3d7905554ec41b607bb8364a94ced372c93ac7963c7729653d88cf32765
@random-zebra random-zebra modified the milestones: 4.0.0, 4.1.0 Nov 25, 2019
@CaveSpectre11
Copy link
Author

Rebased with current (pre-4.0) master and resolved conflicts.

@random-zebra random-zebra modified the milestones: 4.1.0, Future Mar 30, 2020
@random-zebra random-zebra modified the milestones: Future, 5.0.0 Apr 24, 2020
CaveSpectre11 and others added 2 commits May 8, 2020 21:27
Repair autocombinerewards issues, add the ability to configure the
frequency that it is run, the ability to combine up to the maximum
transaction size allowed, and backwards compatability to the old
wallet format.
@CaveSpectre11
Copy link
Author

Rebased with current master

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this work @CaveSpectre11 and sorry for the late review.
AutocombineRewards is another of those things that have been put on the backburner, and are probably in need of a good overhaul.

This PR adds nice improvements though so, I think, it should get merged.
While going through it, I've noticed a discrepancy (not due to this PR) in the way pwalletMain->nAutoCombineThreshold is saved (as CAmount) and the way it is set and considered (as int corresponding to a whole PIV value).
This breaks the inequalities with nStakeSplitThreshold (which is considered and saved as CAmount, i.e. a number corresponding to the satoshis), and thus requires additional multiplications (by COIN) during CWallet::AutoCombineDust(). See inline comments.
To avoid confusion this should be made consistent: either save its variable in the wallet as int, or treat it as CAmount everywhere. I think this second option is better, and in line with the stake split threshold.

Aside from that, one minor concern is about the databasing of the new settings.
Since we are going to add a new key anyway, wouldn't it make more sense to simply use it for the frequency? This way the "old" settings (fEnable and threshold) can keep the old place/datastructure and the frequency could simply take the default value of 15 when not set. This would prevent the complex logic of "invalidating" the old settings with a dummy threshold value of -1 , rewriting it each time we call WriteAutoCombineSettings.


if (fEnable)
if (fEnable) {
nThreshold = params[1].get_int();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The threshold is inserted in PIV value (either as integer or decimal) but it must be saved as CAmount, e.g.

nThreshold = AmountFromValue(params[1]);

to be able to be checked against the nStakeSplitThreshold and the output values in CWallet::AutoCombineDust()

if (pwalletMain->fCombineDust) {
std::string strComment;

obj.push_back(Pair("threshold", int(pwalletMain->nAutoCombineThreshold)));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above. pwalletMain->nAutoCombineThreshold should be treated as CAmount so can't be directly cast to int, without dividing by COIN. So here we should use

obj.push_back(Pair("threshold", ValueFromAmount(pwalletMain->nAutoCombineThreshold)));

Comment on lines +3050 to +3056
// Warn if this will conflict with AutoCombineRewards
if (((pwalletMain->nAutoCombineThreshold > nStakeSplitThreshold) || !pwalletMain->nAutoCombineThreshold)
&& (pwalletMain->fCombineDust)) {
strComment = "***Warning: Threshold level may conflict with autocombinerewards threshold!***";
} else {
strComment = "(none)";
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is repeated in quite a few places, could make sense to abstract it as a boolean method of CWallet.
Also it should consider the case of stake split disabled (nStakeSplitThreshold == 0).

// Combine until our total is enough above the threshold to remain above after adjustments
// Unless no threshold is set; in which case we want to keep going until we hit MAX_STANDARD_TX_SIZE
if (nAutoCombineThreshold &&
((nTotalRewardsValue - nTotalRewardsValue / 10) > nAutoCombineThreshold * COIN))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nAutoCombineThreshold is a CAmount already. If we treat it as such (and not like an int) there's no need to multiply by COIN.


//we don't combine below the threshold unless the fees are 0 to avoid paying fees over fees over fees
if (!maxSize && nTotalRewardsValue < nAutoCombineThreshold * COIN && nFeeRet > 0)
if (!maxSize && vecSend[0].second < nAutoCombineThreshold * COIN && nFeeRet > 0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above. no need to multiply by COIN.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as zebra, thanks for the work and sorry for the late review. Took some time, and lot of other priorities, but we are here now :) .

Left few code comments to be tackled with the others.

if (nAutoCombineBlockFrequency != 0) {
// If the block height hasn't exceeded our frequency; or is not a multiple of our frequency.
if ((nAutoCombineBlockFrequency > chainActive.Tip()->nHeight) ||
(chainActive.Tip()->nHeight % nAutoCombineBlockFrequency)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of call chainActive.Tip() again, use the variable tip that it's at the beginning of the method.

LOCK2(cs_main, cs_wallet);
const CBlockIndex* tip = chainActive.Tip();
if (tip->nTime < (GetAdjustedTime() - 300) || IsLocked()) {
return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to adjust this to the current PoS time protocol.

std::pair<bool, CAmount> pSettingsOld;
pSettingsOld.first = fEnable;
pSettingsOld.second = -1; // Negatives doesn't make sense, so we can use them as flags
if (Write(std::string("autocombinesettings"), pSettingsOld, true)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of write the old format with an special flag, can simple erase it from the DB and directly use v2 always.

@furszy furszy modified the milestones: 4.2.0, Future Jul 1, 2020
@random-zebra
Copy link

No follow-up. Closing.

@random-zebra random-zebra removed this from the Future milestone Aug 17, 2021
Fuzzbawls added a commit that referenced this pull request Mar 8, 2023
…hreshold

63488be Update functional test wallet_autocombine.py (Alessandro Rezzi)
e7437c9 Update functional test DeprecatedRpcTest (Alessandro Rezzi)
1c300a2 Remove deprecated autocombinerewards RPC command (Alessandro Rezzi)
9ea65d5 Fixed and improved setautocombinethreshold (Alessandro Rezzi)
66dbb2e Fixed current AutoCombineDust (Alessandro Rezzi)

Pull request description:

  AutoCombineDust has been bugged since an old PR see (#953  #518).

  To make an example of what's happening let's consider for simplicity  nAutoCombineThreshold = 100 and let's say that the user has 2 UTXOs one of 90 PIVs and the other of 15 PIVs.

  With the current code of AutoCombineDust we will therefore try to send a total of 90+15 = 105 PIVs to ourself, more precisely to avoid the "Insufficient funds" the primary output will contain only the 90% =94.5 PIVs and the remaining 10% = 10.5 PIVs is used to pay fees + sent as a change to ourself. As you can see we end up with two new UTXOs, which are still both smaller than the threshold.

  This process is iterated block by block until we end up with a single UTXO which is smaller than the threshold, i.e in this example 5 PIVs will be burn in fees.

  Moreover the process of "checking all UTXOs at each block for each address of your wallet"  can potentially consume a lot of resources especially if the user has a lot of small UTXOs, therefore I added a new parameter "frequency" which will check for dust every N blocks (where N is the value of frequency), and which is set by default at N = 30.

  Finally I removed the old autocombinerewards since it is deprecated and was planned to be removed in v6.0 (https://github.com/PIVX-Project/PIVX/blob/master/doc/release-notes.md)

ACKs for top commit:
  Fuzzbawls:
    ACK 63488be
  Liquid369:
    tACK 63488be

Tree-SHA512: b06046590d63399e4f579fbcd899f77346c0c5b8fd69613b60874f3920b2d776da97878a5794336c873012757ebef969467f4381441903231bf41c4d70c4635f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants