[Wallet] Call AutocombineDust less often #497

Warrows · 2018-01-19T11:02:32Z

MultiSend and AutocombineDust are called after each new block is found. This is highly inefficient during initial sync. So this PR propose to wait for blockchain and MN sync to call this functions.
It also avoid AutocombineDust trying to iterate on UTXO > to its threshold.
Lastly it does not Autocombine in cases where the threshold is not reached and fees would be incurred.

Fuzzbawls

do we wait for IBD or do we wait for mnsync?

Fuzzbawls · 2018-01-20T11:00:49Z

src/main.cpp

Thoughts on having this based on masternodeSync.IsSynced() or IsInitialBlockDownload(). It is entirely possible to send a transaction before the mnsync is finished, and I agree that limiting this to one or the other is a net benefit...just want to discuss which would make more sense.

Actually I was going for IBD at first. But I think it's better to wait to be fully synced (and get your last incoming TXs) to fire up more TX. Else you are going to make more TX than necessary, incurring fees and blocks bloating.

The first thing that happens within MultiSend() and AutoCombineDust() is checking whether IsInitialBlockDownload() is true, and if so they will return false.

@presstab You are right, I should move it there. But I still think masternodeSync.IsSynced() is superior to IsInitialBlockDownload() in this case.

Warrows · 2018-01-24T10:22:49Z

@presstab So like you said, AutoCombineDust and MultiSend were testing the state before beginning. But actually they were testing IBD for the first and the tip of the chain time for the second. While AutoZeromint is checking MN sync. Let's have some consistency.
@Fuzzbawls I think the best option is MN sync. IBD will return true when a 144 minutes old (or younger) block is found. And the code used for multisend was 300 seconds (chainActive.Tip()->nTime < (GetAdjustedTime() - 300)) which is a little bit harder to read than masternodeSync.IsSynced().

Mrs-X · 2018-01-24T11:58:03Z

FYI, "AutoZeromint was checking MN sync" was used because AutoZeromint is/was very CPU intensive, the wallet would sometimes need several minutes to start. Waiting for MN sync needs normally a couple of minutes, so that's a good compromise.
I would do the same for AutoCombineDust (because it's blocking) but not for MultiSend.
If the wallet runs for several minutes without doing MultiSend users will probably report it as broken.

Warrows · 2018-01-24T14:12:56Z

I don't see why Autocombine should be any different from Multisend. It's basically the same process, they just block a bit when used for the first time and a lot of TX are eligible. Autocombine should not be blocking more than Multisend once #496 is fixed.
To be fair, Multisend was initially waiting for IBD and was changed in #477 to its current state. And this was done in the same spirit I am advocating MN sync over IBD: avoid generating a lot of TX (and fees) where one is enough. MN sync is the best way to ensure we have finished getting the latest blocks from the network when starting the wallet.

Mrs-X · 2018-01-24T15:23:56Z

Both AutoCombine and MultiSend do always create the same number of transactions, no matter when they are called (AutoCombine one for each address (which needs combining), MultiSend for each output which is a reward).

But anyway, I'm also fine with waiting for MN sync, even when that sometimes needs a long time to finish.

If @Fuzzbawls is fine with it I'll test it.

Warrows · 2018-01-24T15:34:36Z

I am not entirely sure about multisend. But autocombine was not always creating the same number of transactions. If you have several TX they are combined. And when an other blocks shows up, they are combined again and so on. So if you are 144 blocks behind, you can have a TX when getting officialy out of IBD and others when really catching up. I grant you the risk is higher on testnet than mainnet. And actually, this PR should solve the problem altogether by not combining below threshold. So maybe what is currently done for multisend is ok.

Fuzzbawls · 2018-01-25T00:34:38Z

for the sake of consistency across all post-synced, transaction-creating automation functions; I can agree to using mnsync.

Mrs-X · 2018-01-26T19:31:44Z

Okay.
@Warrows : want to add some last words/changes or is it ready to merge?

Warrows · 2018-01-26T19:46:15Z

@Mrs-X if you are about to merge I am simply going to squash it.

Autocombine to the threshold and not above Autocombine now start later ->wait for 5mn old block or younger

Mrs-X · 2018-01-26T20:15:09Z

I didn't yet test the MultiSend part, I'll report back in an hour or so.

Mrs-X · 2018-01-26T21:56:16Z

Works as expected, therefore ACK from my side, so you can squash it.

I'm still not 100% sold on the way Multisend works, though.
Sync was about 1hour behind when I started the wallet, so none of the rewards I got in that time were sent and I had to do it by hand. As a masternode owner I would expect that all rewards from the time I closed the wallet would be send when I start the wallet again.
Right now it only makes sense for users who let it run all the time.

Warrows · 2018-01-27T10:07:36Z

I squashed right away.
I don't know when you pulled but if you have changes on multisend you are probably not up to date with 9309698 or the squashed commit 6e39d70. Actually I decided not to use MN sync when I realized the problems it solves are testnet specific. And as you said the change would probably confuse some users.
So there are no changes on multisend behavior in this PR.

Nonetheless, I agree with you, multisend should scan all rewards. But the way it's currently working, it sends only when called on the exact block where maturity of a reward is attained:
if (out.tx->GetDepthInMainChain() != Params().COINBASE_MATURITY() + 1) continue;
Maybe we should review the way multisend is working, but I'd say it's out of the scope of this PR.

Mrs-X · 2018-01-27T10:31:50Z

I agree, we merge this one and have a closer look at the general Multisend approach somewhere in the future.

6e39d70 [Performance] AutocombineDust is called less often (warrows) Tree-SHA512: d336609341222ac1c0ee6b9ab57ede0785ce941450e55408b1813bc44e15a8c6149eff47055516296fc15051ea7abe51375bb4911275e6093cc1cad67ca2c53b

Fuzzbawls added enhancement Wallet labels Jan 19, 2018

Fuzzbawls changed the title ~~[Performance] MultiSend and AutocombineDust are called less often~~ [Wallet] MultiSend and AutocombineDust are called less often Jan 19, 2018

Fuzzbawls reviewed Jan 20, 2018

View reviewed changes

Warrows changed the title ~~[Wallet] MultiSend and AutocombineDust are called less often~~ [Wallet] Call MultiSend and AutocombineDust less often Jan 24, 2018

Warrows force-pushed the fix_autocombine branch from ab7d8c8 to 9309698 Compare January 24, 2018 15:39

[Performance] AutocombineDust is called less often

6e39d70

Autocombine to the threshold and not above Autocombine now start later ->wait for 5mn old block or younger

Warrows force-pushed the fix_autocombine branch from 9309698 to 6e39d70 Compare January 26, 2018 20:01

Warrows changed the title ~~[Wallet] Call MultiSend and AutocombineDust less often~~ [Wallet] Call AutocombineDust less often Jan 27, 2018

Mrs-X merged commit 6e39d70 into PIVX-Project:master Jan 27, 2018

Warrows deleted the fix_autocombine branch January 27, 2018 12:47

Fuzzbawls added this to the 3.1.0 milestone Jan 27, 2018

Warrows mentioned this pull request Feb 5, 2018

[Wallet] Combine fees when possible and fix autocombine insufficient funds #518

Merged

Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Feb 24, 2018

Warrows mentioned this pull request Mar 4, 2018

[Docs] Added release notes for autocombine and proxy GUI. #554

Merged

Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jun 19, 2018

akshaynexus mentioned this pull request May 2, 2019

[Wallet/Performance] Call AutocombineDust less often AirWireOfficial/wire-core#13

Merged

[Wallet] Call AutocombineDust less often #497

[Wallet] Call AutocombineDust less often #497

Uh oh!

Conversation

Warrows commented Jan 19, 2018

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

Warrows Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

presstab Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

Warrows Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

Warrows commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mrs-X commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Warrows commented Jan 24, 2018

Uh oh!

Mrs-X commented Jan 24, 2018

Uh oh!

Warrows commented Jan 24, 2018

Uh oh!

Fuzzbawls commented Jan 25, 2018

Uh oh!

Mrs-X commented Jan 26, 2018

Uh oh!

Warrows commented Jan 26, 2018

Uh oh!

Mrs-X commented Jan 26, 2018

Uh oh!

Mrs-X commented Jan 26, 2018

Uh oh!

Warrows commented Jan 27, 2018

Uh oh!

Mrs-X commented Jan 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Warrows Jan 20, 2018 •

edited

Loading

Warrows commented Jan 24, 2018 •

edited

Loading

Mrs-X commented Jan 24, 2018 •

edited

Loading