People are already complaining about memory usage of the wallet. How much will this make it worse?

I would guess maybe 40 bytes per transaction. I am running some tests to compare.

The current code is very poor for zapwallettxes because it will re-sort the entire wallet transactions, every time it adds a new one.

With a wallet containing 10184 transactions, I did not observe any measurable memory overhead to the cache.

 SIZE    VSZ    SZ
329352 347192 86798 d78a880
320552 338396 84599 d78a880+5873cc8

Nice!
Adding a wtx to the wallet could also be faster (especially big wallets) because AddToWallet() does also call OrderedTxItems() (which reads in all the whole transaction history).

utACK.

concept ACK, light utACK

Also, I feel like any memory overhead required would already exist instantaneously at the call site anyway, this just persists the requirement for the lifetime of the application instead?

edit: To clarify, this doesn't really require any extra memory, except in that it is now persistent across the applications lifetime, not just when needed.

Needs rebase

@luke-jr rebase please

Rebased

Increased memory from 1.5GB on master to 1.8GB
Improved zapwallet time to 5 minutes from something approximating infinity

$ ./src/bitcoin-cli -testnet getwalletinfo
{
"walletversion": 60000,
"balance": 1937898.88787003,
"unconfirmed_balance": 0.00000000,
"immature_balance": 1275.53301591,
"txcount": 776267,
"keypoololdest": 1446285387,
"keypoolsize": 999999,
"paytxfee": 0.00000000
}

@pstratem Pull request 6850 will improve zapwallet / rescan time similarly with no memory increase. However, this pull request will be good for AddToWallets when near the tip, where there is intermittent CPU spiking when the indexes are recreated.

IMO, this pull request is needed even with the memory increase, but 6850 is less controversial.

Rescan on wallet containing 14WCrzmqVCD7Thf79vutWTB6y1hUc8pP19 (maybe about 53k transactions) went from 23 minutes to 10 minutes.

oh my numbers there were for the whole rescan, if I limit the benchmark to just the chunk of the blockchain that have transactions... it goes from 843 seconds to 35 seconds from 63/sec to 1541/sec; or 24x faster.

On a wallet with 410k transactions in it, this repeatably decreases memory usage by ~5MB from 822MB to 816MB. I think it's fair to say that any increase is negligible.

@gmaxwell I would expect any memory increase to be within "measurement error" tolerances, i.e. not significant. This is essentially just storing another index (or two?). It's great that your measurements seem to agree.

And the previous code which ultimately recreated the indexes on each AddToWallet is simply bad.

ACK.