refactor: Make mapBlocksUnknownParent local, and rename it #19594

hebasto · 2020-07-26T21:18:19Z

LarryRuane's comment in #16981:

Does it bother anyone else that this is static? This is global state, which, in general, equals evil. It must be static, given that it's declared here, since its state must persist across calls to this function (very often, a block and its parent are in different blk files). We could eliminate this global state by allocating this map in the caller of this function, src/init.cpp: ThreadImport(), and passing it by reference to LoadExternalBlockFile().

Another thing that seems missing from the current design is, what happens if there are leftover entries in this map after reindexing has completed? That means we've read blocks from disk but never found their parent blocks. In my testing, this map is empty at the end of reindexing, as expected. But what if it wasn't? That would mean we're missing one or more blocks/blk00nnn.dat files, or some blocks from those files. Isn't that worth at least a LogPrintf()? Making this change would also require moving the map out to the caller, because this function doesn't know if it will be called again.

hebasto · 2020-07-26T21:18:56Z

Friendly ping @LarryRuane :)

DrahtBot · 2020-07-27T03:25:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750 ([Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl)
[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749 ([Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex by dongcarl)
allow -loadblock blocks to be unsorted #20331 (allow -loadblock blocks to be unsorted by LarryRuane)
tree-wide: De-globalize ChainstateManager #20158 (tree-wide: De-globalize ChainstateManager by dongcarl)
Improve runtime performance of --reindex #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.

practicalswift · 2020-08-14T15:26:00Z

Concept ACK: local is easier to reason about :)

LarryRuane

ACK adc08e1
Looks good, very nice PR, better than I could have done! I did some basic testing (ran bitcoind -reindex).

Here's a slightly tangential suggestion, as long as you're changing this code, to eliminate one level of looping:

while (!queue.empty()) {
    const uint256& head = queue.front();
    auto it = blocks_with_unknown_parent->find(head);
    if (it == blocks_with_unknown_parent->end()) {
        queue.pop_front();
        continue;
    }
    std::shared_ptr<CBlock> pblockrecursive = std::make_shared<CBlock>();
    if (ReadBlockFromDisk(*pblockrecursive, it->second, chainparams.GetConsensus()))
    {
        LogPrint(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, pblockrecursive->GetHash().ToString(),
                head.ToString());
        LOCK(cs_main);
        BlockValidationState dummy;
        if (::ChainstateActive().AcceptBlock(pblockrecursive, dummy, chainparams, nullptr, true, &it->second, nullptr))
        {
            nLoaded++;
            queue.push_back(pblockrecursive->GetHash());
        }
    }
    blocks_with_unknown_parent->erase(it);
    NotifyHeaderTip();
}

LarryRuane · 2020-08-14T16:31:59Z

src/validation.cpp

@@ -4752,15 +4758,17 @@ void LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi

                NotifyHeaderTip();

+                if (!blocks_with_unknown_parent) continue;


Nice! It was hard to understand the previous code when !dbp.

LarryRuane · 2020-08-14T17:10:35Z

src/validation.cpp

                        range.first++;
-                        mapBlocksUnknownParent.erase(it);
+                        blocks_with_unknown_parent->erase(it);


Suggested change

range.first++;

mapBlocksUnknownParent.erase(it);

blocks_with_unknown_parent->erase(it);

range.first = blocks_with_unknown_parent->erase(it);

As written this is safe with this container type, but the assignment makes fewer assumptions (and is one less line of code!).

Agree. But current diff is minimal :)

LarryRuane · 2020-08-14T18:05:29Z

src/validation.cpp

+                        if (dbp && blocks_with_unknown_parent) {
+                            blocks_with_unknown_parent->emplace(block.hashPrevBlock, *dbp);
+                        }


You could test either variable here (probably blocks_with_unknown_parent would be best), but I assume you're testing both since the code references both. The only downside is it could lead the casual reader to think that only one of these could be nullptr and not the other. The emplace simplification is nice here.

... but I assume you're testing both since the code references both.

Correct.

hebasto · 2020-08-14T19:08:44Z

@LarryRuane

Here's a slightly tangential suggestion, as long as you're changing this code, to eliminate one level of looping:

Let's keep this PR diff minimal and focused :)

promag

Concept ACK.

epson121

Code review ACK adc08e1

theStack

Code Review ACK adc08e1 🧇

dongcarl · 2020-10-19T21:17:07Z

~~Concept ACK. This is a necessary change for 2f2e8a2 (#20158) to work correctly.~~
Sorry, looking at this again now. It doesn't seem like it's fixing anything that's not working. If the concern is "what happens if the map is not empty at the end of reindexing," then we should write a test case for that and implement the warning.

amitiuttarwar · 2020-11-03T02:40:41Z

code review ACK adc08e1

but I think OP should be updated. code currently is a pure refactor to maintain less global state. however, OP also has section that asks "what happens if there are leftover entries in this map after reindexing has completed", which I don't believe is addressed in this PR?

tangential question: is -loadblock still used, or mostly a leftover relic? I was having a hard time understanding the purpose of -loadblock, and why it wouldn't take advantage of the orphan handling we have available. Looks like it was introduced in #883 and intentionally doesn't handle orphans (#1695). Seems like it might just be a dated mechanism that we still have around? So I'm interested to hear if there are current use cases.

LarryRuane · 2020-11-03T17:33:37Z

@amitiuttarwar :

... I don't believe is addressed in this PR

The OP does mention logging if there are entries leftover entries, and I think that would be a good idea. But no other recovery is necessary, because the client will download the missing blocks from peers (I've verified this).

While working on zcash, I recently looked into the -loadblock argument and concluded that it should be removed, see zcash/zcash#4679 (comment), I'll quote the relevant part (the same 2012 commit hash referenced in this comment also exists in the bitcoin repo here):

I don't think -loadblock makes sense at all. I just found this interesting commit where -reindex was added and -loadblock already existed -- so -reindex was a nicer way to do what people used -loadblock to do previously. That strengthens my opinion that -loadblock isn't worth fixing. My guess is that back in 2012, the "headers-first" block download hadn't been implemented and merged yet, so blocks were in height order in the blk?????.dat files, so -loadblock worked well. Headers-first download broke it, but there's been no strong reason to fix it since -reindex existed by then.

I could make a small PR to remove -loadblock, if that would be helpful (that shouldn't be part of the current PR).

LarryRuane · 2020-11-03T18:06:45Z

me:

I could make a small PR to remove -loadblock

On further thought, we shouldn't do this, because I can easily imagine that it may be currently used for testing (outside of our repo), for example, with regtest.

sipa · 2020-11-03T18:07:31Z

@amitiuttarwar The purpose of -loadblock is loading external blocks from e.g. a bootstrap.dat file. I don't know if those see much use still, but we distribute the script to produce bootstrap.dat files still (see https://github.com/bitcoin/bitcoin/tree/master/contrib/linearize).

maflcko · 2020-11-03T18:39:37Z

It should also be possible to specify all block files of another datadir via multiple -loadblock. (sync via local disk)

LarryRuane · 2020-11-03T19:09:38Z

It should also be possible to specify all block files of another datadir via multiple -loadblock

No, because when using -loadblock (as opposed to -reindex), there is no unknown parent map. When -loadblock encounters a block whose parent hasn't yet been seen, which is very common beginning with headers-first download, it's silently ignored.

We could add an unknown parents map to the -loadblock path, but that doesn't exist currently. I could make a tiny PR to do that, if it would be useful (it would make sense base it on the current PR).

amitiuttarwar · 2020-11-03T23:00:15Z

@LarryRuane

The OP does mention logging if there are entries leftover entries, and I think that would be a good idea. But no other recovery is necessary, because the client will download the missing blocks from peers (I've verified this).

right, sure, I think logging would make sense, but I'm talking about the difference between OP and code changes. Unless I'm mistaken, the code doesn't address this, so its misleading to talk about in the OP. additional logging could be added to this PR or it could be addressed in a separate PR, but the code & description should match :)

RE: -loadblock:

I just posted in irc asking if people are currently using. one person said they still use it
if we are trying to import block files from another datadir, wouldn't we just add them in the right directory and then reindex?

on irc @sipa said

i think part is that you don't want to encourage people copying entire datadirs, as chainstates don't get re-verified
if you copy a chainstate from someone (not just block files), they don't get re-verified (because your node wouldn't know it's operating on an imported copy)

as Larry said, one reason loadblock wouldn't work with the blocks dat is because it disables the parent map. but I don't think we should add functionality to amplify support unless we have concretely known use cases

but again, I don't think this is the most relevant to this PR. I current don't feel any conclusive evidence towards removing loadblock or adding support, but if I did I think opening an issue & continuing conversation there would make sense

DrahtBot · 2021-02-01T13:10:35Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

LarryRuane · 2021-04-24T04:51:37Z

@hebasto, I rebased this PR's commit as part of #20331. Feel free to cherry-pick it back to here. (A review of #20331 would be appreciated, BTW.)

maflcko · 2021-11-03T10:01:09Z

This will also fix an issue where the fuzz test goes OOM when running too long

DrahtBot · 2022-02-22T12:17:51Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

hebasto · 2022-04-21T14:44:18Z

I won't be able to focus on this stuff in the near future.

So closing up for grabs.

…cal, and rename it dd065da refactor: Make mapBlocksUnknownParent local, and rename it (Hennadii Stepanov) Pull request description: 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](bitcoin/bitcoin#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. ACKs for top commit: mzumsande: re-ACK dd065da theStack: re-ACK dd065da shaavan: reACK dd065da Tree-SHA512: 9cd20e44d2fa1096dd405bc107bc065ea8f904f5b3f63080341b08d8cf57b790df565f58815c2f331377d044d5306708b4bf6bdfc5ef8d0ed85d8e97d744732c

refactor: Make mapBlocksUnknownParent local, and rename it

adc08e1

DrahtBot added Refactoring Tests Validation labels Jul 26, 2020

This was referenced Jul 27, 2020

Improve runtime performance of --reindex #16981

Merged

assumeutxo #15606

Closed

LarryRuane approved these changes Aug 14, 2020

View reviewed changes

maflcko removed the Tests label Aug 23, 2020

promag reviewed Sep 7, 2020

View reviewed changes

epson121 reviewed Sep 7, 2020

View reviewed changes

theStack approved these changes Oct 11, 2020

View reviewed changes

DrahtBot mentioned this pull request Oct 15, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

LarryRuane mentioned this pull request Nov 7, 2020

allow -loadblock blocks to be unsorted #20331

Closed

This was referenced Dec 22, 2020

[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750

Merged

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Merged

DrahtBot added the Needs rebase label Feb 1, 2021

hebasto closed this Apr 21, 2022

hebasto added the Up for grabs label Apr 21, 2022

LarryRuane mentioned this pull request Jul 8, 2022

refactor: Make mapBlocksUnknownParent local, and rename it #25571

Merged

maflcko removed the Up for grabs label Jul 8, 2022

bitcoin locked and limited conversation to collaborators Jul 8, 2023

		@@ -4752,15 +4758,17 @@ void LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi

		NotifyHeaderTip();

		if (!blocks_with_unknown_parent) continue;

refactor: Make mapBlocksUnknownParent local, and rename it #19594

refactor: Make mapBlocksUnknownParent local, and rename it #19594

Uh oh!

Conversation

hebasto commented Jul 26, 2020

Uh oh!

hebasto commented Jul 26, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

practicalswift commented Aug 14, 2020

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

LarryRuane Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

LarryRuane Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 14, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

epson121 left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitiuttarwar commented Nov 3, 2020

Uh oh!

LarryRuane commented Nov 3, 2020

Uh oh!

LarryRuane commented Nov 3, 2020

Uh oh!

sipa commented Nov 3, 2020

Uh oh!

maflcko commented Nov 3, 2020

Uh oh!

LarryRuane commented Nov 3, 2020

Uh oh!

amitiuttarwar commented Nov 3, 2020

Uh oh!

DrahtBot commented Feb 1, 2021

Uh oh!

LarryRuane commented Apr 24, 2021

Uh oh!

maflcko commented Nov 3, 2021

Uh oh!

DrahtBot commented Feb 22, 2022

Uh oh!

hebasto commented Apr 21, 2022

Uh oh!

Uh oh!

DrahtBot commented Jul 27, 2020 •

edited

Loading

dongcarl commented Oct 19, 2020 •

edited

Loading