Fix OpenDiskFile's fReadOnly flag to work, as well as other problems importing read-only block files #2042

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:bugfix_open_readonly changing 1 files +10 −6

luke-jr commented at 9:48 PM on November 26, 2012: member

OpenDiskFile was always opening files in "rb+" mode (read-write binary), and if that failed and fReadOnly was false, trying to truncate the file. Truncate would always fail (AFAIK) unless a race condition set the file read-write between the two attempts.

This changes it to actually honour fReadOnly. The truncate race ''is not'' fixed, and cannot easily be fixed (fopen lacks a "open read-write, random access, but don't truncate" mode). If we want to actually try truncating in !fReadOnly mode, I'm pretty sure we need to unlink first.

This bug was introduced as part of ultraprune, so does not affect any released versions.
Bugfix: Actually open read-only when fReadOnly is set, and never try to truncate files if that fails 52ee934c54
Bugfix: Avoid trying to flush block file when importing ec2a8eedfc
laanwj commented at 7:30 AM on November 27, 2012: member

Need rebase already?!
luke-jr commented at 12:00 PM on November 27, 2012: member

Oops yeah, I'll fix that in a bit. (I built it on top of ultraprune's branch)
BitcoinPullTester commented at 5:48 PM on November 27, 2012: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/ec2a8eedfc8fd37f340bc6a434d64c87eae0b5b4 for test log.

This pull does not merge cleanly onto current master
in src/main.cpp:None in ec2a8eedfc
```
1509 | @@ -1510,10 +1510,14 @@ void static FlushBlockFile()
1510 |      posOld.nPos = 0;
1511 |  
1512 |      FILE *fileOld = OpenBlockFile(posOld);
1513 | -    FileCommit(fileOld);
1514 | -    fclose(fileOld);
1515 | +    if (fileOld)
```
sipa commented at 8:40 PM on November 27, 2012:

If opening the block file fails, there is something seriously wrong, and I think we should raise a fatal error or so.

Diapolo commented at 9:54 PM on November 27, 2012:

Yes, that should be the way to take here.
in src/main.cpp:None in ec2a8eedfc
```
1724 | @@ -1721,7 +1725,9 @@ bool SetBestChain(CBlockIndex* pindexNew)
1725 |      // Make sure it's successfully written to disk before changing memory structure
1726 |      bool fIsInitialDownload = IsInitialBlockDownload();
1727 |      if (!fIsInitialDownload || view.GetCacheSize()>5000) {
1728 | -        FlushBlockFile();
1729 | +        if (!fImporting)
```
sipa commented at 8:41 PM on November 27, 2012:

Importing is not read-only. Only reindexing is.

sipa commented at 10:25 PM on November 30, 2012:

Actually, the only sane way is keeping a fBlockDirty flag or so, setting it to true when a block is written, and then inside FlushBlockFile only sync the block file part if that flag is true (the undo file part should always be synced).
BitcoinPullTester commented at 4:41 PM on January 24, 2013: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/ec2a8eedfc8fd37f340bc6a434d64c87eae0b5b4 for test log.

This pull does not merge cleanly onto current master
luke-jr commented at 4:54 PM on January 30, 2013: member

I think I was asleep when I wrote this. The fReadOnly bug is real, but isn't fixed correctly in this case and only matters for read-only blk*.dat files, which have other problems not addressed herein. When/if I take the time to correctly get read-only blk files working, I'll open a new pullreq..
luke-jr closed this on Jan 30, 2013
sidhujag referenced this in commit f62e657a70 on Apr 23, 2018
HashUnlimited referenced this in commit 718b6cccce on May 11, 2018
DrahtBot locked this on Sep 8, 2021

Contributors