wallet: crash fix, handle non-writable db directories

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 loading 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
Concept ACK	stickies-v, rkrux
Stale ACK	bensig, achow101

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:

#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

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

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

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

furszy commented at 3:58 pm on January 22, 2026:

Done as suggested. Thanks.

luke-jr changes_requested

rkrux commented at 2:24 pm on January 22, 2026: contributor

Concept ACK 2ef6adde86f9a248463b8b628f559012be103382, will finish review.

luke-jr referenced this in commit ef5fdf190f on Jan 22, 2026

luke-jr referenced this in commit 240a9e801f on Jan 22, 2026

luke-jr referenced this in commit 12fd702a76 on Jan 22, 2026

furszy renamed this:
~~wallet: improve error msg when db directory is not writable~~
wallet: early error when db directory is not writable
on Jan 22, 2026

furszy force-pushed on Jan 22, 2026

furszy renamed this:
~~wallet: early error when db directory is not writable~~
wallet: crash fix, handle non-writable db directories
on Jan 22, 2026

DrahtBot removed the label Needs rebase on Jan 22, 2026

furszy commented at 6:28 pm on January 22, 2026: member

Updated per feedback and rebased.

in src/util/fs_helpers.cpp:324 in 0f2381ae60 outdated

320+#endif
321+
322+    if (auto created{fsbridge::fopen(tmp, mode)}) {
323+        std::fclose(created);
324+        std::error_code ec;
325+        fs::remove(tmp, ec); // clean up, ignore errors

rkrux commented at 2:31 pm on January 23, 2026:

In 0f2381ae6058ff12197c9ac0bcacbe380a55b206 “wallet: early error when db directory is not writable”

Nit to highlight it even further that ec is unused.

 0@@ -321,8 +321,8 @@ bool IsDirWritable(const fs::path& dir_path)
 1 
 2     if (auto created{fsbridge::fopen(tmp, mode)}) {
 3         std::fclose(created);
 4-        std::error_code ec;
 5-        fs::remove(tmp, ec); // clean up, ignore errors
 6+        std::error_code _;
 7+        fs::remove(tmp, _); // clean up, ignore errors
 8         return true;
 9     }
10     return false;

furszy commented at 2:59 pm on January 23, 2026:

I think we should be good already with the comment.

in test/functional/wallet_createwallet.py:42 in e990a1c5b2

37+        os.chmod(dir_path, original_dir_perms & ~(stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH))
38+        if is_dir_writable(dir_path):
39+            self.log.warning("Skipping non-writable directory test: unable to enforce read-only permissions")
40+        else:
41+            # Run actual test
42+            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)

rkrux commented at 2:34 pm on January 23, 2026:

In e990a1c5b2642431cd4a83d31102be5a8abd65eb “test: add coverage for wallet creation in non-writable directory”

We don’t need the descriptors argument anymore.

0--- a/test/functional/wallet_createwallet.py
1+++ b/test/functional/wallet_createwallet.py
2@@ -39,7 +39,7 @@ class CreateWalletTest(BitcoinTestFramework):
3             self.log.warning("Skipping non-writable directory test: unable to enforce read-only permissions")
4         else:
5             # Run actual test
6-            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)
7+            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)
8         # Reset directory permissions for cleanup
9         dir_path.chmod(original_dir_perms)

furszy commented at 3:02 pm on January 23, 2026:

Done as suggested.

in test/functional/wallet_startup.py:50 in c22de0d82e

42@@ -38,6 +43,27 @@ def create_unnamed_wallet(self, **kwargs):
43         shutil.move(self.nodes[0].wallets_path / wallet_name / "wallet.dat", self.nodes[0].wallets_path / "wallet.dat")
44         shutil.rmtree(self.nodes[0].wallets_path / wallet_name)
45 
46+    def test_load_unwritable_wallet(self, node):
47+        self.log.info("Test wallet load failure due to non-writable directory")
48+        wallet_name = "bad_permissions"
49+
50+        node.createwallet(wallet_name, descriptors=True)

rkrux commented at 2:35 pm on January 23, 2026:

In c22de0d82ed87846a40629e40746b696621da3c7 “test: add coverage for loading a wallet in a non-writable directory”

Same removal of this argument.

 0--- a/test/functional/wallet_startup.py
 1+++ b/test/functional/wallet_startup.py
 2@@ -47,7 +47,7 @@ class WalletStartupTest(BitcoinTestFramework):
 3         self.log.info("Test wallet load failure due to non-writable directory")
 4         wallet_name = "bad_permissions"
 5 
 6-        node.createwallet(wallet_name, descriptors=True)
 7+        node.createwallet(wallet_name)
 8         node.unloadwallet(wallet_name)
 9 
10         dir_path = node.wallets_path / wallet_name

furszy commented at 3:02 pm on January 23, 2026:

Done as suggested.

furszy force-pushed on Jan 23, 2026

furszy commented at 3:02 pm on January 23, 2026: member

Updated per feedback. Thanks rkrux.

achow101 commented at 8:47 pm on January 26, 2026: member

ACK 3e43bce7688ed0f660e74734802068c9fc266412

DrahtBot requested review from rkrux on Jan 26, 2026

luke-jr referenced this in commit 607799b6db on Jan 29, 2026

luke-jr referenced this in commit 44e9edefc2 on Jan 29, 2026

luke-jr referenced this in commit fde25e03f3 on Jan 29, 2026

DrahtBot added the label Needs rebase on Feb 7, 2026

wallet: handle non-writable db directories

1) For wallet load, this fixes a crash.

We currently allow loading wallets located on non-writable directories.
This is problematic because the node crashes on any subsequent write.
E.g. generating a block is enough to trigger it.

2) For wallet creation, this improves the returned error msg.

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"

28f9d6f35b

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

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.

271846280d

furszy force-pushed on Feb 8, 2026

DrahtBot removed the label Needs rebase on Feb 8, 2026

wallet: crash fix, handle non-writable db directories #34176

1) For Wallet Creation

2) For Wallet Loading

Code Coverage & Benchmarks

Reviews

Conflicts