Cache vout IsMine() value on `CWallet::AvailableCoins()`

joaopaulofonseca commented at 1:59 pm on March 7, 2017: contributor

This PR tries to improve the performance of the listunspent, fundrawtransaction, sendtoaddress and other RPC calls by improving CWallet::AvailableCoins() iteration through the wallet transactions.

E.g. If a given address is an outpoint of N transactions, I guess there’s no point to check N times if the address IsMine().

This PR supersedes #7823 although it’s a different approach. In that PR, caching accurately those values was a little tricky because of all the scenarios that could make the cache invalidated (conflicts, double-spends, reorganizations, abandoned transactions, etc, …).

Still, with this approach I got performance gains up to 90% on CWallet::AvailableCoins() for large wallets.

laanwj added the label Wallet on Mar 7, 2017

in src/wallet/wallet.cpp: in dca8d8926e outdated

2090+                    mine = im->second;
2091+                } else {
2092+                    mine = mapOutputIsMine[pcoin->tx->vout[i].scriptPubKey] = IsMine(pcoin->tx->vout[i]);
2093+                }
2094+
2095+                if (mine != ISMINE_NO && !(IsSpent(wtxid, i)) &&

pedrobranco commented at 6:00 pm on March 7, 2017:

I there’s an ACK from the community, I would love to see this giant if break down with several early continues (to facilitate the reading), as applied here.

ryanofsky commented at 7:15 pm on March 8, 2017:

This would seem like a good idea, especially if the continues were documented with new comments. Not really on-topic for this PR, however.

NicolasDorier commented at 3:11 am on March 8, 2017: contributor

Please check #9167, morcos is working on it

joaopaulofonseca commented at 11:26 am on March 8, 2017: contributor

@pedrobranco yes, that if is a little mess. I’ll also take a look at it. @NicolasDorier If I understood the @morcos PR correctly, it’s related to the isminetype of the transaction inputs and not the outputs, which we need here to find our utxos.

NicolasDorier commented at 11:51 am on March 8, 2017: contributor

my bad I read too fast.

NicolasDorier commented at 12:00 pm on March 8, 2017: contributor

Why not having the cache scoped to the WalletTransaction type ? Instead of

0IsMine(pcoin->tx->vout[i])

we would have

0pcoin->IsMine(i)

where WalletTransaction::IsMine would cache the result at the WalletTransaction level.

this would make it more readable,
this cache could be populated everywhere in the code when IsMine is called,
the cache would span accross different calls to AvailableCoins, making AvailableCoins even faster.

in src/wallet/wallet.cpp: in dca8d8926e outdated

2088+                std::map<CScript, isminetype>::const_iterator im = mapOutputIsMine.find(pcoin->tx->vout[i].scriptPubKey);
2089+                if (im != mapOutputIsMine.end()) {
2090+                    mine = im->second;
2091+                } else {
2092+                    mine = mapOutputIsMine[pcoin->tx->vout[i].scriptPubKey] = IsMine(pcoin->tx->vout[i]);
2093+                }

ryanofsky commented at 7:13 pm on March 8, 2017:

Can simplify this as:

0+                auto inserted = mapOutputIsMine.emplace(pcoin->tx->vout[i].scriptPubKey, ISMINE_NO);
1+                if (inserted.second) {
2+                   inserted.first->second = IsMine(pcoin->tx->vout[i]);
3+                }
4+                isminetype mine = inserted.first->second;

ryanofsky commented at 7:20 pm on March 8, 2017: member

utACK dca8d8926e655ba6639e748bcd32c560a5539b88

Code seems fine, but I don’t know very much about the performance implications of this change, and would leave it for others to discuss. NicolasDorier’s suggestion seems like a reasonable alternative, too.

joaopaulofonseca commented at 12:13 pm on March 10, 2017: contributor

@NicolasDorier that’s a good suggestion. Actually a while ago I was trying to create a cache to store these values, but at the wallet level. But couldn’t manage to successfully achieve a solution with both reliability and speed. But I will look into your approach. @ryanofsky Ok, I like it. I’ll simplify that piece of code.

Cache vout IsMine() value on `CWallet::AvailableCoins()` 46cbc0afa0

joaopaulofonseca force-pushed on Mar 13, 2017

RHavar commented at 7:05 pm on June 1, 2017: contributor

I’ve tested 46cbc0afa0060f0065dd48f8e7e20fe413f5b10c

Before median time of listunspent after a few runs on my wallet was 14.4 seconds, now it’s 12.8s.

So it seems faster, but not earth shatteringly so. For reference my wallet has about ~520k transactions in it, where of them ~168k are mine (outgoing transactions).

–

To me it seems like @NicolasDorier’s suggestion is the way to go, I suspect the reuse of the cache is going to help a lot.

laanwj commented at 3:28 pm on June 5, 2017: member

@RHavar thanks for testing!

joaopaulofonseca commented at 3:48 pm on June 5, 2017: contributor

@RHavar thanks a lot for your feedback! The current implementation of listunspent is a little tricky to optimize without a larger refactor.

I’ve tried to optimize each of the two heavier calls within listunspent: IsMine() and IsSpent(). But without optimizing both, they will only fix different use cases separately, which will behave very differently for large wallet vs large amount of unspents.

I haven’t had much time to work on this lately, but it’s a problem I’d like to come up with a clean solution in the near future, since it’s an issue affecting almost everyone with large wallets.

ryanofsky commented at 7:31 pm on July 25, 2017: member

utACK 46cbc0afa0060f0065dd48f8e7e20fe413f5b10c. Code is slightly simplified since last review, and change looks good assuming performance improvements warrant it.

NicolasDorier commented at 2:37 am on July 26, 2017: contributor

utACK 46cbc0a, it is simple enough. Can keep wallet level caching for later PR.

ryanofsky commented at 5:21 pm on October 12, 2017: member

Change is simple and has multiple acks, but needs to be rebased.

MarcoFalke commented at 6:30 pm on November 10, 2017: member

Not rebased in 30 days.

This is now up for grabs.

MarcoFalke closed this on Nov 10, 2017

joaopaulofonseca commented at 2:33 pm on November 12, 2017: contributor

Sorry guys, haven’t had much time to work on this lately.. Although this improves the speed, the last tests I did showed there are still some edge cases where the unspent count differs which means that the cache is not accurate.. I’ll try to pick this up in the future, and reopen if needed.

ryanofsky commented at 10:34 pm on January 8, 2018: member

Could add up for grabs label

MarcoFalke added the label Up for grabs on Jan 11, 2018

giaki3003 referenced this in commit 96c79c9835 on May 2, 2019

DrahtBot locked this on Sep 8, 2021

Cache vout IsMine() value on CWallet::AvailableCoins() #9939

Cache vout IsMine() value on `CWallet::AvailableCoins()` #9939