util: detect and warn when using exFAT on MacOS

willcl-ark commented at 10:29 am on December 10, 2024: member

exFAT is known to cause intermittent corruption on MacOS.

Therefore we should warn when using this fs format for either the blocks or data directories.

See #28552 for more context.

DrahtBot commented at 10:30 am on December 10, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31453.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3
Concept ACK	l0rinc, laanwj, sipa, helloscoopa, hebasto, edilmedeiros, davidgumberg, Zero-1729
Stale ACK	marcofleon

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:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot added the label Utils/log/libs on Dec 10, 2024

willcl-ark commented at 10:32 am on December 10, 2024: member

I did toy with having this trigger a full on InitError, but the behaviour is intermittent and seems worst during IBD. Exiting would also likely break things for a fair few users, due to exFAT being used in cross-platform storage devices. Therefore a warning seems more apt.

The root cause seems to be that some calls to F_FULLFSYNC fcntl silently failing on exFAT. This appears to stem from the fact that, whilst supported, F_FULLFSYNC may not be fully implemented for exFAT:

F_FULLFSYNC Does the same thing as fsync(2) then asks the drive to flush all buffered data to the permanent storage device (arg is ignored). This is currently imple-mented implemented mented on HFS, MS-DOS (FAT), and Universal Disk Format (UDF) file systems. The operation may take quite a while to complete. Certain FireWire drives have also been known to ignore the request to flush their buffered data.

link: https://newosxbook.com/src.jl?tree=xnu&file=/bsd/man/man2/fcntl.2 and: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fcntl.2.html#//apple_ref/doc/man/2/fcntl

From debugging this seems to manifest from fast processing speed; if I add enough LogPrintf’s then it eventually goes away, presumably due to the reduced IBD speed. This may also indicate that users on slower (CPU) machines may suffer from this less frequently.

We do read and write to the block files from the correct position/offsets at failure but the wrong data is found there. So this appears to be an issue at the kernel level.

I will open a new issue with some outputs from my debugging on this.

in src/common/init.cpp:81 in a7d52d023e outdated

76+        }};
77+        for (const auto& check : paths_to_check) {
78+            FSType fs_type = GetFilesystemType(check.path);
79+            switch(fs_type) {
80+                case FSType::EXFAT:
81+                    LogWarning("Specified %s \"%s\" is exFAT which is known to have intermittent corruption problems on MacOS.", check.description, fs::PathToString(check.path));

maflcko commented at 10:48 am on December 10, 2024:

Shouldn’t this be an InitWarning? Otherwise gui users (basically everyone on macos?) will never see this on their screen, unless they dig up the debug log at the right position?

willcl-ark commented at 11:43 am on December 10, 2024:

Thanks for catching marco. It was indeed a mistake to change this from an InitError in my first draft to LogWarning.

Will fixup shortly

willcl-ark commented at 10:46 am on December 11, 2024:

Done in 83839a35f07055dd03924b5bdab46bf31df33b35

in src/common/init.cpp:66 in a7d52d023e outdated

62@@ -62,6 +63,32 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
63             fs::create_directories(net_path / "wallets");
64         }
65 
66+        // Warn if we are trying to put the datadir on an exFAT fs on MacOS
67+        // This is an upstream issue known to cause bugs, see #28552
68+#ifdef __APPLE__

l0rinc commented at 11:15 am on December 10, 2024:

Q: is this related to a macs or just generally using flash drives as storage?

willcl-ark commented at 11:19 am on December 10, 2024:

I haven’t encountered the same error using this drive to sync when formatted with APFS, so I in my opinion, no.

I have come across comments online purporting that certain brands of nvme drives don’t play well with apple’s (internal) nvme controllers, which can also cause similar issues. But here I feel pretty certain this is an exFAT x MacOS issue, and we may well see the same issues with an internal exFAT drive.

If anyone has a mac with an internal sata/nvme drive which they could format to exFAT and test IBD with, that would be great! (I haven’t)

willcl-ark commented at 11:20 am on December 10, 2024:

Oh I should add here, (and probably in #31454) that I tested this same drive using exFAT on Linux, and did not see any corruption to block 450k. By which point every MacOS test had corrupted some data and failed.

edilmedeiros commented at 5:32 pm on January 3, 2025:

If my memory is not failing me, I have seem only macOS related issues reporting problems with exFAT external drivers. I tried exFAT on the internal drive of an old 2016 Intel Macbook Pro (original 1GB SSD, APFS on a primary partition and exFAT on a secondary partition for Bitcoin data) and IBD failed on v26. It was about a year ago, I was trying to understand if it was a problem with the external drive I was using at the time or a software problem. Unfortunately, I can’t try that again with that computer now.

marcofleon approved

marcofleon commented at 11:35 am on December 10, 2024: contributor

utACK a7d52d023eee6559d04e2e191f17176f2fe92c00. This happened to me earlier this year during IBD using a mac M3 and an external SSD formatted by default as exFAT. Took me a bit to figure it out, so a warning would’ve helped.

Reformatting to APFS solved the issue for me. Would that be worth adding to the warning as a suggestion for a solution?

willcl-ark commented at 11:38 am on December 10, 2024: member

Reformatting to APFS solved the issue for me. Would that be worth adding to the warning as a suggestion for a solution?

Yes that could be nice. Another idea that I had suggested was linking to a help page. Perhaps I could dd a section to https://github.com/bitcoin/bitcoin/blob/master/doc/files.md describing the problematic FS/OS combination and add the url to the warning to make things super-easy for users to fix themselves?

cc @fanquake

laanwj commented at 12:13 pm on December 10, 2024: member

Concept ACK, i think this makes sense in general. exFAT is meant for storing photos, not as a mature filesystem for database workloads, so subpar reliability and performance are to be expected.

l0rinc commented at 2:10 pm on December 10, 2024: contributor

Concept ACK

willcl-ark force-pushed on Dec 10, 2024

sipa commented at 4:19 pm on December 10, 2024: member

Concept ACK

willcl-ark force-pushed on Dec 10, 2024

DrahtBot added the label CI failed on Dec 10, 2024

DrahtBot commented at 4:47 pm on December 10, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34204028171

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Dec 10, 2024

marcofleon commented at 11:39 am on December 11, 2024: contributor

Nice, the warning and new addition to the docs lgtm.

ACK ee05e730466f250e131eb92165bcdb8bab68c864

DrahtBot requested review from laanwj on Dec 11, 2024

DrahtBot requested review from l0rinc on Dec 11, 2024

DrahtBot requested review from sipa on Dec 11, 2024

helloscoopa commented at 12:09 pm on December 11, 2024: none

Concept ACK

hebasto commented at 10:36 am on December 12, 2024: member

Concept ACK.

in src/common/init.cpp:75 in ee05e73046 outdated

72+            std::string_view description;
73+        };
74+        std::array<PathCheck, 2> paths_to_check{{
75+            {args.GetDataDirNet(), "data directory"},
76+            {args.GetBlocksDirPath(), "blocks directory"}
77+        }};

tdb3 commented at 2:27 am on December 15, 2024:

Makes sense to cover blocksdir and datadir. Since the user could also set -walletdir=<dir> when starting bitcoind, do you think it would be worth warning for it as well? Much lighter disk usage than blocksdir and datadir, but if we’re doubtful of macOS’s exFAT implementation, then maybe we would want to warn the user when storing wallets with it?

edilmedeiros commented at 5:14 pm on January 3, 2025:

Agree with this, even though I have never encountered any problems with the wallet files on a Mac with a FAT formatted disk, only on the blocks dir.

willcl-ark commented at 12:40 pm on January 20, 2025:

I am not convinced that walletdir is a problem, do we have any reports of this?

I can add it, but it looks cleanest to first refactor GetWalletDir into ArgsMan, so that we can fetch this directory here in common/init.cpp, which will make this change quite a bit bigger…

tdb3 commented at 10:59 pm on January 20, 2025:

Fair point. Either approach (the refactor, or deferring) sounds reasonable. In the big picture, the warnings for blocksdir and datadir add value and I don’t think walletdir warning should hold up the PR.

I do wonder how common it actually is that someone (using macOS) is using exFAT for just the walletdir, and would therefore miss the warning. I would imagine most users would be using the default walletdir location, and could therefore see the warning as it pertains to datadir.

tdb3 commented at 2:29 am on December 15, 2024: contributor

Concept ACK Great find!

in src/common/init.cpp:82 in ee05e73046 outdated

77+        }};
78+        for (const auto& check : paths_to_check) {
79+            FSType fs_type = GetFilesystemType(check.path);
80+            switch(fs_type) {
81+                case FSType::EXFAT:
82+                    InitWarning(strprintf(_("Specified %s \"%s\" is exFAT which is known to have intermittent corruption problems on MacOS. "

luke-jr commented at 0:55 am on December 25, 2024:

For GUI, maybe we should check this while they still have the opportunity to change it…

philmb3487 commented at 8:05 am on January 1, 2025:

just one nit, the OS is named ‘macOS’

Zero-1729 commented at 9:23 pm on January 14, 2025:

I’d be good to add a recommendation here, too, so the user is aware of possible immediate solutions.

0                    InitWarning(strprintf(_("Specified %s \"%s\" is exFAT which is known to have intermittent corruption problems on MacOS, use APFS instead. "

willcl-ark commented at 12:47 pm on January 20, 2025:

For GUI, maybe we should check this while they still have the opportunity to change it…

To add this check to the qt/intro.cpp flow would require duplicating some of the init checks, as well as:

first creating the chosen directory
check what FS Type the created dir is
if EXFAT prompt the user to change filesystems
either remove the just-created directories (or keep them) depending on response.
etc.

If we want that kind of logic then I’m happy to add it, but not sure if we need much more than a loud InitWarning myself.

just one nit, the OS is named ‘macOS’

Renamed in all places

luke-jr commented at 0:55 am on December 25, 2024: member

Would be ideal if we had a way to reproduce the bug, and can test if the user is actually affected, in case Apple fixes it. But this is better than nothing for now

edilmedeiros commented at 5:26 pm on January 3, 2025: contributor

Concept ACK.

Already lost many hours trying to debug this and my conclusion so far is that the FAT driver on macOS is flawed. Also, it’s quite difficult to reproduce this reliably, IBD will fail in a different point every time. Maybe we can detect a broken block in persistent memory and restart IBD from there?

davidgumberg commented at 11:36 pm on January 8, 2025: contributor

Concept ACK

I second the suggestion by @tdb3 above to also check -walletdir, although I have looked and have not been able to find similar reports of corruption with SQLite and exFAT drives on macOS.

To add a little data pointing in the direction of this bug only existing on macOS, I have synced nodes with datadirs on exFAT drives over both SATA and USB on Fedora 40 and have a long-running bitcoind node that has been running off of an exFAT SATA SSD on Fedora 40 for the past ~6 months and have never experienced any of the symptoms described in #28552 or #31454.

in src/util/fs_helpers.cpp:45 in ee05e73046 outdated

40@@ -41,6 +41,10 @@
41 #include <shlobj.h> /* For SHGetSpecialFolderPathW */
42 #endif // WIN32
43 
44+#ifdef __APPLE__
45+#include <sys/mount.h>

luke-jr commented at 9:12 pm on January 9, 2025:

The manpage says we need #include <sys/param.h> too

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/statfs.2.html

willcl-ark commented at 12:42 pm on January 20, 2025:

added in df1ba101419729aafa382d970ee605e2a6273e26

in doc/files.md:132 in ee05e73046 outdated

124@@ -123,6 +125,12 @@ Path           | Description | Repository notes
125 `addr.dat`     | Peer IP address BDB database; replaced by `peers.dat` in [0.7.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.7.0.md) | [PR #1198](https://github.com/bitcoin/bitcoin/pull/1198), [`928d3a01`](https://github.com/bitcoin/bitcoin/commit/928d3a011cc66c7f907c4d053f674ea77dc611cc)
126 `onion_private_key` | Cached Tor onion service private key for `-listenonion` option. Was used for Tor v2 services; replaced by `onion_v3_private_key` in [0.21.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.21.0.md) | [PR #19954](https://github.com/bitcoin/bitcoin/pull/19954)
127 
128+## Filesystem recommendations
129+
130+When choosing a filesystem for the data directory (`datadir`) or blocks directory (`blocksdir`), some filesystems should be avoided:
131+
132+- **MacOS**: The exFAT filesystem should not be used. There have been multiple reports of database corruption when using exFAT on MacOS for Bitcoin Core. This appears to be due to filesystem-level issues with exFAT on MacOS. See [Issue #31454](https://github.com/bitcoin/bitcoin/issues/31454) for more details.

Zero-1729 commented at 9:22 pm on January 14, 2025:

It’d be good to clarify here that it is recommended to use APFS instead.

0- **MacOS**: The exFAT filesystem should not be used, it is recommended to use the APFS filesystem instead. There have been multiple reports of database corruption when using exFAT on MacOS for Bitcoin Core. This appears to be due to filesystem-level issues with exFAT on MacOS. See [Issue [#31454](/bitcoin-bitcoin/31454/)](https://github.com/bitcoin/bitcoin/issues/31454) for more details.

Zero-1729 commented at 9:33 pm on January 14, 2025: contributor

Concept ACK

I encountered this error late last year as well. Thank you for working on this. I think having the warning reported is a good measure.

I’ve dropped a few comments above.

I tested the patch on an M1 MBP running MacOS v15.2 with an exFAT drive (83839a35f07055dd03924b5bdab46bf31df33b35):

0$ ./build/src/qt/bitcoin-qt --datadir=/Volumes/Little\ Test/
1
2Warning: Specified data directory "/Volumes/Little Test" is exFAT which is known to have intermittent corruption problems on MacOS. See https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#filesystem-recommendations for more information.
3Warning: Specified blocks directory "/Volumes/Little Test/blocks" is exFAT which is known to have intermittent corruption problems on MacOS. See https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#filesystem-recommendations for more information.
42025-01-14 19:45:56.627 bitcoin-qt[55978:5075996] +[IMKClient subclass]: chose IMKClient_Modern
52025-01-14 19:45:56.627 bitcoin-qt[55978:5075996] +[IMKInputSession subclass]: chose IMKInputSession_Modern
6...

As a minor nit, I do think given this adds a whole bunch of references to macOS, it should be formatted as macOS instead of MacOS for consistency, as macOS is used more often across the repo.

Also, I think it’ll be helpful to have the disk format warning displayed as a dialog for those running the GUI, same way we get a warning if the disk is too small to accommodate blocks:

util: detect and warn when using exFAT on macOS

exFAT is known to cause corruption on macOS. See #28552.

Therefore we should warn when using this fs format for either the blocks
or data directories.

df1ba10141

willcl-ark force-pushed on Jan 20, 2025

willcl-ark commented at 1:55 pm on January 20, 2025: member

df1ba101419729aafa382d970ee605e2a6273e26 squashes the commits together, and also removes the duplicate warning messages, instead printing a single warning with a list of violating directories, e.g. going from:

0Warning: Specified data directory "/Volumes/sandisk1/signet" is exFAT which is known to have intermittent corruption problems on MacOS. See https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#filesystem-recommendations for more information.
1Warning: Specified blocks directory "/Volumes/sandisk1/signet/blocks" is exFAT which is known to have intermittent corruption problems on MacOS. See https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#filesystem-recommendations for more information.

to

0Warning: The following paths are on exFAT which is known to have intermittent corruption problems on MacOS: data directory ("/Volumes/sandisk1/signet"), blocks directory ("/Volumes/sandisk1/signet/blocks"). See https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#filesystem-recommendations for more information.

… if both datadir and blocksdir are on exfat on macOS.

tdb3 approved

tdb3 commented at 11:37 pm on January 20, 2025: contributor

code review and untested ACK df1ba101419729aafa382d970ee605e2a6273e26

I also think a GUI warning will be beneficial (and would guess is pertinent to a significant number of the users running macOS and exFAT)

While I don’t have the hardware to test locally, fwiw cross compilation was successful with Ubuntu 24.04.

0Cross compiling ....................... TRUE, for Darwin, aarch64
1C++ compiler .......................... Clang 18.1.3, /usr/bin/clang++

DrahtBot requested review from edilmedeiros on Jan 20, 2025

DrahtBot requested review from davidgumberg on Jan 20, 2025

DrahtBot requested review from hebasto on Jan 20, 2025

DrahtBot requested review from Zero-1729 on Jan 20, 2025

DrahtBot requested review from marcofleon on Jan 20, 2025

util: detect and warn when using exFAT on MacOS #31453

Code Coverage & Benchmarks

Reviews

Conflicts