A suggestion from IRC: allow the chainstate cache to use unused mempool memory.
If the mempool is not completely full, treat the difference between the maximum size and the actual usage as available for the coin cache.
This also changes the early flush trigger from (usage > 0.9 * space) to (usage > 0.9 * space && usage > space - 100MB). This means we’re not permanently leaving 10% of the space unused when the space is large.
At the risk of it being blatantly obvious, this would especially allow nodes with default settings during IBD to sync faster, yes?
concept ACK
At the risk of it being blatantly obvious, this would especially allow nodes with default settings during IBD to sync faster, yes?
Indeed!
At the risk of appearing stupid, how does this help nodes with default settings sync faster during IBD?
…coincache helps to validate blocks..?
@sipa do you have any concern that this gives outside peers some control over when your coinsviewcache is flushed? That seems not ideal to me, at least until we are smarter about flushing it.
I had previously imagined doing this by setting the mempool limit low initially and then increasing it to the desired setting after IBD was finished. But I’m not sure I succeeded in having the code architected to make that possible.
1281+ nCoinCachePlusMempoolUsage = nMempoolSizeMax + nTotalCache; // the rest goes to in-memory cache
1282 LogPrintf("Cache configuration:\n");
1283 LogPrintf("* Using %.1fMiB for block index database\n", nBlockTreeDBCache * (1.0 / 1024 / 1024));
1284 LogPrintf("* Using %.1fMiB for chain state database\n", nCoinDBCache * (1.0 / 1024 / 1024));
1285- LogPrintf("* Using %.1fMiB for in-memory UTXO set\n", nCoinCacheUsage * (1.0 / 1024 / 1024));
1286+ LogPrintf("* Using %.1fMiB for UTXO set (shared with mempool)\n", nCoinCachePlusMempoolUsage * (1.0 / 1024 / 1024));
Seeing a roughly 30% decrease in time to reindex to 270'000 with this change.
08:04:43 Cache configuration: 08:04:43 * Using 2.0MiB for block index database 08:04:43 * Using 8.0MiB for chain state database 08:04:43 * Using 290.0MiB for in-memory UTXO set
08:04:45 UpdateTip: height=0 cache=0.0MiB(0tx) 08:08:09 UpdateTip: height=200000 cache=172.9MiB(553629tx) 08:12:52 UpdateTip: height=230000 cache=274.9MiB(668943tx) 08:19:08 UpdateTip: height=260488 cache=239.4MiB(318281tx) 08:19:08 UpdateTip: height=260489 cache=239.4MiB(318525tx) 08:22:11 UpdateTip: height=270000 cache=31.7MiB(8943tx)
07:50:16 Cache configuration: 07:50:16 * Using 2.0MiB for block index database 07:50:16 * Using 8.0MiB for chain state database 07:50:16 * Using 290.0MiB for UTXO set (plus up to 286.1MiB of unused mempool space) … 07:50:18 UpdateTip: height=0 cache=0.0MiB(0tx) 07:53:33 UpdateTip: height=200000 cache=356.1MiB(1336689tx) 07:56:58 UpdateTip: height=230000 cache=575.5MiB(1900937tx) 08:01:02 UpdateTip: height=260488 cache=576.1MiB(1324684tx) 08:01:11 UpdateTip: height=260489 cache=16.0MiB(0tx) 08:02:32 UpdateTip: height=270000 cache=486.6MiB(972452tx)
2552@@ -2553,6 +2553,7 @@ enum FlushStateMode {
2553 * or always and in all cases if we're in prune mode and are deleting files.
2554 */
2555 bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
2556+ size_t cacheSize = mempool.DynamicMemoryUsage();
I wonder if it would be better to call LimitMempoolSize right before this? During a reorg the mempool size isn’t limited until the end, so theoretically FlushStateToDisk could get called and have its effective cache size reduced below the expected minimum. @sdaftuar Is there any harm in calling an extra LimitMempoolSize in the middle of a reorg other than possible inefficiencies of what you’re evicting?
EDIT: Shoot this isn’t exactly what I was imagining. I was thinking it would only limit the mempool if it was going to flush anyway, but this suggestion will limit the mempool every time FSTD is called which defeats the whole point of not limiting inside a reorg.
utACK modulo suggestion about calling LimitMempoolSize in FSTD.
However if we decide there are downsides to that suggestion, I’m fine with leaving it out.
EDIT: maybe 0dedace? Though I’m not sure it’s worth it..
2583@@ -2583,11 +2584,16 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
2584 if (nLastSetChain == 0) {
2585 nLastSetChain = nNow;
2586 }
2587- size_t cacheSize = pcoinsTip->DynamicMemoryUsage();
2588+ cacheSize += pcoinsTip->DynamicMemoryUsage();
2589 // The cache is large and close to the limit, but we have time now (not in the middle of a block processing).
2590- bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCacheUsage;
2591+ bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCachePlusMempoolUsage;
@sipa sorry, one more issue. this line would be pretty broken if someone sets a large maxmempool. i was thinking maybe we should change it to 20.0/19 so it doesn’t make the base case behavior a smaller effective cache size, but then realized if someone sets say a 2GB mempool and doesn’t make a larger dbcache, then they could be flushing all the time.
Perhaps this problem and the other one could be avoided by just calculating any extra available space max(0,(maxmempool - usage)) and comparing only the pcoinstip usage to (dbcache limit + any extra available space).
@morcos Not sure I’m a fan of 0dedace. I believe this code will work correctly for now, but only because we don’t seem to call FlushStateToDisk anywhere the mempool might be in an inconsistent state. I don’t think it’s safe to call LimitMempoolSize() when the mempool is in an inconsistent state, because CTxMemPool::TrimToSize() and CTxMemPool::Expire rely on CTxMemPool::CalculateDescendants() to do the right thing, and that function assumes that setMemPoolChildren is correct for all entries. If CalculateDescendants() doesn’t catch everything it should, then we could wind up with orphans in the mempool.
So in particular, during a reorg, we call AcceptToMemoryPool() for all the transactions in a disconnected block. If someone were to add a FlushStateToDisk() call inside ATMP (which I think seems harmless on the face of it), then that would introduce a bug that could cause orphans in the mempool after a reorg.
So at the very least, we would need to document that FlushStateToDisk() has a new requirement, that the mempool is in a consistent state when invoked. But as this condition is difficult to assert for and test, I think my preference would be to not impose this requirement, just to reduce the chance we introduce a bug in the future.
But also, note that #9208 should solve the problem of the mempool growing beyond its configured limit during a reorg…
Looks good @sipa , thanks
Seems like an actual lock inversion, I think you fixed it previously by moving the mempool.DynamicMemoryUsage() to the top of FlustStateToDisk, which makes sense to me.
However I don’t understand why CWallet::ReacceptWalletTransactions is locking mempool.cs anyway, so maybe that should also be removed.
I guess you lost the advantage of the extra space in VerifyDB, but that seems no big loss to me.
If the mempool is not completely full, treat the difference between
the maximum size and the actual usage as available for the coin cache.
This also changes the early flush trigger from (usage > 0.9 * space)
to (usage > 0.9 * space && usage > space - 100MB). This means we're not
permanently leaving 10% of the space unused when the space is large.
#8610 on top of master (2a524b8e8fe69ef487fd8ea1b4f7a03f473ed201) with -reindex-chainstate -dbcache=2048:
02017-01-03 22:39:59 Cache configuration:
12017-01-03 22:39:59 * Using 2.0MiB for block index database
22017-01-03 22:39:59 * Using 8.0MiB for chain state database
32017-01-03 22:39:59 * Using 2038.0MiB for in-memory UTXO set (plus up to 286.1MiB of unused mempool space)
42017-01-03 22:43:04 height=200000 cache=356.1MiB(1336689tx)
52017-01-03 22:46:20 height=230000 cache=575.5MiB(1900937tx)
62017-01-03 22:51:26 height=270000 cache=915.5MiB(2499199tx)
72017-01-03 22:53:26 height=280000 cache=1050.5MiB(2816546tx)
Master (2a524b8e8fe69ef487fd8ea1b4f7a03f473ed201) -reindex-chainstate -dbcache=2048:
02017-01-03 23:01:24 Cache configuration:
12017-01-03 23:01:24 * Using 2.0MiB for block index database
22017-01-03 23:01:24 * Using 8.0MiB for chain state database
32017-01-03 23:01:24 * Using 2038.0MiB for in-memory UTXO set
42017-01-03 23:04:29 height=200000 cache=356.1MiB(1336689tx)
52017-01-03 23:07:46 height=230000 cache=575.5MiB(1900937tx)
62017-01-03 23:12:48 height=270000 cache=915.5MiB(2499199tx)
72017-01-03 23:14:45 height=280000 cache=1050.5MiB(2816546tx)
Master (2a524b8e8fe69ef487fd8ea1b4f7a03f473ed201) -reindex-chainstate -dbcache=300:
02017-01-03 23:24:34 Cache configuration:
12017-01-03 23:24:34 * Using 2.0MiB for block index database
22017-01-03 23:24:34 * Using 8.0MiB for chain state database
32017-01-03 23:24:34 * Using 290.0MiB for in-memory UTXO set
42017-01-03 23:27:55 height=200000 cache=172.9MiB(553629tx)
52017-01-03 23:32:32 height=230000 cache=274.9MiB(668943tx)
62017-01-03 23:36:41 height=250000 cache=128.6MiB(113010tx)
72017-01-03 23:42:36 height=270000 cache=31.7MiB(8943tx)
#8610 on top of master (2a524b8e8fe69ef487fd8ea1b4f7a03f473ed201) -reindex-chainstate -dbcache=300
02017-01-03 23:47:30 Cache configuration:
12017-01-03 23:47:30 * Using 2.0MiB for block index database
22017-01-03 23:47:30 * Using 8.0MiB for chain state database
32017-01-03 23:47:30 * Using 290.0MiB for in-memory UTXO set (plus up to 286.1MiB of unused mempool space)
42017-01-03 23:50:37 height=200000 cache=356.1MiB(1336689tx)
52017-01-03 23:53:58 height=230000 cache=575.5MiB(1900937tx)
62017-01-03 23:56:50 height=250000 cache=504.4MiB(1124858tx)
72017-01-03 23:59:30 height=270000 cache=486.6MiB(972452tx)
82017-01-04 00:02:10 height=280000 cache=75.2MiB(48075tx)