wallet: improve error msg when db directory is not writable

furszy commented at 7:44 pm on December 29, 2025: member

Make wallet creation and load fail with a clear error when the db directory isn’t writable.

1) For Wallet Creation

Before: creating a wallet would return a generic error: “SQLiteDatabase: Failed to open database: unable to open database file”

After: creating a wallet returns: “SQLiteDatabase: Failed to open database in directory <dir_path>: directory is not writable”

2) For Wallet Loading

We currently allow the load of wallets located on non-writable directories. This is problematic because the node crashes on any subsequent write; generating a block is enough to trigger it. Can be verified just by running the following test on master: https://github.com/furszy/bitcoin-core/commit/85fa4e2910f0a8f43d4ee5a1322cbb69656d85e9

Also, to check directory writability, this creates a tmp file rather than relying on the permissions() functions, since perms bits alone may not reliably reflect actual writability in some systems.

Testing Note: Pushed the tests in separate commits so they can be cherry-picked on master for comparison.

DrahtBot added the label Wallet on Dec 29, 2025

DrahtBot commented at 7:45 pm on December 29, 2025: 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/34176.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	bensig
Concept ACK	stickies-v

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34269 (wallet: disallow creating new or restoring to an unnamed (default) wallet by achow101)
#33014 (rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures by b-l-u-e)

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 CI failed on Dec 29, 2025

furszy force-pushed on Dec 30, 2025

in test/functional/wallet_createwallet.py:39 in 16b82f13db

34+        dir_path = node.wallets_path / wallet_name
35+        dir_path.mkdir(parents=True)
36+        os.chmod(dir_path, stat.S_IREAD | stat.S_IEXEC)
37+        try:
38+            (dir_path / f".tmp_{random.randrange(1 << 32)}").touch() # Verify the directory is actually non-writable
39+            self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")

rkrux commented at 9:00 am on December 30, 2025:

From the CI failure logs:

0stderr:
1/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_createwallet.py:39: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
2  self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")

0- self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")
1+ self.log.warning("Skipping non-writable directory test: unable to enforce read-only permissions")

rkrux commented at 9:16 am on December 30, 2025:

Based on the logs, it appears that all the 9 failing jobs skipped the test. At the moment, I don’t know why the touch function didn’t throw here even after non-writable permissions given to the directory. The jobs failed because the clean up was not done after the test because of access denied errors. Restoring the original permissions of the directory in a finally block should help here.

 0stderr:
 1D:\a\bitcoin\bitcoin\build\test\functional\wallet_createwallet.py:39: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
 2
 3  self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")
 4
 5Traceback (most recent call last):
 6
 7  File "D:\a\bitcoin\bitcoin\build\test\functional\wallet_createwallet.py", line 202, in <module>
 8
 9    CreateWalletTest(__file__).main()
10
11    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
12
13  File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_framework.py", line 154, in main
14
15    exit_code = self.shutdown()
16
17  File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_framework.py", line 332, in shutdown
18
19    shutil.rmtree(self.options.tmpdir)
20
21    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
22
23  File "C:\hostedtoolcache\windows\Python\3.14.2\x64\Lib\shutil.py", line 852, in rmtree
24
25    _rmtree_impl(path, dir_fd, onexc)
26
27    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
28
29  File "C:\hostedtoolcache\windows\Python\3.14.2\x64\Lib\shutil.py", line 697, in _rmtree_unsafe
30
31    onexc(os.rmdir, fullname, err)
32
33    ~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
34
35  File "C:\hostedtoolcache\windows\Python\3.14.2\x64\Lib\shutil.py", line 693, in _rmtree_unsafe
36
37    os.rmdir(fullname)
38
39    ~~~~~~~~^^^^^^^^^^
40
41PermissionError: [WinError 5] Access is denied: 'D:\\a\\_temp\\test_runner_₿_🏃_20251230_063644\\wallet_createwallet_186\\node0\\regtest\\wallets\\bad_permissions'

furszy commented at 4:08 pm on December 30, 2025:

Done. Thanks.

furszy commented at 4:09 pm on December 30, 2025:

Done as suggested. Thanks.

in test/functional/wallet_createwallet.py:44 in 16b82f13db

39+            self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")
40+            return
41+        except PermissionError:
42+            pass
43+        assert_raises_rpc_error(-4, f"SQLiteDatabase: Failed to open database in directory '{str(dir_path)}': directory is not writable", node.createwallet, wallet_name=wallet_name, descriptors=True)
44+        dir_path.chmod(dir_path.stat().st_mode | stat.S_IWRITE)

rkrux commented at 9:04 am on December 30, 2025:

By debugging the permissions at 3 different stages here, it appears that the original permissions of the directory are not set at the end of this test.

 0diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py
 1index 3bd8e6a310..29a2db3dab 100755
 2--- a/test/functional/wallet_createwallet.py
 3+++ b/test/functional/wallet_createwallet.py
 4@@ -33,7 +33,9 @@ class CreateWalletTest(BitcoinTestFramework):
 5         wallet_name = "bad_permissions"
 6         dir_path = node.wallets_path / wallet_name
 7         dir_path.mkdir(parents=True)
 8+        print(dir_path.stat().st_mode)
 9         os.chmod(dir_path, stat.S_IREAD | stat.S_IEXEC)
10+        print(dir_path.stat().st_mode)
11         try:
12             (dir_path / f".tmp_{random.randrange(1 << 32)}").touch() # Verify the directory is actually non-writable
13             self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")
14@@ -42,12 +44,13 @@ class CreateWalletTest(BitcoinTestFramework):
15             pass
16         assert_raises_rpc_error(-4, f"SQLiteDatabase: Failed to open database in directory '{str(dir_path)}': directory is not writable", node.createwallet, wallet_name=wallet_name, descriptors=True)
17         dir_path.chmod(dir_path.stat().st_mode | stat.S_IWRITE)
18+        print(dir_path.stat().st_mode)

0➜  bitcoin git:(2025_wallet_check_db_permissions) ✗ ./build/test/functional/wallet_createwallet.py            
12025-12-30T09:01:50.770611Z TestFramework (INFO): PRNG seed is: 3561247270765759257
22025-12-30T09:01:50.771467Z TestFramework (INFO): Initializing test directory /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_5m4h9kjq
32025-12-30T09:01:51.370097Z TestFramework (INFO): Test wallet creation failure due to non-writable directory
416877
516704
616832
72025-12-30T09:01:51.424150Z TestFramework (INFO): Stopping nodes
82025-12-30T09:01:51.689178Z TestFramework (INFO): Cleaning up /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/bitcoin_func_test_5m4h9kjq on exit
92025-12-30T09:01:51.689295Z TestFramework (INFO): Tests successful

I believe 16877 (0o40755) should be set back at the end of this test.

furszy commented at 4:08 pm on December 30, 2025:

Done as suggested. Thanks.

in test/functional/wallet_createwallet.py:36 in 16b82f13db

27@@ -25,9 +28,26 @@ def set_test_params(self):
28     def skip_test_if_missing_module(self):
29         self.skip_if_no_wallet()
30 
31+    def test_bad_dir_permissions(self, node):
32+        self.log.info("Test wallet creation failure due to non-writable directory")
33+        wallet_name = "bad_permissions"
34+        dir_path = node.wallets_path / wallet_name
35+        dir_path.mkdir(parents=True)
36+        os.chmod(dir_path, stat.S_IREAD | stat.S_IEXEC)

rkrux commented at 9:18 am on December 30, 2025:

https://docs.python.org/3/library/stat.html#stat.S_IREAD

stat.S_IREAD Unix V7 synonym for S_IRUSR.

Maybe s/stat.S_IREAD/stat.S_IRUSR to avoid using the Unix synonym, thereby making it platform independent. Just a guess because the test works fine on my MacOS.

furszy commented at 4:09 pm on December 30, 2025:

Done as suggested. Thanks.

rkrux commented at 9:31 am on December 30, 2025: contributor

Combining the individual suggestions, the full diff:

 0diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py
 1index 3bd8e6a310..f0b34d685d 100755
 2--- a/test/functional/wallet_createwallet.py
 3+++ b/test/functional/wallet_createwallet.py
 4@@ -33,21 +33,23 @@ class CreateWalletTest(BitcoinTestFramework):
 5         wallet_name = "bad_permissions"
 6         dir_path = node.wallets_path / wallet_name
 7         dir_path.mkdir(parents=True)
 8-        os.chmod(dir_path, stat.S_IREAD | stat.S_IEXEC)
 9+        original_dir_perms = dir_path.stat().st_mode
10+        os.chmod(dir_path, original_dir_perms & ~(stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH))
11         try:
12-            (dir_path / f".tmp_{random.randrange(1 << 32)}").touch() # Verify the directory is actually non-writable
13-            self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")
14-            return
15+            if (dir_path / f".tmp_{random.randrange(1 << 32)}").touch(): # Verify the directory is actually non-writable
16+                self.log.warning("Skipping non-writable directory test: unable to enforce read-only permissions")
17+                return
18+            assert_raises_rpc_error(-4, f"SQLiteDatabase: Failed to open database in directory '{str(dir_path)}': directory is not writable", node.createwallet, wallet_name=wallet_name, descriptors=True)
19         except PermissionError:
20             pass
21-        assert_raises_rpc_error(-4, f"SQLiteDatabase: Failed to open database in directory '{str(dir_path)}': directory is not writable", node.createwallet, wallet_name=wallet_name, descriptors=True)
22-        dir_path.chmod(dir_path.stat().st_mode | stat.S_IWRITE)
23+        finally:
24+            dir_path.chmod(original_dir_perms)
25

furszy force-pushed on Dec 30, 2025

furszy commented at 4:17 pm on December 30, 2025: member

Thanks for the review @rkrux! Applied all suggestions except the touch() if-block, since the function doesn’t return a boolean (per docs, it either returns None or an exception). Also pulled out the is_dir_writable function to make the introduced try-except-finally workflow slightly more readable.

DrahtBot removed the label CI failed on Dec 30, 2025

wallet: improve error msg when db directory is not writable

Add `IsDirWritable` helper in util/fs_helpers to test whether a directory
can be written to by creating a temporary file.

Use this check when opening a SQLite database to fail early with a clear
error if the database directory is not writable, preventing obscure SQLite
errors.

Before: creating a wallet would throw a generic error:
"SQLiteDatabase: Failed to open database: unable to open database file"

After: creating a wallet throws:
"SQLiteDatabase: Failed to open database in directory <dir_path>: directory is not writable"

350840e960

furszy force-pushed on Dec 30, 2025

stickies-v commented at 8:03 am on January 3, 2026: contributor

Hmm. Nothing against better reporting when something failed, but the changes necessary to do that here seem a bit excessive for the pay-off? It doesn’t seem like this change would have made resolving #34163 trivial?

Didn’t test, but would sqlite3_errmsg(m_db) offer a more simple alternative? I think I’m slightly Approach NACK atm, unless I’m wrong on the usefulness of the change.

furszy commented at 5:17 pm on January 3, 2026: member

the changes necessary to do that here seem a bit excessive for the pay-off? It doesn’t seem like this change would have made resolving #34163 trivial?

It turned out not to help with #34163, but it is a general improvement to reporting read-only permission issues. Which we currently don’t do properly. Can take #34163 as the starting point that led to uncover other issues.

Something I didn’t mention in the PR description is that this also fixes opening a non-writable wallet directory, which is actually more important than issues during creation. We currently open the wallet even when the directory is read-only, and then crash on any subsequent write because we cannot write to the -journal file. I should add a simple test case demonstrating this bad behavior and append it to the PR description..

Also, do you really think that labeling these changes as “excessive” is correct? We are talking about an 8-line function that improves the user experience at almost no cost and could be useful in other contexts as well. Even setting aside the post-opening crash and only focusing on the creation issue, I wouldn’t label this as an excessive change.

Didn’t test, but would sqlite3_errmsg(m_db) offer a more simple alternative? I think I’m slightly Approach NACK atm, unless I’m wrong on the usefulness of the change.

sqlite3_errmsg(m_db) returns the same message as sqlite3_errstr(ret). Both return the generic and not particularly useful “unable to open database file”.

furszy commented at 5:41 pm on January 3, 2026: member

@stickies-v done. Run this simple test on master: https://github.com/furszy/bitcoin-core/commit/85fa4e2910f0a8f43d4ee5a1322cbb69656d85e9 The node crashes after loading a wallet in a non-writable directory; generating a block is enough to trigger it.

furszy force-pushed on Jan 3, 2026

DrahtBot added the label CI failed on Jan 3, 2026

test: add coverage for wallet creation in non-writable directory 504e21416e

test: add coverage for loading a wallet in a non-writable directory

Previously, wallets in non-writable directories were loaded,
leading to crashes on any subsequent write.

2ef6adde86

furszy force-pushed on Jan 3, 2026

DrahtBot removed the label CI failed on Jan 3, 2026

stickies-v commented at 7:41 am on January 5, 2026: contributor

Also, do you really think that labeling these changes as “excessive” is correct?

It’s not a big diff, but adding helper functions to improve diagnostic messages for a rare issue that is reasonably straightforward for the user to diagnose felt like perhaps not something we should have in our codebase, hence why I used “excessive” (in a relative way).

The node crashes after loading a wallet in a non-writable directory; generating a block is enough to trigger it.

This seems like a much better rationale to me for this PR. Concept ACK. Approach-wise, I’m confused why the sqlite3_db_readonly call we already have doesn’t do what we’d expect it to do, but I’m not sure I’ll be diving into this further, just wanted to leave early concept feedback.

bensig commented at 11:59 pm on January 7, 2026: contributor

ACK 2ef6adde86f9a248463b8b628f559012be103382

Tests passed

DrahtBot requested review from stickies-v on Jan 7, 2026

DrahtBot added the label Needs rebase on Jan 21, 2026

DrahtBot commented at 11:29 pm on January 21, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/util/fs_helpers.cpp:314 in 2ef6adde86

309+{
310+    // Attempt to create a tmp file in the directory
311+    if (!fs::is_directory(dir_path)) throw std::runtime_error(strprintf("Path %s is not a directory", fs::PathToString(dir_path)));
312+    FastRandomContext rng;
313+    const auto tmp = dir_path / fs::PathFromString(strprintf(".tmp_%d", rng.rand64()));
314+    if (auto created{fsbridge::fopen(tmp, "a")}) {

luke-jr commented at 11:31 pm on January 21, 2026:

0    if (auto created{fsbridge::fopen(tmp, "wx")}) {

On the unlikely chance someone already has a file with that name or races us for it…

(Python’s pathlib touch method already has this check)

maflcko commented at 7:39 am on January 22, 2026:

would need to be

0#ifdef __MINGW64__
1            "w" // Temporary workaround for [#30210](/bitcoin-bitcoin/30210/)
2#else
3            "wx"
4#endif

luke-jr changes_requested

wallet: improve error msg when db directory is not writable #34176

1) For Wallet Creation

2) For Wallet Loading

Code Coverage & Benchmarks

Reviews

Conflicts