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

[pinheadmz]

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:

init()
ThreadImport() <-- process fReindex flag
LoadExternalBlockFile()
AcceptBlock()
SaveBlockToDisk()
FindBlockPos()
FlushBlockFile() <-- 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]

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.

[LarryRuane]

Approach and code review ACK a6b05f8
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.

[LarryRuane]

crtACK 386faaf
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.

[LarryRuane]

ACK ed57565

[jonatack]

Concept ACK

[LarryRuane]

ACK e7a0339

[pinheadmz]

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:

bitcoin/src/flatfile.cpp

Lines 81 to 86 in 82793f1

	bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize)
	{
	FILE* file = Open(FlatFilePos(pos.nFile, 0)); // Avoid fseek to nPos
	if (!file) {
	return error("%s: failed to open file %d", __func__, pos.nFile);
	}

...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?

[hebasto]

Tested that this PR indeed fixes #2039:

• on master (82793f1):

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

• this PR (5afd6bf)

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

[LarryRuane]

re-ACK, although now rebase is needed

[mzumsande]

Concept ACK, will review more closely soon.

The added unit test seems to fail under Win64.

[pinheadmz]

force-push to e600eb4:

• 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. 🤷‍♂️

[furszy]

Code review ACK 8a1ab9a

[pinheadmz]

Thanks for the reviews, @mzumsande @furszy @pablomartin4btc @LarryRuane I just rebased on master to clean up CI

[furszy]

Rebase diff ACK dfcef53.

Please try to not rebase the PR when it is not needed. A not related CI failure, or an spurious one, will not block a PR from getting it merged.

[sipa]

utACK dfcef53

[mzumsande]

re-ACK dfcef53

[achow101]

ACK dfcef53

[maflcko]

Any reason to change this to the aggressive busy-loop helper? The previous should work just fine with the right timeout. Also, I think wait_for_debug_log should be renamed to busy_wait_for_debug_log

[pinheadmz]

I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.

[maflcko]

wait_for_debug_log (the variant of the function that accepts raw bytes) does not have a sleep, so it will try to maxx out IO at 100%, no?

[pinheadmz]

OK I see now thanks. So I guess... start_node calls wait_for_rpc_connection. Is RPC available before reindexing is finished? If we can rely on that to prevent race conditions then this could just be assert_debug_log.

[maflcko]

If we can rely on that to prevent race conditions then this could just be assert_debug_log.

assert_debug_log is also syncing (it has a timeout argument), so no race should happen. My comment is only about the IO usage. assert_debug_log is not using a busy wait, but a sleepy wait. Otherwise they are exactly identical.

However, if assert_debug_log is used to sync, my personal preference is to provide the timeout argument, so that the code is self-documenting.

I think in this context my feedback is mostly of stylistic nature, but I could imagine other scenarios where a more expensive reindex in combination with the busy wait may lead to test timeouts on slow storage.

[pinheadmz]

Got it, thanks. Worth opening a new PR?

[maflcko]

If you want, yes, I am happy to review. Though, I won't create a pull myself.