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

Correct issues with dust totaling near threshold #849

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@CaveSpectre11
Copy link

CaveSpectre11 commented Mar 27, 2019

A problem was introduced with #518 when a utxo smaller than 10% of the threshold takes the total combined coins above the threshold, or when the total amount of dust is less than 10% above the threshold.

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 the utxo 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 widdle the two combined transactions fall below the threshold.

If there is another utxo to add in order to get us far enough above the threshold that the 10% reduction is still above the threshold; then we're now good when accounting for the 10% in our check in the for loop. However there is still one other case that slips through. 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.

Correct issues with dust totaling near threshold
A problem was introduced with #518 when a utxo smaller than 10% of the threshold takes the total combined coins above the threshold, or when the total amount of dust is less than 10% above the threshold.

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 the utxo 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 widdle the two combined transactions fall below the threshold.

If there is another utxo to add in order to get us far enough above the threshold that the 10% reduction is still above the threshold; then we're now good when accounting for the 10% in our check in the for loop.  However there is still one other case that slips through.  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.
@Warrows
Copy link
Collaborator

Warrows left a comment

Nice find and looks like a good fix. I'll test it later. utACK for now. With a small coding style nit.

// Combine to the threshold and not way above
if (nTotalRewardsValue > nAutoCombineThreshold * COIN)
// Combine until our total is enough above the threshold to remain above after adjustments
if ((nTotalRewardsValue-nTotalRewardsValue/10) > nAutoCombineThreshold * COIN)

This comment has been minimized.

Copy link
@Warrows

Warrows Mar 27, 2019

Collaborator

nit: I think we usually put spaces around '-' and '/' operators so if ((nTotalRewardsValue - nTotalRewardsValue / 10) > nAutoCombineThreshold * COIN) would be preferred here.

This comment has been minimized.

Copy link
@CaveSpectre11

CaveSpectre11 Mar 27, 2019

Author

Understood and apologies. I'm usually good about determining the style; but I made this change through the github browser... as I'm rolling it up from my original fix in a distant child of PivX... So I'm new here and didn't get a chance to establish a pivx dev area for myself.

Should I modify and resubmit, or something for when someone is next in the module?

This comment has been minimized.

Copy link
@Warrows

Warrows Mar 27, 2019

Collaborator

No problem, I'm being picky about it. If you can amend your commit and force push it, it would be the best way to go.

This comment has been minimized.

Copy link
@CaveSpectre11

CaveSpectre11 Mar 27, 2019

Author

No worries; I'm a stickler in my IRL job when people don't stick to the established coding standard; so completely understood. Looks like I was able to recommit the file (I think)

@Warrows

This comment has been minimized.

Copy link
Collaborator

Warrows commented Apr 14, 2019

Sorry we merged a PR moving wallet files before this one. If you could rebase it, it would be awesome.

@CaveSpectre11

This comment has been minimized.

Copy link
Author

CaveSpectre11 commented Apr 16, 2019

I have further improvements to autocombine rewards; so I will repackage it together and submit again on one PR.

@CaveSpectre11

This comment has been minimized.

Copy link
Author

CaveSpectre11 commented Apr 17, 2019

Close, superseded with #867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.