MacOSX leveldb corruption fix #3327

1. 
   laanwj commented at 11:58 am on November 28, 2013: member

   Here’s the gist of what’s going wrong:

   1. PosixMmapFile is used for performing sequential writes. Internally, it maps regions of the file one after the other. Each region is unmapped before the next region is mapped.
   2. In some circumstances on OS X, the unmapped data is not written to disk.

   The fix is to perform a msync before the munmap on OS X, which seems to be where this issue manifests itself most.

   Note: Simply performing a write with the `sync’ option is not a sufficient fix because the sync will happen after the munmap.

   See also: http://hackingdistributed.com/2013/11/27/bitcoin-leveldb/

2. Fix LevelDB 197 on OS X

   Here's the gist of what's going wrong:
   
   1.  PosixMmapFile is used for performing sequential writes.  Internally, it maps
       regions of the file one after the other.  Each region is unmapped before the
       next region is mapped.
   
   2.  In some circumstances on OS X, the unmapped data is not written to disk.
   
   The fix is to perform a msync before the munmap on OS X, which seems to be where
   this issue manifests itself most.
   
   Note:  Simply performing a write with the `sync' option is not a sufficient fix
       because the sync will happen after the munmap.

   d9fb93da74
3. 
   wtogami commented at 12:17 pm on November 28, 2013: contributor

   https://bitcointalk.org/index.php?topic=337294.0 Binaries of 0.8.5 containing this patch are available from this thread.
4. 
   BitcoinPullTester commented at 12:28 pm on November 28, 2013: none

   Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d9fb93da74b3e76871455feb649ea3157b7b9107 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
5. 
   sipa commented at 1:50 pm on November 28, 2013: member

   Judging from the discussion on the leveldb issue page, this is not considered a full fix, and likely not what will be merged upstream (Sanjay hinted at dropping mmap entirely even, it seems).

   I’m fine with this change in 0.8.6 only as a quick fix, but it should probably go into the LevelDB repo in a side branch there first.

6. 
   laanwj commented at 4:49 pm on November 28, 2013: member

   Right, if this fixes the MacOS corruption then it’s going to make a lot of people happy to upgrade to 0.8.6.

   As soon as someone implements a better solution we can of course switch to that, and for master it may be preferable to wait for that…

7. 
   theuni commented at 5:42 pm on November 28, 2013: member

   ACK. This is a full fix in that it fixes the source of the corruption. It wouldn’t make sense to leave this unpatched in master or release branch now that it’s known and understood. I’d say it should be pulled in as-is, and if upstream goes with a new implementation instead, we can swap it out at that point.
8. 
   wtogami commented at 5:08 am on November 30, 2013: contributor

   https://code.google.com/p/leveldb/issues/detail?id=197 This is the associated upstream ticket? Robert’s patch is not mentioned.

   #2770 (comment) @toffoo had a crash with Robert’s patch.

   https://bitcointalk.org/index.php?topic=337294.msg3775189#msg3775189 cfields found another issue that is likely related and another patch.

   https://bitcointalk.org/index.php?topic=337294.msg3775451#msg3775451 New build including both patches

9. 
   rescrv commented at 8:12 pm on November 30, 2013: none

   The upstream ticket has been open for a few months. Sanjay will likely not close it until the next release of LevelDB which will contain a fix of some sort.

   The scenario cfields describes is a non-issue. He claims that we won’t unmap the data for the second half of a write. Unmapping the data is not required for any of LevelDB’s correctness invariants. The patch he suggests is effectively the same as setting the sync=true option for writes (which you are already doing).

10. 
    wtogami commented at 6:47 am on December 3, 2013: contributor

    https://bitcointalk.org/index.php?topic=337294.msg3718821#msg3718821 Two separate Bitcoin builds are available for testing.

    • mactest1 build contains @rescrv’s #3327 and follows https://github.com/bitcoin/bitcoin/commits/0.8.6
    • OMG8 contains #3340 and follows https://github.com/litecoin-project/bitcoinomg/commits/0.8.5-OMG8
11. 
    laanwj commented at 8:14 am on December 5, 2013: member

    Closing in favor of not using mmap at all on MacOSX (#3340)
12. laanwj closed this on Dec 5, 2013

    ---

13. Bushstar referenced this in commit 2c30818f7b on Apr 8, 2020
14. Bushstar referenced this in commit 9145080d07 on Apr 8, 2020
15. DrahtBot locked this on Sep 8, 2021

Contributors
laanwj wtogami BitcoinPullTester sipa theuni rescrv