[RPC] [Wallet] AutoCombineRewards fixes and Improvements

[CaveSpectre11]

Try 3, after battling through rebase and squash unsuccessfully. See more in #867 .

Initially, this fix is a rebase of PR #849 ; which is contained within wallet.cpp. More description is written in that PR, however the nutshell version is that there were certain situations were you would continually try to combine the previous combination (below the threshold) and the change (taking you above the threshold).

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. I found an old change in many PIVX based coins that implemented a concept of "AutoCombineRewardsThresholdTime", which 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 ProcessNewBlock for the 5 second block.

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.

After some testing, I added one other addition; the concept of a "one shot" dust cleanup, allowing you to specify a block frequency of "zero", which will run the autocombine on the processing of the next block. This functionality 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 "on" and "frequency 0" is saved in the database, but after it runs, it will be shut off for the duration of the wallet run.

If they do not want it running on startup; they would simply "autocombinerewards false" after it's done it's one shot.

The feature, in it's entirety:

• Defaults to false
• When "true", requires a threshold (in coin count)
• 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")

For example.
Configuring One Shot

autocombinerewards true 5 0

{
  "enabled": "on",
  "threshold": 5,
  "frequency": "nextblock"
}

Before the run

getautocombineinfo

{
  "enabled": "on",
  "threshold": 5,
  "frequency": "nextblock"
}

After the run

getautocombineinfo

{
  "enabled": "off",
  "threshold": 5,
  "frequency": "startup"
}

Configuring for frequency use*

autocombinerewards true 5 250

{
  "enabled": "on",
  "threshold": 5,
  "frequency": 250
}

The latter will run every time the block count is a multiple of 250.

Please Note: This code has been tested on two Pivx based coins I have implemented it in. I, however, am not a PIVX holder so I was unable to test this code in PIVX itself; but I felt the changes makes autocombinerewards a useful feature, and porting back to PIVX is the right thing to do. Conceptually it should work fine, however there may have been a mistake made when porting, so please test on my behalf.

[CaveSpectre11]

UniValue autocombinerewards(const UniValue& params, bool fHelp)
{
    string strCommand;
    bool fEnable = false;

strCommand is actually an unused variable; I had intended to have "true|false|onetime", but I ran into issues with strings and booleans mixed together. Still considering changing it from "true|false" to "on|off|onetime", but with it functional, it's a much lower priority.

[Warrows]

Concept ACK.
There is a small documentation mistake though.

[Warrows]

ACK ba7cd7f

[Mrs-X]

Looks good, utACK.

[Mrs-X]

ACK with a little nit for readability:

vecSend.push_back(make_pair(scriptPubKey, nTotalRewardsValue));
...
...
vecSend[0].second = nTotalRewardsValue - (nTotalRewardsValue / 10);

could be a one-liner with

vecSend.push_back(make_pair(scriptPubKey, nTotalRewardsValue - (nTotalRewardsValue / 10)));

[Fuzzbawls]

Would like to see the if statements to have their sides reversed as commented, that would be more in line with everywhere else in the codebase when checking a variable against a static numeric.

Also, needs more testing when upgrading from a wallet what had previously used autocombinerewards; the change in walletdb.cpp's ReadKeyValue() is of particular concern here, as pSettings has been changed. Initial tests resulted in a non-fatal error dialog being displayed while loading the wallet.

[Mrs-X]

std::pair<std::pair<bool, CAmount>,int> pSettings;
...
...
pwallet->fCombineDust = pSettings.first.first;
pwallet->nAutoCombineThreshold = pSettings.first.second;
pwallet->nAutoCombineBlockFrequency = pSettings.second;

This causes a permanent "Warning: error reading wallet.dat! All keys read correctly, but transaction data or address book entries might be missing or incorrect." warning during start which does not disappear without repairing wallet.dat.

I think we need something more user-friendly for this to not be drowned in user complains.

Note: this only happens with wallets which control masternodes, others seem to start without the warning. which have already autocombine settings persisted before.

(didn't see that @Fuzzbawls mentioned this also. I should have refreshed the page before posting :-) )

[CaveSpectre11]

Note: this only happens with wallets which have already autocombine settings persisted before.

(didn't see that @Fuzzbawls mentioned this also. I should have refreshed the page before posting :-) )

No worries; the additional observations help me understand the issue (and why I didn't see it during testing). Two questions.

1. Does re-establishing your autocombine settings resolve the error (as it now saves with the new settings?) (not an acceptable 'solution' I know)
2. Is there any established technique existing in the code to differentiate between old and new format when reading the values?

[CaveSpectre11]

Would like to see the if statements to have their sides reversed as commented, that would be more in line with everywhere else in the codebase when checking a variable against a static numeric.

I would prefer not to change this. The first time I saw this coding style was when reviewing code from a foreign team I was leading; I made them change it to accepted standard. It kept popping up in their code, and I had an epiphany one night.

The coding style of having constants to the left in conditionals moves the extremely common compare vs. assignment bug from a difficult investigation to a simple compiler error. A brilliant little technique that I then adopted; and I wish they had defended it when I flagged it initially.

Many years later, my compiler complained that I was trying to assign a value to a constant, and sure enough... I had = instead of ==. So that etched the style permanently into my brain.

So yes @Fuzzbawls if you insist, I will reverse it. However I think the reason why I adopted that style is worthy of your consideration.

[Fuzzbawls]

my main rationale in wanting the if statement's conditionals reversed is to adhere to what is already adopted as "standard" in the codebase. that is to say that the variable itself is compared against a static numeric, rather than a static numeric being compared against a variable...it is semantics, for sure, but consistency is important.

[CaveSpectre11]

Note: this only happens with wallets which have already autocombine settings persisted before.
(didn't see that @Fuzzbawls mentioned this also. I should have refreshed the page before posting :-) )

No worries; the additional observations help me understand the issue (and why I didn't see it during testing). Two questions.

1. Does re-establishing your autocombine settings resolve the error (as it now saves with the new settings?) (not an acceptable 'solution' I know)
2. Is there any established technique existing in the code to differentiate between old and new format when reading the values?

@Mrs-X @Fuzzbawls . I have been looking into this some, although I don't have any access to a wallet to play with. Am I correct in seeing that ssValue is a datastream, and that it's going to read the size of pSettings; regardless of what was saved, so it's going to either read past the end of the file (if the record was last), or read too much and mess up the reads since the stream will be off?

If that's the case; it looks like it's why there's multsend vs. msettingsv2; and the established away would be to leave autocombinesettings as is, and make an acsettingsv2 for the new format.

After typing all of the below , I'll move forward with creating autocombinesettingsv2 logic to maintain a separate record to address this (and address the other nits while I'm at it); attempt to reproduce the situation in my implementation and verify the fix.

Ideally; having these have a size associated with it would open backwards compatibility (an idea for you guys to think about for the future), such as (pseudo)

            std::pair<bool, CAmount> pSettingsOld;
            std::pair<std::pair<bool, CAmount>,int> pSettings;
            ssValue >> recordSize;
            if (pSettingsOld.size() == recordSize) {
                       ssValue >> pSettingsOld;
                       pwallet->fCombineDust = pSettingsOld.first;
                       pwallet->nAutoCombineThreshold = pSettingsOld.second;
                       pwallet->nAutoCombineBlockFrequency = 15; // default
           } else if (pSettings.size() == recordSize) {
                       ssValue >> pSettings;
                       pwallet->fCombineDust = pSettings.first.first;
                       pwallet->nAutoCombineThreshold = pSettings.first.second;
                       pwallet->nAutoCombineBlockFrequency = pSettings.second;
           } else {
                       recordSizeError = true;
           }  
[...]
    } catch (...} {
        return false;
    }
    if (recordSizeError) {
        strError="Error reading wallet database: "<< strType << " record corrupt";
        return false;
    }
    return true;

[Mrs-X]

@CaveSpectre11

Does re-establishing your autocombine settings resolve the error (as it now saves with the new settings?)

Yes, it does.

Is there any established technique existing in the code to differentiate between old and new format when reading the values?

Only if your change would be introduced with a protocol update.
I think a proper way would be to move it out of wallet.dat into its own file (similar to banlist.dat or budget.dat), but that would need a protocol update as well.

A (ugly) temporary solution could be something like:

• move the code for loading the autocombine settings to a new method
• there do something like

bool getAutocombineSettings()
{
    bool fAutocombineException = true;

    try {
        fAutocombineException = false;
        std::pair<std::pair<bool, CAmount>,int> pSettings;
        ssValue >> pSettings;
        pwallet->fCombineDust = pSettings.first.first;
        pwallet->nAutoCombineThreshold = pSettings.first.second;
        pwallet->nAutoCombineBlockFrequency = pSettings.second;
    } catch (...) {
        fAutocombineException = true;
    }

    if (fAutocombineException) {
        try {
            pwallet->fCombineDust = pSettings.first;
            pwallet->nAutoCombineThreshold = pSettings.second;
        } catch (...) {
            return false;
        }
    }
    return true;
}

You get the idea. But I still think it's an ugly solution...and exceptions are expensive...

Edit: we both posted in parallel :-)

[CaveSpectre11]

@Mrs-X

You get the idea. But I still think it's an ugly solution...and exceptions are expensive...

Definitely agreed. Thankfully because autocombine doesn't work very well currently, there is likely very few that would be effected by this.

Would it make sense to self recover so it will only do it once? e.g.

    if (fAutocombineException) {
        try {
            pwallet->fCombineDust = pSettings.first;
            pwallet->nAutoCombineThreshold = pSettings.second;
            pwallet->nAutoCombineBlockFrequency = 15;  // interpret for new functionality
        } catch (...) {
            return false;
        }
	// convert to new format
        WriteAutoCombineSettings(pwallet->fCombineDust, 
                                 pwallet->nAutoCombineThreshold, 
                                 pwallet->nAutoCombineBlockFrequency);
    }

[Mrs-X]

Would it make sense to self recover so it will only do it once? e.g.

At this point wallet.dat is already opened for reading, so this wouldn't be a good idea.

If at all you could do this after it's closed, but I don't have a good feeling with that.
And, ReadKeyValue() is also called in during the wallet.dat recovery and I wouldn't touch that.

[CaveSpectre11]

@Mrs-X

Would it make sense to self recover so it will only do it once? e.g.

At this point wallet.dat is already opened for reading, so this wouldn't be a good idea.

If at all you could do this after it's closed, but I don't have a good feeling with that.
And, ReadKeyValue() is also called in during the wallet.dat recovery and I wouldn't touch that.

Ok; that understanding above is exactly why I asked :) I guess leaving it with a debug message in the log to suggest they re-establish their settings is the best we can do, rather than risk introducing any new issues.

I will likely resubmit a new PR with this fix, the nits, and one other improvement as well. This level of functionality solved my issues, but a few others still had problems (one with rewards coming to over 200 wallets, so they had to sweep the dust up into just a small number of dust piles per wallet), and another with a dev wallet that had sat untouched, receiving a 3% cut of every block reward.

So I re-purposed the "0" threshold to be something useful... to combine up to the maxSize.

I'll roll that in with all of the above discussions, and we can have another crack at it :)

[CaveSpectre11]

Just an update. I've had very little time with access to my linux box and development environment. My initial tests reproduced the problem but didn't successfully trigger the catch. I need to slice in another timeslot to dig into debugging what went wrong.

[CaveSpectre11]

Closing to provide a clean PR