allow -loadblock blocks to be unsorted #20331

LarryRuane · 2020-11-07T05:24:09Z

This PR was suggested in this comment in #19594, and builds on that PR (the first commit here).

This PR allows the blocks in files specified by the -loadblock=file command line (configuration) options (there can be multiple) to be unsorted. This is already allowed by -reindex, but currently when -loadblock encounters a block with an unknown parent, it is ignored. This means the blocks must be sorted by height to be loaded successfully. This PR fixes this restriction, allowing the blocks across all the specified blocks files to be in any order. This makes the -loadblock option more useful. Also, the affected code is cleaner and more maintainable.

(I'll simplify this description move details to a separate comment before this PR gets merged.)

DrahtBot · 2020-11-07T12:45:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22937 (refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method by ryanofsky)
#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.

LarryRuane · 2020-11-09T18:00:25Z

A useful follow-on to this PR, since loading unordered blocks would become possible, might be the addition of a -loadblocksdir= argument that takes a directory path and loads all blocks found in the directory. That way it's not necessary to specify a very large number of -loadblock arguments. It would then be easy to import an entire blockchain.

Another way to accomplish approximately the same thing would be to replace the entire blocks directory in the data directory and then -reindex. But using -loadblocksdir would allow you to "merge" blocks into your existing blocks list (suppressing duplicates), which is kind of cool.

adamjonas · 2020-11-10T00:37:00Z

Appears that the CI picked up a couple of argument issues with the LoadExternalBlockFile fuzz test:

test/fuzz/load_external_block_file.cpp:33:9: error: no matching function for call to 'LoadExternalBlockFile'
        LoadExternalBlockFile(Params(), fuzzed_block_file, &flat_file_pos, &blocks_with_unknown_parent);
        ^~~~~~~~~~~~~~~~~~~~~
./validation.h:162:6: note: candidate function not viable: requires 5 arguments, but 4 were provided
void LoadExternalBlockFile(
     ^
test/fuzz/load_external_block_file.cpp:35:9: error: no matching function for call to 'LoadExternalBlockFile'
        LoadExternalBlockFile(Params(), fuzzed_block_file);
        ^~~~~~~~~~~~~~~~~~~~~
./validation.h:162:6: note: candidate function not viable: requires 5 arguments, but 2 were provided
void LoadExternalBlockFile(
     ^
2 errors generated.

LarryRuane · 2020-11-10T03:03:23Z

I don't know if anyone has been confused by -loadblock not loading all the blocks, but that did happen on the zcash network (a bitcoin core fork that I also work on): zcash/zcash#4679

LarryRuane · 2020-11-10T03:32:46Z

Thanks, @adamjonas, force-pushed to fix the fuzz test.

mjdietzx · 2020-11-11T18:45:30Z

src/validation.cpp

+    FILE* file = fsbridge::fopen(path, "rb");
+    if (!file) {
+        return error("ReadBlockFromDisk: open failed for %s", path.string());
+    }
+    if (offset > 0 && fseek(file, offset, SEEK_SET)) {
+        fclose(file);
+        return error("ReadBlockFromDisk: fseek failed for %s offset %u", path.string(), offset);
+    }
+    CAutoFile filein(file, SER_DISK, CLIENT_VERSION);


Why was this change made? Could you explain how the behavior of this is changing, aside from the function signature taking const fs::path& path, unsigned int offset instead of const FlatFilePos& pos?

Note: I'm guessing this change was made bc of what you do at line 4810: ReadBlockFromDisk(*pblockrecursive, blk_paths[child_pos.nFile], child_pos.nPos, chainparams.GetConsensus())

But I'm failing to understand why you can't just do: ReadBlockFromDisk(*pblockrecursive, child_pos, chainparams.GetConsensus())

I'm probably missing something here, but I just want to check

Yes, I reached several dead-ends before arriving at this code factoring. We can't use ReadBlockFromDisk(..., child_pos, ...) here because the pos argument is used in lower-level code (OpenFlockFile()) to construct and open a pathname like .../.bitcoin/blocks/blknnnnn.dat, which is just what we want for the -reindex case, but not for the -loadblock case (both share this code), because in the latter, we have a list of arbitrary pathnames to read from. Instead of calling the pos variant in the -reindex case and the path, offset variant in the -loadblock case, it's simpler to just call it in the same way for both cases.

mjdietzx · 2020-11-11T18:46:48Z

src/init.cpp

-        } else {
-            LogPrintf("Warning: Could not open blocks file %s\n", path.string());
-        }
+    if (vImportFiles.size() > 0) {


Do you need the additional code branch here? Would it be preferable to just invoke LoadExternalBlockFiles("Importing", chainparams, vImportFiles, true); no matter what, and the for loop just won't iterate through anything if vImportFiles is empty?

You're right, it's not really needed, but I think it's useful to show the reader here that loading blocks this isn't always being done; otherwise, to know nothing's being done, the reader has to go look at LoadExternalBlockFiles().

I also like that the conditional makes this part of the code similar to the if (fReindex) code just above it; they are doing very similar work.

But I could go either way (no strong preference).

mjdietzx · 2020-11-11T19:09:18Z

src/validation.cpp

+        FILE* file = fsbridge::fopen(blk_paths[n_file], "rb");
+        if (file == nullptr) {
+            LogPrintf("%s: Warning: Could not open blocks file %s\n", __func__, blk_paths[n_file]);
+            return;
+        }


Isn't this kinda duplication of what ReadBlockFromDisk does? I'm wondering why FlatFilePos pos(n_file, 0); isn't created, passed to ReadBlockFromDisk (like before, when signature took FlatFilePos), and then you can pass it to LoadExternalBlockFile

This duplication exists even without this PR. Before this PR, this fopen() happens here.

To summarize why this happens in two places: There are two ways blocks are "loaded" into memory when reindexing (and, now, with this PR, when using -loadblock). First, a blocks file is opened (here) and read from disk (using large reads) sequentially. At this point, we don't know the individual blocks' starting offsets, so we can't create a FlatFilePos to pass to ReadBlockFromDisk() as you suggest. Besides, doing these large reads is more efficient, typically many blocks per read -- plus a partial block at the end (since, again, the block boundaries are unknown). Once in memory, the block boundaries are discovered, and each block is deserialized. But the blocks in these blocks files are mostly out of order, so most of the blocks can't be accepted (have AcceptBlock() called on it) because its parent has not yet been seen.

When such a parent is later found, the child block is then re-read from disk using ReadBlockFromDisk(), deserialized a second time, and then passed to AcceptBlock() (allowed because its parent is now present). In this case, we're reading exactly a single block, because we know where it starts (in the file) and its length (which is what a FlatFilePos encodes).

mjdietzx · 2020-11-11T19:11:42Z

src/validation.h

+    const std::vector<fs::path>& blk_paths,
+    size_t n_file,


Given the name LoadExternalBlockFile I'm confused why blk_paths plural is passed as a param, instead of just a single file/path

That function, LoadExternalBlockFile(), does (attempt to) load the blocks from exactly one block file, but, doing so may cause, as a side effect, the loading of blocks seen earlier (in earlier files) but that could not be loaded then because their parent was missing (and is now present, see my earlier comment). This requires reaching back and reading specific blocks from earlier files, and that requires the full list of block file paths (the blk_paths argument). This basic flow is the same as before this PR. I agree this is somewhat confusing, maybe a comment would help. But maybe not, because perhaps there's no shortcut to just studying the code, I'm not sure.

LarryRuane

@mjdietzx, thank you for your review!

LarryRuane · 2020-11-11T20:14:36Z

src/init.cpp

-        } else {
-            LogPrintf("Warning: Could not open blocks file %s\n", path.string());
-        }
+    if (vImportFiles.size() > 0) {


You're right, it's not really needed, but I think it's useful to show the reader here that loading blocks this isn't always being done; otherwise, to know nothing's being done, the reader has to go look at LoadExternalBlockFiles().

I also like that the conditional makes this part of the code similar to the if (fReindex) code just above it; they are doing very similar work.

But I could go either way (no strong preference).

LarryRuane · 2020-11-11T20:34:43Z

src/validation.cpp

+    FILE* file = fsbridge::fopen(path, "rb");
+    if (!file) {
+        return error("ReadBlockFromDisk: open failed for %s", path.string());
+    }
+    if (offset > 0 && fseek(file, offset, SEEK_SET)) {
+        fclose(file);
+        return error("ReadBlockFromDisk: fseek failed for %s offset %u", path.string(), offset);
+    }
+    CAutoFile filein(file, SER_DISK, CLIENT_VERSION);


Yes, I reached several dead-ends before arriving at this code factoring. We can't use ReadBlockFromDisk(..., child_pos, ...) here because the pos argument is used in lower-level code (OpenFlockFile()) to construct and open a pathname like .../.bitcoin/blocks/blknnnnn.dat, which is just what we want for the -reindex case, but not for the -loadblock case (both share this code), because in the latter, we have a list of arbitrary pathnames to read from. Instead of calling the pos variant in the -reindex case and the path, offset variant in the -loadblock case, it's simpler to just call it in the same way for both cases.

LarryRuane · 2020-11-11T23:09:51Z

src/validation.cpp

+        FILE* file = fsbridge::fopen(blk_paths[n_file], "rb");
+        if (file == nullptr) {
+            LogPrintf("%s: Warning: Could not open blocks file %s\n", __func__, blk_paths[n_file]);
+            return;
+        }


This duplication exists even without this PR. Before this PR, this fopen() happens here.

To summarize why this happens in two places: There are two ways blocks are "loaded" into memory when reindexing (and, now, with this PR, when using -loadblock). First, a blocks file is opened (here) and read from disk (using large reads) sequentially. At this point, we don't know the individual blocks' starting offsets, so we can't create a FlatFilePos to pass to ReadBlockFromDisk() as you suggest. Besides, doing these large reads is more efficient, typically many blocks per read -- plus a partial block at the end (since, again, the block boundaries are unknown). Once in memory, the block boundaries are discovered, and each block is deserialized. But the blocks in these blocks files are mostly out of order, so most of the blocks can't be accepted (have AcceptBlock() called on it) because its parent has not yet been seen.

When such a parent is later found, the child block is then re-read from disk using ReadBlockFromDisk(), deserialized a second time, and then passed to AcceptBlock() (allowed because its parent is now present). In this case, we're reading exactly a single block, because we know where it starts (in the file) and its length (which is what a FlatFilePos encodes).

LarryRuane · 2020-11-11T23:20:17Z

src/validation.h

+    const std::vector<fs::path>& blk_paths,
+    size_t n_file,


That function, LoadExternalBlockFile(), does (attempt to) load the blocks from exactly one block file, but, doing so may cause, as a side effect, the loading of blocks seen earlier (in earlier files) but that could not be loaded then because their parent was missing (and is now present, see my earlier comment). This requires reaching back and reading specific blocks from earlier files, and that requires the full list of block file paths (the blk_paths argument). This basic flow is the same as before this PR. I agree this is somewhat confusing, maybe a comment would help. But maybe not, because perhaps there's no shortcut to just studying the code, I'm not sure.

LarryRuane · 2020-11-11T23:22:10Z

src/validation.cpp

@@ -49,6 +49,7 @@
 #include <validationinterface.h>
 #include <warnings.h>

+#include <cassert>


This isn't needed, I'll remove in a later force-push.

LarryRuane · 2020-11-11T23:22:30Z

src/test/fuzz/load_external_block_file.cpp

@@ -11,6 +11,7 @@
 #include <validation.h>

 #include <cstdint>
+#include <map>


This isn't needed, I'll remove in a later force-push.

LarryRuane · 2020-11-12T17:00:00Z

Here's a manual test procedure (I'll describe testnet because it's faster, but mainnet works too)

testnet node is not running
wait for your testnet3 data directory to get out of date (don't run the client for at least several days)
change directory to your .bitcoin data directory
rename testnet3 to testnet3-save
start the node with -testnet, wait for it to sync (IBD), it will have more blocks (and more blocks files) than testnet3-save
stop the node
rename testnet3 to testnet3-new
rename testnet3-save to testnet3
start the node as follows, except replace 191 with the highest-numbered file in the testnet3-save directory:

bitcoind -testnet -networkactive=0 $(seq 0 191|xargs printf '-loadblock=testnet3-new/blocks/blk%05u.dat ')

This generates a command line like:

bitcoind -testnet -networkactive=0 -loadblock=testnet3-new/blocks/blk00000.dat -loadblock=testnet3-new/blocks/blk00001.dat ...

observe that, even though not syncing with peers, the node successfully reaches a recent block height (look at the most recent UpdateTip in debug.log).

promag · 2020-11-14T11:15:50Z

@LarryRuane I guess it's possible to make a functional test then?

LarryRuane · 2020-11-14T16:02:39Z

@promag I didn't write a new functional test, but I did modify the existing loadblock functional test to cover the new behavior (the test fails without the PR). Is that sufficient?

dongcarl · 2020-12-03T17:26:38Z

Verified that the test fails without the new behaviour, cool!

A couple of things that could be done:

Instead of asserting based on the debug log (which is a bit more fragile), perhaps the assertion can be made based on the number of blocks connected (tip height). We may need to disable networking for this to work reliably.
I think the changes in test/functional should be split into its own commit and put as the first commit in this patchset, this way it's easier for reviewers to verify that the new behavior does indeed do what it's supposed to do.

LarryRuane · 2020-12-04T02:42:18Z

Force-pushed to implement both suggestions from @dongcarl (thank you!).

Reminder, this PR is layered on top of #19594 so please review only the last two commits (of the 3).

UPDATE Mar 5, 2021: I squashed the last two commits, so only review the last commit.

LarryRuane · 2021-02-04T15:25:11Z

Rebased, no functional changes.

LarryRuane · 2021-04-23T16:10:18Z

This required rebase also splits the second commit into two commits to separate refactoring from the functional changes. Ready for review.

LarryRuane · 2021-05-07T04:38:54Z

Rebased. The second commit is now much simpler because of #21727.

sipa · 2021-06-10T22:45:51Z

utACK dbd7f72

LarryRuane · 2021-06-14T03:40:18Z

Rebased to resolve merge conflicts.

This lower-level version takes a path and a seek offset as arguments. Currently unused but is needed by the next commit.

Add LoadExternalBlockFiles() (plural) that accepts a list of paths and loads all the blocks found in these paths. The blocks can be completely out of order; block C (child) can be seen first and its parent P not seen until a later file, then block C will be processed. This requires re-reading block C from the earlier file; block C isn't kept in memory during this interval. The list of paths is either the blocks/blk*.dat files (during reindexing) or given with one or more -loadblk arguments. LoadExternalBlockFiles() calls LoadExternalBlockFile() (singular) to read a single file, but out-of-order blocks requires that this function be able to read individual blocks from earlier files.

DrahtBot · 2021-10-15T09:40:34Z

🐙 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".}

DrahtBot · 2022-02-22T10:49:37Z

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.

LarryRuane · 2022-03-08T21:12:06Z

The -loadblock feature doesn't seem to be often used, so improvements to it aren't much in demand, so closing this. If this feature becomes more important, we can resurrect this PR.

DrahtBot added the Validation label Nov 7, 2020

This was referenced Nov 7, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

Improve runtime performance of --reindex #16981

Merged

LarryRuane force-pushed the loadblock-unsorted branch from 9fbcd0b to 37408b8 Compare November 9, 2020 17:42

LarryRuane force-pushed the loadblock-unsorted branch from 37408b8 to 7afedcf Compare November 10, 2020 03:29

LarryRuane mentioned this pull request Nov 10, 2020

The -loadblock command only reads the first few blocks from the file, but not all. zcash/zcash#4679

Closed

DrahtBot mentioned this pull request Nov 10, 2020

refactor: Make mapBlocksUnknownParent local, and rename it #19594

Closed

mjdietzx reviewed Nov 11, 2020

View reviewed changes

LarryRuane commented Nov 11, 2020

View reviewed changes

LarryRuane force-pushed the loadblock-unsorted branch from 7afedcf to b99ecea Compare December 4, 2020 02:37

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

LarryRuane force-pushed the loadblock-unsorted branch from b99ecea to 1a3f38e Compare February 4, 2021 07:23

DrahtBot removed the Needs rebase label Feb 4, 2021

DrahtBot mentioned this pull request Feb 4, 2021

[Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055

Merged

2 tasks

DrahtBot mentioned this pull request Feb 23, 2021

[Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270

Merged

1 task

DrahtBot added the Needs rebase label Mar 4, 2021

LarryRuane force-pushed the loadblock-unsorted branch from 1a3f38e to 2cfd1b4 Compare March 5, 2021 17:42

DrahtBot removed the Needs rebase label Apr 23, 2021

DrahtBot mentioned this pull request Apr 23, 2021

refactor: Move more stuff to blockstorage #21727

Merged

LarryRuane force-pushed the loadblock-unsorted branch from 78ae057 to c0c9bec Compare April 24, 2021 04:28

DrahtBot mentioned this pull request Apr 28, 2021

refactor: Remove ::Params() global from CChainState #21789

Merged

DrahtBot added the Needs rebase label May 5, 2021

LarryRuane force-pushed the loadblock-unsorted branch from c0c9bec to dbd7f72 Compare May 7, 2021 04:37

DrahtBot removed the Needs rebase label May 7, 2021

DrahtBot mentioned this pull request May 7, 2021

[Bundle 7/7] validation: Farewell, global Chainstate! #21866

Merged

dongcarl mentioned this pull request Jun 8, 2021

validation: Review mutable static local variables in CChainState methods #22189

Closed

DrahtBot added the Needs rebase label Jun 12, 2021

LarryRuane force-pushed the loadblock-unsorted branch from dbd7f72 to a62fc29 Compare June 14, 2021 01:26

DrahtBot removed the Needs rebase label Jun 14, 2021

DrahtBot added the Needs rebase label Jun 29, 2021

hebasto and others added 3 commits July 27, 2021 17:07

refactor: Make mapBlocksUnknownParent local, and rename it

f58f1aa

refactor: Add a new version of ReadBlockFromDisk()

130508e

This lower-level version takes a path and a seek offset as arguments. Currently unused but is needed by the next commit.

LarryRuane force-pushed the loadblock-unsorted branch from a62fc29 to 0c97fa3 Compare July 27, 2021 23:51

DrahtBot removed the Needs rebase label Jul 28, 2021

This was referenced Sep 10, 2021

refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method #22937

Merged

Drop IO priority to idle while reading blocks for peer requests and startup verification #9245

Closed

DrahtBot added the Needs rebase label Oct 15, 2021

LarryRuane closed this Mar 8, 2022

bitcoin locked and limited conversation to collaborators Mar 8, 2023

allow -loadblock blocks to be unsorted #20331

allow -loadblock blocks to be unsorted #20331

Uh oh!

Conversation

LarryRuane commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

LarryRuane commented Nov 9, 2020

Uh oh!

adamjonas commented Nov 10, 2020

Uh oh!

LarryRuane commented Nov 10, 2020

Uh oh!

LarryRuane commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryRuane commented Nov 12, 2020

Uh oh!

promag commented Nov 14, 2020

Uh oh!

LarryRuane commented Nov 14, 2020

Uh oh!

dongcarl commented Dec 3, 2020

Uh oh!

LarryRuane commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LarryRuane commented Feb 4, 2021

Uh oh!

LarryRuane commented Apr 23, 2021

Uh oh!

LarryRuane commented May 7, 2021

Uh oh!

sipa commented Jun 10, 2021

Uh oh!

LarryRuane commented Jun 14, 2021

Uh oh!

DrahtBot commented Oct 15, 2021

Uh oh!

LarryRuane commented Nov 7, 2020 •

edited

Loading

DrahtBot commented Nov 7, 2020 •

edited

Loading

LarryRuane commented Nov 10, 2020 •

edited

Loading

LarryRuane Nov 11, 2020 •

edited

Loading

LarryRuane Nov 11, 2020 •

edited

Loading

LarryRuane commented Dec 4, 2020 •

edited

Loading