blockstorage: do not flush block to disk if it is already there by pinheadmz · Pull Request #27039 · bitcoin/bitcoin

pinheadmz · 2023-02-03T18:50:50Z

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 · 2023-02-03T18:50:53Z

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

LarryRuane

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.

LarryRuane

ACK ed57565e36bf514fc4d063972464c9152a08fb8e

jonatack

Concept ACK

LarryRuane

ACK e7a0339b8cb612088ca212c568c0499c0f02398b

pinheadmz · 2023-02-27T15:20:39Z

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 (5afd6bf274cf50743dc345eb9c0520c1b64c8069)

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

LarryRuane · 2023-02-28T15:58:17Z

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 · 2023-03-01T16:19:54Z

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

pinheadmz · 2023-07-10T13:48:08Z

@ryanofsky separate tests PR is ready for review: #27850

maflcko · 2023-09-05T11:26:36Z

I think the milestone (#27039 (comment)) can be removed, given that this still needs rebase and is not a regression, or is it?

maflcko · 2023-09-12T11:10:42Z

Removed milestone. Let me know if it should be re-added.

pinheadmz · 2023-09-15T19:12:16Z

Rebased on master now that #27850 is merged. This PR becomes very slim, one line of code changed and switching the test from expected fail to expected pass.

mzumsande · 2023-09-15T23:56:10Z

Should squash the two commits so that there isn't a commit for which the CI fails.

pinheadmz · 2023-09-16T14:08:38Z

Should squash the two commits so that there isn't a commit for which the CI fails.

done thanks

furszy

Not blocking if others are happy with the current state.

I would prefer to split up FindBlockPos() prior to introducing any specific performance improvement. This method mixes two different functionalities with zero documentation and a misleading name. I think that we could move forward faster if the function wouldn't be that obscure.

mzumsande · 2023-09-16T14:36:01Z

I would prefer to split up FindBlockPos() prior to introducing any specific performance improvement.

I have an old branch for that (linked in the OP), was originally planning to PR it once this gets merged.

maflcko · 2023-09-18T09:00:01Z

nit (only if you re-touch): I think you can just use self.start_node(0, ...)

pinheadmz · 2023-10-24T18:18:38Z

Rebased on master to fix conflicts and address test changes in the followup #28660

Might be good to get review from @TheCharlatan because this PR touches code just updated in #27866

mzumsande

ACK 8a1ab9aedb6b2ba8222a62d5513b9b0bc68324bd

Getting some chattr warnings, but these are independent of the changes here, so I'll report them in #28660.

furszy

Code review ACK 8a1ab9aed

test: ensure we can reindex from read-only block files now

pinheadmz · 2024-03-12T16:46:57Z

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 · 2024-03-17T15:24:49Z

utACK dfcef53

mzumsande · 2024-03-18T15:15:41Z

re-ACK dfcef53

achow101 · 2024-03-20T16:25:24Z

ACK dfcef53

maflcko · 2024-03-20T18:13:42Z

-                self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
-                    expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+            self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
+            with self.nodes[0].wait_for_debug_log([b"Reindexing finished"]):


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

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.

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?

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.

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.

Got it, thanks. Worth opening a new PR?

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

DrahtBot added the Block storage label Feb 3, 2023

brunoerg reviewed Feb 3, 2023

View reviewed changes

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the reindex-read-only branch from 7a2e5cd to a8db898 Compare February 3, 2023 18:59

brunoerg reviewed Feb 3, 2023

View reviewed changes

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the reindex-read-only branch from a8db898 to a6b05f8 Compare February 3, 2023 21:15

LarryRuane reviewed Feb 4, 2023

View reviewed changes

Comment thread test/functional/feature_reindex.py Outdated

Comment thread test/functional/feature_reindex.py Outdated

Comment thread test/functional/feature_reindex.py Outdated

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the reindex-read-only branch from a6b05f8 to 386faaf Compare February 4, 2023 12:15

pinheadmz mentioned this pull request Feb 7, 2023

-reindex fails if blk0*.dat files read-only #2039

Closed

LarryRuane reviewed Feb 8, 2023

View reviewed changes

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the reindex-read-only branch from 386faaf to ed57565 Compare February 8, 2023 19:50

LarryRuane reviewed Feb 8, 2023

View reviewed changes

jonatack reviewed Feb 20, 2023

View reviewed changes

Comment thread src/node/blockstorage.cpp Outdated

Comment thread src/node/blockstorage.cpp Outdated

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the reindex-read-only branch 2 times, most recently from ec7dab6 to e7a0339 Compare February 22, 2023 16:08

DrahtBot mentioned this pull request Feb 22, 2023

refactor, kernel: Decouple ArgsManager from blockstorage #27125

Merged

LarryRuane reviewed Feb 22, 2023

View reviewed changes

Comment thread test/functional/feature_reindex.py Outdated

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the reindex-read-only branch 2 times, most recently from b787dac to 5afd6bf Compare February 27, 2023 16:21

hebasto reviewed Feb 27, 2023

View reviewed changes

LarryRuane mentioned this pull request Feb 28, 2023

revive 26535 - reindex: allow finalized blk files to use read-only permissions LarryRuane/bitcoin#6

Open

DrahtBot mentioned this pull request Feb 28, 2023

prune: scan and unlink already pruned block files on startup #26533

Merged

DrahtBot added the Needs rebase label Feb 28, 2023

mzumsande reviewed Feb 28, 2023

View reviewed changes

pinheadmz force-pushed the reindex-read-only branch 2 times, most recently from a9a9c86 to e600eb4 Compare March 1, 2023 15:22

DrahtBot removed the Needs rebase label Mar 1, 2023

DrahtBot requested review from LarryRuane and removed request for LarryRuane June 29, 2023 23:56

DrahtBot mentioned this pull request Sep 15, 2023

blockstorage: Return on fatal flush errors #27866

Merged

furszy reviewed Sep 16, 2023

View reviewed changes

maflcko reviewed Sep 18, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 21, 2023

validation: assumeutxo params for testnet and signet #28516

Closed

mzumsande reviewed Oct 25, 2023

View reviewed changes

This was referenced Jan 11, 2024

logging: Update to new logging API #29231

Closed

log, refactor: Allow log macros to accept context arguments #29256

Draft

furszy reviewed Mar 11, 2024

View reviewed changes

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

dfcef53

test: ensure we can reindex from read-only block files now

DrahtBot mentioned this pull request Mar 13, 2024

kernel: Handle fatal errors through return values #29642

Closed

furszy reviewed Mar 16, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 18, 2024

validation: Make translations of fatal errors consistent #29672

Merged

maflcko reviewed Mar 20, 2024

View reviewed changes

mzumsande mentioned this pull request Apr 26, 2024

blockstorage: Separate reindexing from saving new blocks #29975

Merged

pinheadmz mentioned this pull request Apr 30, 2024

test: use sleepy wait-for-log in reindex readonly #30006

Merged

Uh oh!

Conversation

pinheadmz commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

LarryRuane left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pinheadmz commented Feb 27, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane commented Feb 28, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Mar 1, 2023

Uh oh!

pinheadmz commented Jul 10, 2023

Uh oh!

maflcko commented Sep 5, 2023

Uh oh!

maflcko commented Sep 12, 2023

Uh oh!

pinheadmz commented Sep 15, 2023

Uh oh!

mzumsande commented Sep 15, 2023

Uh oh!

pinheadmz commented Sep 16, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Sep 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Oct 24, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Mar 12, 2024

Uh oh!

pinheadmz commented Feb 3, 2023 •

edited

Loading

DrahtBot commented Feb 3, 2023 •

edited

Loading

LarryRuane left a comment •

edited

Loading