blockstorage: do not flush block to disk if it is already there

pinheadmz commented at 6:50 pm on February 3, 2023: member

Closes #2039

When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the blk files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.

FindBlockPos() may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call ConnectBlock() which will save undo data at that time.

The call stack looks like this:

0init()
1ThreadImport() <-- process fReindex flag
2LoadExternalBlockFile()
3AcceptBlock()
4SaveBlockToDisk()
5FindBlockPos()
6FlushBlockFile() <-- unnecessary if block is already on disk

A larger refactor of this part of the code was started by mzumsande here: https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.

DrahtBot commented at 6:50 pm on February 3, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, sipa, mzumsande, achow101
Concept ACK	jonatack, turkycat
Approach ACK	stickies-v
Stale ACK	LarryRuane, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29672 (validation: Make translations of fatal errors consistent by TheCharlatan)
#29642 (kernel: Handle fatal errors through return values by TheCharlatan)
#29231 (logging: Update to new logging API by ajtowns)

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.

DrahtBot added the label Block storage on Feb 3, 2023

in test/functional/feature_reindex.py:89 in 7a2e5cdc7f outdated

86+        # first blk00000.dat file and starts another block file
87+        self.generatetoaddress(self.nodes[1], 300, self.nodes[1].get_deterministic_priv_key().address)
88+        self.stop_node(1)
89+
90+        # make the first block file read-only
91+        path = os.path.join(self.nodes[1].chain_path, 'blocks', 'blk00000.dat');

brunoerg commented at 6:54 pm on February 3, 2023:

0        path = os.path.join(self.nodes[1].chain_path, 'blocks', 'blk00000.dat')

pinheadmz commented at 6:59 pm on February 3, 2023:

thanks! fixed.

pinheadmz force-pushed on Feb 3, 2023

in test/functional/feature_reindex.py:31 in a8db898add outdated

30         self.generatetoaddress(self.nodes[0], 3, self.nodes[0].get_deterministic_priv_key().address)
31         blockcount = self.nodes[0].getblockcount()
32-        self.stop_nodes()
33-        extra_args = [["-reindex-chainstate" if justchainstate else "-reindex"]]
34-        self.start_nodes(extra_args)
35+        self.stop_node(0)

brunoerg commented at 8:45 pm on February 3, 2023:

Instead of stopping the node and right after starting it again, you could use self.restart_node.

pinheadmz commented at 9:15 pm on February 3, 2023:

ah thanks, fixed. also added an explanation of the test to the comments at the top

pinheadmz force-pushed on Feb 3, 2023

in test/functional/feature_reindex.py:84 in a6b05f8db6 outdated

81 
82+    def reindex_readonly(self):
83+        self.connect_nodes(0, 1)
84+        # generate enough blocks to ensure that the -fastprune node fills up the
85+        # first blk00000.dat file and starts another block file
86+        self.generatetoaddress(self.nodes[1], 300, self.nodes[1].get_deterministic_priv_key().address)

LarryRuane commented at 0:03 am on February 4, 2023:

Verified:

0$ ls -l /tmp/btest/node1/regtest/blocks/blk*.dat
1-rw------- 1 larry larry 65400 Feb  3 16:58 /tmp/btest/node1/regtest/blocks/blk00000.dat
2-rw------- 1 larry larry 16384 Feb  3 16:58 /tmp/btest/node1/regtest/blocks/blk00001.dat

brunoerg commented at 6:18 pm on February 8, 2023:

To ensure it, I think you could check if both files exist:

 0diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py
 1index c68773e06..a3c9562f1 100755
 2--- a/test/functional/feature_reindex.py
 3+++ b/test/functional/feature_reindex.py
 4@@ -84,6 +84,9 @@ class ReindexTest(BitcoinTestFramework):
 5         self.generatetoaddress(self.nodes[1], 300, self.nodes[1].get_deterministic_priv_key().address)
 6         self.stop_node(1)
 7 
 8+        assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00000.dat')
 9+        assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00001.dat')
10+
11         # make the first block file read-only
12         path = os.path.join(self.nodes[1].chain_path, 'blocks', 'blk00000.dat')
13         os.chmod(path, 0o444)

pinheadmz commented at 7:50 pm on February 8, 2023:

thanks, added.

in test/functional/feature_reindex.py:92 in a6b05f8db6 outdated

86+        self.generatetoaddress(self.nodes[1], 300, self.nodes[1].get_deterministic_priv_key().address)
87+        self.stop_node(1)
88+
89+        # make the first block file read-only
90+        path = os.path.join(self.nodes[1].chain_path, 'blocks', 'blk00000.dat')
91+        os.chmod(path, 0o444)

LarryRuane commented at 0:04 am on February 4, 2023:

Verified:

0$ ls -l /tmp/btest/node1/regtest/blocks/blk*.dat
1-r--r--r-- 1 larry larry 65400 Feb  3 16:58 /tmp/btest/node1/regtest/blocks/blk00000.dat
2-rw------- 1 larry larry 16384 Feb  3 16:58 /tmp/btest/node1/regtest/blocks/blk00001.dat

in test/functional/feature_reindex.py:95 in a6b05f8db6 outdated

89+        # make the first block file read-only
90+        path = os.path.join(self.nodes[1].chain_path, 'blocks', 'blk00000.dat')
91+        os.chmod(path, 0o444)
92+
93+        # restart and reindex the node with the read-only block file
94+        self.start_node(1, ["-reindex", "-fastprune"])

LarryRuane commented at 0:10 am on February 4, 2023:

Verified that without the PR, the node doesn’t start, and we see this in debug.log:

02023-02-04T00:05:32.219184Z [loadblk] [node/blockstorage.cpp:865] [ThreadImport] Reindexing block file blk00001.dat...
12023-02-04T00:05:32.224307Z [loadblk] [flatfile.cpp:44] [Open] Unable to open file /tmp/btest/node1/regtest/blocks/blk00000.dat
22023-02-04T00:05:32.224320Z [loadblk] [util/system.h:50] [error] ERROR: Flush: failed to open file 0
32023-02-04T00:05:32.224334Z [loadblk] [shutdown.cpp:26] [AbortNode] *** Flushing block file to disk failed. This is likely the result of an I/O error.

Nothing the PR needs to address, but just FYI, the first message may look confusing; it mentions blk00001.dat just before it attempts to write the previous file, blk00001.dat, which is where it runs into the write failure. Notice that this write is what the PR prevents because it’s unnecessary. This is a nice way to test that the write happens without the PR, and doesn’t happen with the PR.

in test/functional/feature_reindex.py:114 in a6b05f8db6 outdated

94+        self.start_node(1, ["-reindex", "-fastprune"])
95+        assert_equal(self.nodes[1].getblockcount(), 312)
96+
97+        # shut down this node, triggering disk flushes, but not to the read-only file
98+        with self.nodes[1].assert_debug_log(expected_msgs=['Flush'], unexpected_msgs=['Error']):
99+            self.stop_node(1)

LarryRuane commented at 0:24 am on February 4, 2023:

There are many Flush log lines for unrelated things (like the ChainState), so consider making that string more specific. Maybe:

02023-02-04T00:15:05.877123Z [shutoff] [logging/timer.h:57] [Log] [bench] FlushStateToDisk: write block and undo data to disk completed (6.62ms)

Similarly, maybe “Error” is too general? It does work, but I’d be afraid that something unrelated might log “Error” in the future and break this test. Maybe look for one of the error strings in my comment above?

pinheadmz commented at 12:14 pm on February 4, 2023:

good call, I was being too general, updated. thanks!

LarryRuane commented at 0:30 am on February 4, 2023: contributor

Approach and code review ACK a6b05f8db63bae7eefb5330048d8b1441f1a9e5e I don’t understand why the test needs to start a second node or why -fastprune is needed (but that may just be me!).

EDIT: never mind, I see now why it’s needed, you’ve documented it well.

reviewers can let me know if the changes should be combined

It seems fine to me that this is a separate PR, but I don’t have a strong opinion.

pinheadmz force-pushed on Feb 4, 2023

in test/functional/feature_reindex.py:11 in 386faaf76e outdated

 7@@ -8,6 +8,7 @@
 8 - Stop the node and restart it with -reindex. Verify that the node has reindexed up to block 3.
 9 - Stop the node and restart it with -reindex-chainstate. Verify that the node has reindexed up to block 3.
10 - Verify that out-of-order blocks are correctly processed, see LoadExternalBlockFile()
11+- Start a second node, generate blocks, then restart with -reindex after setting blk files to read-only

LarryRuane commented at 7:15 am on February 8, 2023:

If you need to retouch, maybe expand this comment to explain why two nodes are needed.

pinheadmz commented at 3:46 pm on February 8, 2023:

I think I added a second node to the test so I could start a clean chain using smaller blk files (-fastprune) without bugging the existing tests that switch a few options on and off throughout.

pinheadmz commented at 7:50 pm on February 8, 2023:

added comment to top of the test where second node is set up

LarryRuane commented at 7:16 am on February 8, 2023: contributor

crtACK 386faaf76ea0edb2150725c5bdb69d234a2f2607 I’m still not able to figure out why two nodes are needed in the test. I tried modifying the test to use just one node, but the new test reindex_readonly(), didn’t pass (the log file contained the unexpected string “failed to open file”). So the second node does seem to be needed.

pinheadmz force-pushed on Feb 8, 2023

LarryRuane commented at 9:25 pm on February 8, 2023: contributor

ACK ed57565e36bf514fc4d063972464c9152a08fb8e

in src/node/blockstorage.cpp:773 in ed57565e36 outdated

617@@ -618,8 +618,8 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
618     if ((int)nFile != m_last_blockfile) {
619         if (!fKnown) {
620             LogPrint(BCLog::BLOCKSTORE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString());
621+            FlushBlockFile(/*fFinalize=*/true, finalize_undo);

jonatack commented at 9:09 pm on February 20, 2023:

The new test fails on master as expected, because the block file is read-only and can’t be written to.

It may be reassuring to have coverage for the other side of this change, to be sure we’re still flushing when expected. If this new line is removed and no flushing is done, all the functional and unit tests still pass (I didn’t run the extended functional tests).

0@@ -618,7 +618,6 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
1     if ((int)nFile != m_last_blockfile) {
2         if (!fKnown) {
3             LogPrint(BCLog::BLOCKSTORE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString());
4-            FlushBlockFile(/*fFinalize=*/true, finalize_undo);
5         }
6         m_last_blockfile = nFile;
7     }

in src/node/blockstorage.cpp:806 in ed57565e36 outdated

802@@ -803,7 +803,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CCha
803 {
804     unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
805     FlatFilePos blockPos;
806-    const auto position_known {dbp != nullptr};
807+    const bool position_known {dbp != nullptr};

jonatack commented at 9:10 pm on February 20, 2023:

nit, fix clang-format since you touch this line

0    const bool position_known{dbp != nullptr};

Edit, while touching up nits in the commit, maybe hit this one, too (feel free to ignore):

0--- a/src/node/blockstorage.cpp
1+++ b/src/node/blockstorage.cpp
2@@ -566,7 +566,7 @@ void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune)
3 
4 static FlatFileSeq BlockFileSeq()
5 {
6-    return FlatFileSeq(gArgs.GetBlocksDirPath(), "blk", gArgs.GetBoolArg("-fastprune", false) ? 0x4000 /* 16kb */ : BLOCKFILE_CHUNK_SIZE);
7+    return FlatFileSeq(gArgs.GetBlocksDirPath(), "blk", gArgs.GetBoolArg("-fastprune", false) ? 0x4000 /* 16KiB */ : BLOCKFILE_CHUNK_SIZE);
8 }

in test/functional/feature_reindex.py:84 in ed57565e36 outdated

81 
82+    def reindex_readonly(self):
83+        self.connect_nodes(0, 1)
84+        # generate enough blocks to ensure that the -fastprune node fills up the
85+        # first blk00000.dat file and starts another block file
86+        self.generatetoaddress(self.nodes[1], 300, self.nodes[1].get_deterministic_priv_key().address)

jonatack commented at 9:26 pm on February 20, 2023:

Adding some logging shows that this generate call is the slowest part of the added test. Questions:

Why 300 blocks? On my laptop the test passes reliably with 239 blocks and fails reliably with 238. Fewer blocks = faster test runs.
Can you explain how the number of blocks used was arrived at, and document that calculation along with the other magic numbers?

 0@@ -29,7 +29,7 @@ class ReindexTest(BitcoinTestFramework):
 1     def reindex(self, justchainstate=False):
 2         self.generatetoaddress(self.nodes[0], 3, self.nodes[0].get_deterministic_priv_key().address)
 3         blockcount = self.nodes[0].getblockcount()
 4-        self.restart_node(0, ["-reindex-chainstate" if justchainstate else "-reindex"])
 5+        self.restart_node(0, extra_args=["-reindex-chainstate" if justchainstate else "-reindex"])
 6         self.connect_nodes(0, 1)
 7         assert_equal(self.nodes[0].getblockcount(), blockcount)  # start_node is blocking on reindex
 8         self.log.info("Success")
 9@@ -72,34 +72,36 @@ class ReindexTest(BitcoinTestFramework):
10             'LoadExternalBlockFile: Out of order block',
11             'LoadExternalBlockFile: Processing out of order child',
12         ]):
13-            self.start_node(0, ["-reindex"])
14+            self.start_node(0, extra_args=["-reindex"])
15 
16         # All blocks should be accepted and processed.
17         assert_equal(self.nodes[0].getblockcount(), 12)
18 
19     def reindex_readonly(self):
20         self.connect_nodes(0, 1)
21-        # generate enough blocks to ensure that the -fastprune node fills up the
22-        # first blk00000.dat file and starts another block file
23-        self.generatetoaddress(self.nodes[1], 300, self.nodes[1].get_deterministic_priv_key().address)
24+        block_count = self.nodes[1].getblockcount()
25+
26+        num_blocks_to_add = 239  # document how this is calculated...
27+        self.log.info(f"Generate {num_blocks_to_add} blocks to ensure the -fastprune node fills up the first blk00000.dat file and starts another block file")
28+        self.generatetoaddress(self.nodes[1], num_blocks_to_add, self.nodes[1].get_deterministic_priv_key().address)
29         self.stop_node(1)
30 
31         assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00000.dat')
32         assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00001.dat')
33 
34-        # make the first block file read-only
35-        path = self.nodes[1].chain_path / 'blocks' / 'blk00000.dat'
36-        os.chmod(path, 0o444)
37+        self.log.info("Make the first block file read-only")
38+        filename = self.nodes[1].chain_path / 'blocks' / 'blk00000.dat'
39+        os.chmod(filename, 0o444)
40 
41-        # restart and reindex the node with the read-only block file
42+        self.log.info("Restart the node and reindex with the read-only block file")
43         self.start_node(1, ["-reindex", "-fastprune"])
44-        assert_equal(self.nodes[1].getblockcount(), 312)
45+        assert_equal(self.nodes[1].getblockcount(), block_count + num_blocks_to_add)
46 
47-        # shut down this node, triggering disk flushes, but not to the read-only file
48+        self.log.info("Shut down this node, triggering disk flushes, but not to the read-only file")
49         with self.nodes[1].assert_debug_log(expected_msgs=['FlushStateToDisk'], unexpected_msgs=['failed to open file']):
50             self.stop_node(1)
51 
52-        self.log.info("Reindex read-only Success")
53+        self.log.info("Success of reindex read-only test")
54 
55     def run_test(self):
56         self.reindex(False)

pinheadmz commented at 4:06 pm on February 22, 2023:

Thanks Jon I believe I addressed all the style nits and reduced the number of generated blocks as requested in e7a0339b8c. Adding test coverage for flush to disk is next…

jonatack commented at 9:32 pm on February 20, 2023: contributor

Concept ACK

pinheadmz force-pushed on Feb 22, 2023

in test/functional/feature_reindex.py:102 in e7a0339b8c outdated

 99+
100+        self.generatetoaddress(self.nodes[1], blocks_needed, addr)
101+        self.stop_node(1)
102+
103+        assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00000.dat')
104+        assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00001.dat')

LarryRuane commented at 10:44 pm on February 22, 2023:

Some assertions are not checking that the PR’s changes are working correctly, they’re ensuring that the test is setting things up as expected, and sometimes that’s not obvious to readers. Sorry I didn’t think to mention this earlier, but if you retouch, consider adding a comment before these two lines to make it clear that all we’re doing here is verifying the test setup.

pinheadmz commented at 3:37 pm on February 23, 2023:

Ok good feedback. Yeah these asserts are more like sanity checks. Testing the test! Although they also fail early on certain code regressions for example if the -fastprune size is changed in the future.

in test/functional/feature_reindex.py:96 in e7a0339b8c outdated

93+
94+        # How many blocks do we need to roll over a new .blk file?
95+        block_count = self.nodes[1].getblockcount()
96+        fastprune_blockfile_size = 0x10000
97+        size_needed = fastprune_blockfile_size - (block_size * block_count)
98+        blocks_needed = size_needed // block_size

LarryRuane commented at 10:50 pm on February 22, 2023:

I like the way you’re calculating the number of blocks needed rather than hard-coding the number; I verified that this value (when I run the test) is 240, which matches, with a tiny bit of rounding, what @jonatack suggested.

LarryRuane commented at 10:54 pm on February 22, 2023: contributor

ACK e7a0339b8cb612088ca212c568c0499c0f02398b

pinheadmz commented at 3:20 pm on February 27, 2023: member

Rebased on master and prepended an extra commit with a unit test for BlockManager. The test passes on master and the branch without modification, hopefully illustrating that at least some behaviors are not affected.

It’s hard to test precisely for the branch’s change in a unit test because file-flushing is not something we can really predict or test for (sometimes it happens as soon as we fwrite()).

The actual error in issue #2039 comes from an attempt to OPEN a file with rw when the file is read-only:

https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/flatfile.cpp#L81-L86

…and this PR avoids the call to Flush() altogether when we know it is not necessary @jonatack lemme know if the new test is reassuring enough, or if you have advice for better coverage?

pinheadmz force-pushed on Feb 27, 2023

hebasto commented at 6:04 pm on February 27, 2023: member

Tested that this PR indeed fixes #2039:

on master (82793f1984911774b111117f2e81d5f3b0bbec68):

0$ chmod u-w ~/.bitcoin/signet/blocks/blk00003.dat
1$ ./src/bitcoind -signet -reindex
2...
32023-02-27T17:58:31Z Reindexing block file blk00003.dat...
42023-02-27T17:58:31Z Unable to open file /home/hebasto/.bitcoin/signet/blocks/blk00003.dat
52023-02-27T17:58:31Z ERROR: Flush: failed to open file 3
62023-02-27T17:58:31Z *** Flushing block file to disk failed. This is likely the result of an I/O error.
72023-02-27T17:58:31Z Error: A fatal internal error occurred, see debug.log for details
8Error: A fatal internal error occurred, see debug.log for details
9...

this PR (5afd6bf274cf50743dc345eb9c0520c1b64c8069)

0$ chmod u-w ~/.bitcoin/signet/blocks/blk00003.dat
1$ ./src/bitcoind -signet -reindex  # no errors

DrahtBot added the label Needs rebase on Feb 28, 2023

LarryRuane commented at 3:58 pm on February 28, 2023: contributor

re-ACK, although now rebase is needed

mzumsande commented at 4:20 pm on February 28, 2023: contributor

Concept ACK, will review more closely soon.

The added unit test seems to fail under Win64.

pinheadmz force-pushed on Mar 1, 2023

pinheadmz commented at 4:19 pm on March 1, 2023: member

force-push to e600eb442802ca7b23189c8ceee448730e22e75a:

rebased on master and fixed conflict
added 2-second sleep before calls to SaveBlockToDisk(). This is to ensure that if the file is modified, its modification timestamp will definitely be updated. This test was intermittent on Windows I think because FAT has a 2-second resolution for mtime values. 🤷‍♂️

in src/test/blockmanager_tests.cpp:134 in dc1c1a1ce8 outdated

128+    const fs::path file_path = node::GetBlockPosFilename(pos2);
129+    const fs::file_time_type time1 = fs::last_write_time(file_path);
130+
131+    // Ensure that if the file is modified its timestamp is updated,
132+    // even on Windows (FAT has 2 second resolution)
133+    std::this_thread::sleep_for(std::chrono::milliseconds(2000));

mzumsande commented at 4:34 pm on March 1, 2023:

if this is only necessary for windows, would it be possible to hide it behind #ifdef WIN32 so that the unit tests don’t run 3*2=6 seconds longer for everyone else (which is quite a lot)?

pinheadmz commented at 8:51 pm on March 1, 2023:

Thanks, I guarded the sleep for WIN32, cleaned up the lint and mentioned the undo data in OP.

DrahtBot removed the label Needs rebase on Mar 1, 2023

jonatack commented at 5:55 pm on March 1, 2023: contributor

@pinheadmz Thanks for working on unit test coverage – will circle back soon.

mzumsande commented at 7:48 pm on March 1, 2023: contributor

Tested ACK e600eb442802ca7b23189c8ceee448730e22e75a

I feel like one small step is missing in the motivation why this is safe: FlushBlockFile() not only flushes the blk file, but possibly also the undo file (and does this no longer during reindex after this change), so it would be good to mention somewhere (e.g. in the OP) why this is ok. My understanding is that new undo data will only be available after connecting blocks to the chain, which only happens in the second part of the reindex where we rebuild the chain - so flushing in the first part would be a no-op anyway, and skipping it is ok.

Also, if you retouch, flake8 and Clang-format show a few minor nits for me in the added code.

pinheadmz force-pushed on Mar 1, 2023

in src/test/blockmanager_tests.cpp:106 in a3c25a4c8f outdated

101+    block2.nVersion = 2;
102+    CBlock block3;
103+    block3.nVersion = 3;
104+
105+    // They are 80 bytes header + 1 byte 0x00 for vtx length
106+    int TEST_BLOCK_SIZE = 81;

LarryRuane commented at 9:48 pm on March 1, 2023:

0    constexpr int TEST_BLOCK_SIZE{81};

in src/test/blockmanager_tests.cpp:112 in a3c25a4c8f outdated

107+
108+    // Blockstore is empty
109+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), 0);
110+
111+    // Write first block
112+    FlatFilePos pos1 = blockman.SaveBlockToDisk(block1, 1, chain, *params, nullptr);

LarryRuane commented at 9:54 pm on March 1, 2023:

0    // Write the first block; dbp=nullptr means this block doesn't already have a disk
1    // location, so allocate a free location and write it there.
2    FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1, chain, *params, /*dbp=*/nullptr)};

and similar below

in src/test/blockmanager_tests.cpp:137 in a3c25a4c8f outdated

132+    // Ensure that if the file is modified its timestamp is updated,
133+    // even on Windows (FAT has 2 second resolution)
134+    std::this_thread::sleep_for(std::chrono::milliseconds(2000));
135+#endif
136+
137+    // Attempt to save block 3 to original position of block 2

LarryRuane commented at 9:55 pm on March 1, 2023:

0    // Attempt, but fail, to save block 3 to original position of block 2

in src/test/blockmanager_tests.cpp:163 in a3c25a4c8f outdated

145+    node::ReadBlockFromDisk(read_block, pos2, params->GetConsensus());
146+    BOOST_CHECK_EQUAL(read_block.nVersion, 2);
147+
148+    // blk00000.dat was not modified
149+    const fs::file_time_type time2 = fs::last_write_time(file_path);
150+    BOOST_CHECK(time1 == time2);

LarryRuane commented at 11:06 pm on March 1, 2023:

Well, on non-windows, it may have been modified, but within the same second. This is kind of a minor point (probably don’t need to change anything), but if in the future there’s a bug such that blk00000.dat was modified, this test might not catch it. I verified this by hacking the test such that the file is modified, and about once in 10 or 20 runs, the timestamp didn’t change. (This is on Ubuntu.)

But I guess that’s okay, because if there is such a bug, it will likely be caught quickly. It’s just that I wish this test could be 100 percent solid. The only way I could think of to do that is to read and checksum the file, and compare the checksums instead of the timestamps. (Then you wouldn’t need that sleep for WIN32.) It wouldn’t be a performance problem, because block files are very small in this test.

Just something to consider, it may not be worth the trouble.

achow101 commented at 8:21 pm on April 21, 2023:

The only way I could think of to do that is to read and checksum the file, and compare the checksums instead of the timestamps.

Isn’t the point that the file contents aren’t changing? So checksum wouldn’t work because the files will be the same whether it’s rewritten or not. The only things that can be different are the filesystem’s metadata on the modification time, and these are not covered by a checksum.

pinheadmz commented at 8:57 pm on April 21, 2023:

The only things that can be different are the filesystem’s metadata on the modification time, and these are not covered by a checksum.

It might be the same usefulness actually, I seem to remember that if file contents aren’t changed, when you call fwrite() or fflush(), the modification timestamp won’t be updated. I researched different ways to detect a fflush() and this unit test was the closest. Most systems flush when fwrite() is called, and then a subsequent fflush() doesn’t do anything at all. The best I could do with this unit test was cover current behavior to ensure that the PR didn’t break it.

So actually it might be reasonable to just ditch the timestamp stuff altogether and just ensure that file contents aren’t modified.

The better test coverage is in the functional test which will throw on master when bitcoin tries to open the read-only file.

in src/test/blockmanager_tests.cpp:183 in a3c25a4c8f outdated

167+
168+    // Record last modified time of blk00000.dat now that it is full and finished
169+    const fs::file_time_type time3 = fs::last_write_time(file_path);
170+
171+    // Sanity check: blk00000.dat should have been modified writing the dummy blocks.
172+    BOOST_CHECK(time2 != time3);

LarryRuane commented at 11:19 pm on March 1, 2023:

This seems more problematic than what I mentioned above because this can cause false-positive. Is the resolution one second? Could all that happened since time2 was sampled be in the same second? But I ran the test many times and did not see the problem.

pinheadmz commented at 4:04 pm on March 2, 2023:

What I learned is that windows/FAT systems have 2-second resolution but Linux/EXT4 etc have ms or ns resolution. I had a hard time determining the best method for testing when a file is flushed but I think you’re right, using timestamps is just asking for intermittency. I think I wanted to avoid hashing files because it means opening the file while blockmanager might already have it open… but I think that’s going to be the best way to test.

pinheadmz commented at 4:26 pm on March 2, 2023:

ok trying another approach where we ALWAYS pause, to ensure incremental timestamps. But on windows we have to pause for 2 seconds. All other filesystems we only need to wait for 1 ms.

in src/node/blockstorage.cpp:624 in a3c25a4c8f outdated

620@@ -621,7 +621,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
621 
622     bool finalize_undo = false;
623     if (!fKnown) {
624-        while (m_blockfile_info[nFile].nSize + nAddSize >= (gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64kb */ : MAX_BLOCKFILE_SIZE)) {
625+        while (m_blockfile_info[nFile].nSize + nAddSize >= (gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64 KiB */ : MAX_BLOCKFILE_SIZE)) {

LarryRuane commented at 11:31 pm on March 1, 2023:

as long as you’re touching this line

0        const unsigned int max_size{gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64 KiB */ : MAX_BLOCKFILE_SIZE};
1        while (m_blockfile_info[nFile].nSize + nAddSize >= max_size) {

pinheadmz commented at 2:49 pm on April 26, 2023:

I’m going to revert this so it can be fixed in https://github.com/bitcoin/bitcoin/pull/27191

LarryRuane commented at 11:34 pm on March 1, 2023: contributor

Verified clang-format-diff. The unit test is nice! You may want to mention in the commit comment that it doesn’t test this PR (I was a little confused at first because I was expecting that it was).

pinheadmz force-pushed on Mar 2, 2023

pinheadmz commented at 4:36 pm on March 2, 2023: member

force-push to b4ce309ef5f2bed5ed1f861305317a0dca3dc5f4:

addressed all nits and review comments by @LarryRuane
added 1ms delay in blockmanager test for all non-windows systems (cc: @mzumsande )

LarryRuane commented at 4:47 pm on March 2, 2023: contributor

ACK b4ce309ef5f2bed5ed1f861305317a0dca3dc5f4 If you retouch, consider the suggestion for the unit test commit comment here: #27039#pullrequestreview-1320666918 (I think you may have just missed it), but it’s not important.

DrahtBot requested review from mzumsande on Mar 2, 2023

pinheadmz commented at 4:49 pm on March 2, 2023: member

consider the suggestion for the unit test commit comment here: #27039 (review) (I think you may have just missed it), but it’s not important.

ah doh! I added a sentence to the PR description but if I rebase again I’ll add to the actual commit

in src/test/blockmanager_tests.cpp:145 in 1658bc2f6f outdated

140+    const fs::file_time_type time1 = fs::last_write_time(file_path);
141+
142+    std::this_thread::sleep_for(pause);
143+
144+    // Attempt, but fail, to save block 3 to original position of block 2
145+    blockman.SaveBlockToDisk(block3, 3, chain, *params, &pos2);

mzumsande commented at 10:00 pm on March 10, 2023:

I don’t understand what this part of the test is testing - calling SaveBlockToDisk for a block at a specified but incorrect position is something that should never be possible in an actual, non-corrupted node unless I’m missing something. But if it somehow happened anyway, even if the existing block isn’t overwritten, I’d imagine that it would still cause some havoc, like changing block file statistics here for the added size of a block that is never actually written to disk, which would probably be bad.

pinheadmz commented at 11:36 am on March 11, 2023:

What I’m asserting here is that if a position is passed in, the function called “save block to disk” does not save a block to disk. By using incorrect data I can easily prove that it is not actually written to disk.
The call to AddBlock you highlight here confuses and concerns me a bit - since we know a known block won’t be written, why are we updating the metadata at all?

mzumsande commented at 2:20 pm on March 11, 2023:

2. The call to AddBlock you highlight here confuses and concerns me a bit - since we know a known block won’t be written, why are we updating the metadata at all?

Because the block index LevelDB database (saved in blocks/index) is being wiped at the beginning of a reindex and needs to be rebuilt from scratch as we look at each block. Part of this database is meta info about each blockfile (see bitcoin wiki). I think that’s the main reason why we have to call SaveBlockToDisk for each block during a reindex at all.

pinheadmz commented at 2:31 pm on March 11, 2023:

ok make sense thanks. do you think my first point justifies the method of the test or is that just confusing then and I should remove it?

mzumsande commented at 2:32 pm on March 14, 2023:

I think that both removing it and adding an explanatory comment would be ok. Maybe the test could also check that the block statistics (CBlockFileInfo) is not the same after the call (nBlocks should have increased)?

pinheadmz commented at 6:00 pm on March 14, 2023:

added in force-push to 5e579c6435145d84e43fb79224f2cb09b40ea047 and rebased on master

DrahtBot requested review from mzumsande on Mar 10, 2023

pinheadmz force-pushed on Mar 14, 2023

pinheadmz force-pushed on Mar 20, 2023

pinheadmz commented at 5:00 pm on March 20, 2023: member

force push to: 93c70287a6434c6c665a211dc4dfbbd9c3db4083

rebase on master
fix failing unit test

mzumsande commented at 3:57 pm on March 21, 2023: contributor

Code Review ACK 93c70287a6434c6c665a211dc4dfbbd9c3db4083

DrahtBot requested review from LarryRuane on Mar 21, 2023

LarryRuane commented at 6:22 pm on April 6, 2023: contributor

ACK 93c70287a6434c6c665a211dc4dfbbd9c3db4083

DrahtBot removed review request from LarryRuane on Apr 6, 2023

in src/test/blockmanager_tests.cpp:149 in 93c70287a6 outdated

144+    // When FlatFilePos* dbp is given, SaveBlockToDisk() will not write or
145+    // overwrite anything to the flat file block storage. It will, however,
146+    // update the blockfile metadata. This is to facilitate reindexing
147+    // when the user has the blocks on disk but the metadata is being rebuilt.
148+    CBlockFileInfo* block_data = blockman.GetBlockFileInfo(0);
149+    BOOST_CHECK(block_data->nBlocks == 2);

stickies-v commented at 1:12 pm on April 11, 2023:

nit: some inconsistent usage of BOOST_CHECK and BOOST_CHECK_EQUAL, I think using BOOST_CHECK_EQUAL here and in a few other places makes sense

in src/test/blockmanager_tests.cpp:100 in 93c70287a6 outdated

 95+    CChain chain{};
 96+
 97+    // Ensure (by delaying execution of certain steps)
 98+    // that if a file is modified its timestamp will be updated.
 99+#ifdef WIN32
100+    // FAT filesystem has write time resolution of 2 seconds

stickies-v commented at 12:40 pm on April 12, 2023:

Is this a reliable way to check for a FAT filesystem? WIN32 could also use other filesystems I think (e.g. NTFS), and likewise - FAT could still be used without WIN32, I think?

I think I agree with LarryRuane that comparing files is probably more robust and portable, which I think is reasonable with these small file sizes? But perhaps you’ve found this is too much of a performance hit?

You could also check the filesystem directly with statfs - orGetVolumeInformationA on Windows.

in src/test/blockmanager_tests.cpp:126 in 93c70287a6 outdated

121+    // Write the first block; dbp=nullptr means this block doesn't already have a disk
122+    // location, so allocate a free location and write it there.
123+    FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1, chain, *params, /*dbp=*/nullptr)};
124+
125+    // Write second block
126+    FlatFilePos pos2 = blockman.SaveBlockToDisk(block2, 2, chain, *params, nullptr);

turkycat commented at 2:00 am on April 16, 2023:

nit: use of assignment operator on line 126 while using initializer syntax for the same type on 123.

0    FlatFilePos pos2{blockman.SaveBlockToDisk(block2, /*nHeight=*2, chain, *params, /*dbp=*/nullptr)};

pinheadmz commented at 2:35 pm on April 17, 2023:

Thanks good eye. If I retouch I’ll fix this.

in src/test/blockmanager_tests.cpp:142 in 93c70287a6 outdated

137+
138+    // Record last modified time of blk00000.dat
139+    const fs::path file_path = node::GetBlockPosFilename(pos2);
140+    const fs::file_time_type time1 = fs::last_write_time(file_path);
141+
142+    std::this_thread::sleep_for(pause);

turkycat commented at 2:37 am on April 16, 2023:

can you explain why you’re sleeping after reading last_write_time? If I understand correctly, based on the other comments in this thread, the sleep is necessary because the file modified time may not be updated immediately after the contents are written. This test does pass on my linux machine (which has a significantly shorter resolution time), yet I feel that we should be sleeping immediately after the potential file write does/does not occur before checking the file time and verifying it? This comment applies to the other uses of sleep_for in this test.

pinheadmz commented at 2:40 pm on April 17, 2023:

This pause is intended to guarantee that time1 (L140) and time2 (L162) will be different if any file modification happens between the two readings. On windows for example with its 2 second resolution, all kinds of stuff could be happening to that file during those 22 lines and the timestamp would still not change (if no delay were present).

turkycat commented at 2:47 am on April 16, 2023: none

code review and local test run ACK

470ef396

(edited by @DrahtBot due to GitHub bug https://github.com/MarcoFalke/DrahtBot/issues/31)

DrahtBot requested review from turkycat on Apr 16, 2023

DrahtBot removed review request from turkycat on Apr 16, 2023

DrahtBot requested review from turkycat on Apr 16, 2023

DrahtBot removed review request from turkycat on Apr 16, 2023

DrahtBot requested review from turkycat on Apr 16, 2023

DrahtBot removed review request from turkycat on Apr 17, 2023

DrahtBot requested review from turkycat on Apr 17, 2023

DrahtBot removed review request from turkycat on Apr 17, 2023

DrahtBot requested review from turkycat on Apr 17, 2023

fanquake added this to the milestone 26.0 on Apr 17, 2023

pablomartin4btc commented at 3:43 pm on April 19, 2023: member

Concept ACK. (went thru notes and other related PRs mentioned on the Review club meeting preparation webpage - Please add label when possible)

DrahtBot removed review request from turkycat on Apr 19, 2023

DrahtBot requested review from turkycat on Apr 19, 2023

pinheadmz added the label Review club on Apr 19, 2023

DrahtBot removed review request from turkycat on Apr 19, 2023

DrahtBot requested review from turkycat on Apr 19, 2023

in src/test/blockmanager_tests.cpp:174 in 93c70287a6 outdated

169+
170+    std::this_thread::sleep_for(pause);
171+
172+    // Now fill up the first file to fastprune limit and start new block file
173+    CBlock dummy_block;
174+    FlatFilePos last_block;

stickies-v commented at 4:20 pm on April 19, 2023:

nit: unintuitive name, it’s not a block

0    FlatFilePos last_block_pos;

stickies-v commented at 3:30 pm on April 20, 2023: contributor

Approach ACK 93c70287a6434c6c665a211dc4dfbbd9c3db4083 (modulo tests)

Conceptually, this seems straightforward: we don’t want to update block files during reindex, so avoiding the flushing altogether is desireable.

I think the only potential for unexpected behaviour change is if there are places other than BlockManager::SaveBlockToDisk() where block files are modified (and potentially not flushed). Looking at the callsites of FlatFileSeq::Open() and all of its parent callsites, it seems like the only place where we open a block file with read_only == false is in BlockManager::SaveBlockToDisk(). From that, I think the concept and approach make sense and seem safe.

When it comes to the tests, I’m not yet convinced. I think relying on fs::last_write_time is fragile, and I don’t know if the test is useful enough to warrant this hacky approach. I understand this flushing behaviour is inherently difficult to test, but I’m not sure the current approach is worth the hassle?

DrahtBot removed review request from turkycat on Apr 20, 2023