refactor: Make mapBlocksUnknownParent local, and rename it #25571

LarryRuane · 2022-07-08T05:01:57Z

This PR is a second attempt at #19594. This PR has two motivations:

Improve code hygiene by eliminating a global variable, mapBlocksUnknownParent
Fix fuzz test OOM when running too long (see #19594 comment)

A minor added advantage is to release mapBlocksUnknownParent memory when the reindexing phase is done. The current situation is somewhat similar to a memory leak because this map exists unused for the remaining lifetime of the process. It's true that this map should be empty of data elements after use, but its internal metadata (indexing structures, etc.) can have non-trivial size because there can be many thousands of simultaneous elements in this map.

This PR helps our efforts to reduce the use of global variables. This variable isn't just global, it's hidden inside a function (it looks like a local variable but has the static attribute).

This global variable exists because the -reindex processing code calls LoadExternalBlockFile() multiple times (once for each block file), but that function must preserve some state between calls (the mapBlocksUnknownParent map). This PR fixes this by allocating this map as a local variable in the caller's scope and passing it in on each call. When reindexing completes, the map goes out of scope and is deallocated.

I tested this manually by reindexing on mainnet and signet. Also, the existing feature_reindex.py functional test passes.

DrahtBot · 2022-07-08T09:34:01Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25499 (Use steady clock for all millis bench logging by MarcoFalke)
#16981 (Improve runtime performance of --reindex by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa

utACK 3bd8416

src/validation.cpp

shaavan

Concept ACK

This PR changes the scope of mapBlocksUnknown parent to the local variable of the caller function of LoadExternalBlockFile.
I agree with converting the variable from a static global type to a local type.

The current situation is somewhat similar to a memory leak because this map exists unused for the remaining lifetime of the process. It's true that this map should be empty of data elements after use, but its internal metadata (indexing structures, etc.) can have non-trivial size because there can be many thousands of simultaneous elements in this map.

I agree with the reasoning, and this seems to cause an OOM error during fuzz testing.
I verified that the code refactoring was concise and clear. The code changes are purely refactoring modifications and do not introduce and change in code functionality.
The added documentation for the LoadExternalBlockFile, clearly explains the usage of the function and the reasoning for the used type of the arguments.
I was able to compile and run this PR on Ubuntu 22.04 successfully. And I was able to reindex on the signet network using the following command successfully

./src/bitcoind -signet -reindex

Since, according to the guidelines, we need to include all the headers directly used in a file, I would suggest two instances of such inclusion.

#include <map> in src/node/blockstorage.cpp and src/test/fuzz/load_external_block_file.cpp file.
#include <cassert> in src/validation.cpp file.

theStack · 2022-07-08T15:25:28Z

Concept ACK

LarryRuane · 2022-07-08T16:30:11Z

Force-pushed to address review comments (thanks @sipa and @shaavan!)

theStack

Code-review ACK d6bbb65 (also tested -reindexing on signet)

Nice improvement! Probably it's worth it to investigate if the code-base contains more instances of static variables inside functions that don't need to exist for the whole lifetime of the process and can thus be eliminiated by using a limited scope instead.

src/validation.h

LarryRuane · 2022-07-09T16:32:47Z

Force-pushed to address review comment (change variable name dps back to the original name dbp), thanks @theStack.

theStack

re-ACK e963615

shaavan

reACK e963615

Changes since my last review:

Renamed dps -> dbp
Added the headers <map> and <cassert> where they were used.
Rewritten assert condition to make the code simpler.

mzumsande

Code Review ACK e963615

I agree that it is good to remove the global map (I think this could also improve memory usage a little bit if the -reindex didn't finish successfully in case of block file corruption, and we switched to IBD without being able to connect and remove all entries from the map).

I don't get how this could fix a fuzzer OOM error, #19594 (comment) also doesn't elaborate on this.

mzumsande · 2022-07-11T18:17:36Z

src/node/blockstorage.cpp

@@ -834,6 +835,9 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
        // -reindex
        if (fReindex) {
            int nFile = 0;
+            // Map of disk positions for blocks with unknown parent (only used for reindex);


nit (no need to re-push for that): "(only used for reindex)" is obvious and could be dropped now that this got moved into a code block conditional on fReindex.

LarryRuane · 2022-07-11T22:24:06Z

I don't get how this could fix a fuzzer OOM error, #19594 (comment) also doesn't elaborate on this.

The fuzzer calls LoadExternalBlockFile() many times, and without this PR, each call adds entries to the static multimap that are never removed (they would be in real life, almost always, but not with random input).

It's easy to reproduce the OOM problem without causing an actual OOM (which could take a long time depending on your system's memory resources):

$ FUZZ=load_external_block_file src/test/fuzz/fuzz

and while that's running, in another window, monitor its memory usage. I ran ps -C b-test -o rss= every 10 minutes (for some reason, the fuzz executable is called b-test when running). With the master branch (units are kbyte):

(Then I stopped the test, clearly it continuously grows.) With this PR, there's expected startup growth, then it's unchanging:

LarryRuane · 2022-07-17T01:19:25Z

@practicalswift @promag @amitiuttarwar
You had previously ACKed PR #19594 by @hebasto that this PR replaces, maybe you would like to review, thanks.

Co-authored-by: Larry Ruane <larryruane@gmail.com>

LarryRuane · 2022-07-18T18:07:43Z

Force-pushed rebase for merge conflict.

theStack

re-ACK dd065da

shaavan

reACK dd065da

Changes since my last review:

Rebased over the master.

mzumsande · 2022-07-29T14:11:05Z

re-ACK dd065da

… rename it

07dfbb5 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès) Pull request description: Fixes #22189. The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables. ACKs for top commit: jamesob: ACK 07dfbb5 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a)) theStack: Concept and code-review ACK 07dfbb5 Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0

DrahtBot added the Refactoring label Jul 8, 2022

DrahtBot mentioned this pull request Jul 8, 2022

Use steady clock for all millis bench logging #25499

Merged

sipa reviewed Jul 8, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

shaavan reviewed Jul 8, 2022

View reviewed changes

LarryRuane force-pushed the 2022-07-mapBlocksUnknownParent branch from 3bd8416 to d6bbb65 Compare July 8, 2022 16:28

DrahtBot mentioned this pull request Jul 8, 2022

Improve runtime performance of --reindex #16981

Merged

theStack approved these changes Jul 9, 2022

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

LarryRuane force-pushed the 2022-07-mapBlocksUnknownParent branch from d6bbb65 to e963615 Compare July 9, 2022 16:31

theStack approved these changes Jul 9, 2022

View reviewed changes

shaavan approved these changes Jul 11, 2022

View reviewed changes

mzumsande reviewed Jul 11, 2022

View reviewed changes

DrahtBot mentioned this pull request Jul 13, 2022

[kernel 3b/n] Decouple {Dump,Load}Mempool from ArgsManager #25487

Merged

DrahtBot added the Needs rebase label Jul 18, 2022

refactor: Make mapBlocksUnknownParent local, and rename it

dd065da

Co-authored-by: Larry Ruane <larryruane@gmail.com>

LarryRuane force-pushed the 2022-07-mapBlocksUnknownParent branch from e963615 to dd065da Compare July 18, 2022 18:07

DrahtBot removed the Needs rebase label Jul 18, 2022

theStack approved these changes Jul 19, 2022

View reviewed changes

shaavan approved these changes Jul 22, 2022

View reviewed changes

fanquake merged commit 5871b5b into bitcoin:master Jul 29, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 29, 2022

Merge bitcoin#25571: refactor: Make mapBlocksUnknownParent local, and…

466a45a

… rename it

LarryRuane deleted the 2022-07-mapBlocksUnknownParent branch July 29, 2022 20:23

aureleoules mentioned this pull request Nov 16, 2022

Make static nLastFlush and nLastWrite Chainstate members #26513

Merged

bitcoin locked and limited conversation to collaborators Jul 29, 2023

refactor: Make mapBlocksUnknownParent local, and rename it #25571

refactor: Make mapBlocksUnknownParent local, and rename it #25571

Uh oh!

Conversation

LarryRuane commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Jul 8, 2022

Uh oh!

LarryRuane commented Jul 8, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LarryRuane commented Jul 9, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

LarryRuane commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LarryRuane commented Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LarryRuane commented Jul 18, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jul 29, 2022

Uh oh!

Uh oh!

LarryRuane commented Jul 8, 2022 •

edited

Loading

DrahtBot commented Jul 8, 2022 •

edited

Loading

LarryRuane commented Jul 11, 2022 •

edited

Loading

LarryRuane commented Jul 17, 2022 •

edited

Loading