The final PR for this is in. See: https://github.com/bitcoin/bitcoin/pull/3346

Ready for final review and testing. It's merge-ready barring any new reports of build problems.

Since you said you're happy to send along the bounty (when merged), I have a proposal for you:
As mentioned above, I'm not ok with keeping it because IMO it was a regression that I introduced anyway. Instead, I'd like to see it donated to Sean's Outpost (http://seansoutpost.com).

However, given that they are working hard to help families for Christmas, I'd really hoped to have this taken care in time to contribute.

Could I persuade you to go ahead and send along half to Sean's Outpost, and the rest after merge? I obviously have every intention of getting this into master.

After thinking this over a bit more, and seeing new crash data from toffoo, I believe there's still a bit more to fix.

From IRC:
<cfields> the issue is that while it's looping through the write, it unmaps/remaps. and if there's a write that straddles a mapping, it can be split and not flushed to disk properly by osx
<cfields> robert's patch fixes that part
<cfields> however, once the write is complete, it's not immediately unmapped
<cfields> so at that point, the very tail may not be flushed

So the scenario would look like this:
- Append a new record that straddles the write boundary. Let it return from a successful Append() and don't write any new data (this is the distinction)
- Kill -9 or pull the plug before Close()
- Robert's change has caused the interim data in Append() to be flushed to disk, however the tail will not be unmapped until Close()

I've pushed a proof-of-concept "fix" here: https://github.com/theuni/bitcoin/commit/1d0e54bfb76b3891468582df97c4429174063c3c
This is just a copy/paste of what happens in Sync(), except that it's forced at the end of each logical write. Note however that Sync() first flushes to disk, so it would be sending possibly unsync'd data.

Though I hate to admit defeat (I had nailed down the issue, but my approach to fix was incorrect), Robert Escriva has submitted the correct fix: https://groups.google.com/forum/#!topic/leveldb/GXhx8YvFiig
It probably didn't hurt that he's a leveldb consultant

His victory speech: http://hackingdistributed.com/2013/11/27/bitcoin-leveldb/

I'd like to thank Robert for diving into the issue. He's got my +1 for the bounty.

As discussed on IRC, I don't think my first change is enough.

I've poked around and now I can reproduce the problem at-will by injecting some sleeps in key places, basically magnifying an unlikely race condition. I'll post my findings and some patches tomorrow, once I can make a bit more sense of it.

Thanks Mike. Though I must point out that my summation above is a theory, and certainly shouldn't be taken as fact. But the premise is basically this: We can't really know if the GCC idiom is supported on Apple's compiler-of-the-week, but it seems we can be sure that OSMemoryBarrier will work as intended, so there should be no harm in switching to it.

Upon further investigation, this does indeed seem to be unintended in leveldb.

Their atomics are borrowed from here (see the note in port/atomic_pointer.h): https://code.google.com/p/gperftools/source/browse/src/base/atomicops.h#102

It's clear there that mach/apple takes precedence over x86. The patch above fixes leveldb to match that behavior. Regardless of whether or not this fixes the db corruption for bitcoin, I'll take it upstream.

In the current code, OSX falls into this this case:
(Note that clang also defines __GNUC__)

I believe that's unintentional. Moving the "#elif defined(OS_MACOSX)" up causes the barrier to be rather than

I have a theory, and a binary to try out for someone who can reproduce. I don't see the issue on my macbook.

Patch is here: https://github.com/theuni/bitcoin/commit/ac0d918bb49e0f8393b1e27662bc4c25113980b2
Binary is here for anyone who trusts me (you can, but of course you shouldn't) https://www.dropbox.com/s/r959wns6rm4xjsu/Bitcoin-Qt.dmg

Theory goes like this:

Currently leveldb's atomics use a gcc-style compiler barrier like:
asm volatile("" : : : "memory");

OSX has a native memory barrier that can be used instead. It's my understanding that on x86/x86_64, a real memory barrier is not needed, but only a flag to tell the compiler not to optimize+reorder store/loads. Looks to me like it's a mistake, and that the OSX function should be chosen instead.

In my tests, the asm hack was often completely optimized away, and loads/stores were reordered anyway. So I believe the barrier in current code is effectively a nop.

Since this is a build-time optim and not run-time, it does not explain why 10.8 is affected while 10.6 is. However, if atomics are busted, timings in other underlying functions (write/fsync/mmap/etc) would have a huge effect, so it seems reasonable that this could be the indirect cause.

This is on my todo list (I did the initial autotools port).

Because it's a regression, I consider this part of the port that was not completed. Thus, no bounty needed, I'll take care of it very soon. I have most of it done in a branch somewhere, just need to finish up my current project first.