Connecttrace fewer blocks

narula commented at 8:04 pm on June 29, 2017: contributor

During a large reorg, the number of blocks kept in memory could grow to be very large. This change makes it so that only the last 10 blocks are kept in memory, and if necessary, older blocks are re-read from disk.

Also introduce a parameter to FlushStateToDisk to ensure that we do not try to prune during the life of a ConnectTrace, because we might need to re-read the block from disk.

Addresses https://github.com/bitcoin/bitcoin/issues/9027

Only keep a few (10) ConnectTrace blocks in memory during a reorg.

During a large reorg, the number of blocks kept in memory could grow
to be very large.  This change makes it so that only the last 10
blocks are kept in memory, and if necessary, older blocks are re-read
from disk.

c0e4575aa6

Don't prune blocks while using a ConnectTrace.

Introduce a parameter to FlushStateToDisk to ensure that we do not try
to prune during the life of a ConnectTrace, because we might need to
re-read the block from disk.

b7077bd98e

jonasschnelli commented at 8:13 pm on June 29, 2017: contributor

Concept ACK (not looked to closely at the code yet). 10 blocks seems to be little… maybe it should be configurable (most memory consuming processes are configurable right now).

jonasschnelli added the label Validation on Jun 29, 2017

gmaxwell commented at 8:22 pm on June 29, 2017: contributor

why would ten be too little? If it made the code simpler I would have expected it to just do one, reading them isn’t free but it’s not that expensive– shouldn’t be the limiting factor in reorg performance.

jonasschnelli commented at 8:40 pm on June 29, 2017: contributor

why would ten be too little?

I kinda followed the assumption that some users want to reorg as fast as possible (I’s probably often a tiny difference) regardless of the depth of the reorganisation. I guess those users do not care about using 200MB+ for tracing the blocks in mem during a reorg. But it’s a wild assumptions and maybe I’m wrong.

TheBlueMatt commented at 8:56 pm on June 29, 2017: member

Well I’m not sure how many users are really focused on how long a 20 block reorg will take…should be sufficiently rare that a user spending a lot of time thinking about it is probably less than useful.

gmaxwell commented at 9:09 pm on June 29, 2017: contributor

@jonasschnelli reorgs are slow but for reasons that have nothing to do with reading blocks from the disk. it can easily take many seconds to reorg 20 blocks, reading them will take a few milliseconds. Making reorgs faster is great but I think this is the wrong place to optimize for that.

in src/validation.cpp:189 in b7077bd98e

185@@ -186,7 +186,7 @@ enum FlushStateMode {
186 };
187 
188 // See definition for documentation
189-static bool FlushStateToDisk(const CChainParams& chainParams, CValidationState &state, FlushStateMode mode, int nManualPruneHeight=0);
190+static bool FlushStateToDisk(const CChainParams& chainParams, CValidationState &state, FlushStateMode mode, int nManualPruneHeight, bool prune);

sipa commented at 11:14 pm on June 29, 2017:

It seems the prune flag is implied by mode != FLUSH_STATE_IS_NEEDED.

narula commented at 7:57 pm on July 3, 2017:

Yes, you’re right. But is it reasonable to rely on that for prune’s semantics?

JeremyRubin commented at 8:27 pm on July 27, 2017:

Why not preserve the default args?

in src/validation.cpp:1758 in b7077bd98e

1765-        if (!setFilesToPrune.empty()) {
1766-            fFlushForPrune = true;
1767-            if (!fHavePruned) {
1768-                pblocktree->WriteFlag("prunedblockfiles", true);
1769-                fHavePruned = true;
1770+    if (prune) {

JeremyRubin commented at 8:36 pm on July 27, 2017:

could collapse this with branch below?

in src/validation.cpp:2039 in b7077bd98e

2031@@ -2029,6 +2032,12 @@ class ConnectTrace {
2032         assert(pblock);
2033         blocksConnected.back().pindex = pindex;
2034         blocksConnected.back().pblock = std::move(pblock);
2035+        if (++num_connected >= MAX_CONNECT_TRACE_BLOCKS_IN_MEMORY) {
2036+            // Clear out a block
2037+            int idx = blocksConnected.size() - num_connected;
2038+            assert(idx >= 0);
2039+            blocksConnected.at(idx).pblock = NULL;

JeremyRubin commented at 8:40 pm on July 27, 2017:

better to just call reset() explicitly, should the ptr ever be made non-smart.

in src/validation.cpp:2035 in b7077bd98e

2031@@ -2029,6 +2032,12 @@ class ConnectTrace {
2032         assert(pblock);
2033         blocksConnected.back().pindex = pindex;
2034         blocksConnected.back().pblock = std::move(pblock);
2035+        if (++num_connected >= MAX_CONNECT_TRACE_BLOCKS_IN_MEMORY) {

JeremyRubin commented at 7:12 am on July 28, 2017:

slight preference for

0++num_connected;
1if (num_connected >= MAX_CONNECT_TRACE_BLOCKS_IN_MEMORY)

in src/validation.cpp:2349 in b7077bd98e

2344@@ -2336,8 +2345,18 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2345             fInitialDownload = IsInitialBlockDownload();
2346 
2347             for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2348-                assert(trace.pblock && trace.pindex);
2349-                GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs);
2350+                assert(trace.pindex);
2351+                std::shared_ptr<const CBlock> pthisBlock;

JeremyRubin commented at 7:15 am on July 28, 2017:

probably a bit cleaner to just do:

0std::shared_ptr<const CBlock> pthisBlock(trace.pblock);
1if (!pThisBlock) {
2
3}

in src/validation.cpp:2351 in b7077bd98e

2344@@ -2336,8 +2345,18 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2345             fInitialDownload = IsInitialBlockDownload();
2346 
2347             for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2348-                assert(trace.pblock && trace.pindex);
2349-                GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs);
2350+                assert(trace.pindex);
2351+                std::shared_ptr<const CBlock> pthisBlock;
2352+                if (trace.pblock == NULL) {
2353+                    std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();

JeremyRubin commented at 7:16 am on July 28, 2017:

probably no need to make pblockNew? Can just assign directly with make_shared to pthisBlock.

JeremyRubin commented at 7:25 am on July 28, 2017: contributor

Generally, looks ok to me, a couple style nits.

I think the one thing that I would want to consider is that counting the number of blocks is not an exact match for the amount of memory used. Maybe you can count blocks based on their memory weight. For example, if we were to ever add some really big data structure in memory when we load a block this code would break, but a limit in MB would just be more aggressive in evicting from memory.

I also think that it might be nice if you can maybe increase the batching or do an async read. Because now it seems that the disk seeks are now in the hot path, whereas it might be possible to prefetch them and never wait. Then again, this case (> 10 reorg) is rare enough it’s probably premature to care that much about latency here.

fanquake added the label Up for grabs on May 17, 2018

fanquake closed this on May 17, 2018

DrahtBot locked this on Sep 8, 2021

Connecttrace fewer blocks #10708