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

furszy · 2025-12-29T19:44:56Z

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: furszy@85fa4e2

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 · 2025-12-29T19:45:02Z

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  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.

rkrux

Combining the individual suggestions, the full diff:

diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py
index 3bd8e6a310..f0b34d685d 100755
--- a/test/functional/wallet_createwallet.py
+++ b/test/functional/wallet_createwallet.py
@@ -33,21 +33,23 @@ class CreateWalletTest(BitcoinTestFramework):
         wallet_name = "bad_permissions"
         dir_path = node.wallets_path / wallet_name
         dir_path.mkdir(parents=True)
-        os.chmod(dir_path, stat.S_IREAD | stat.S_IEXEC)
+        original_dir_perms = dir_path.stat().st_mode
+        os.chmod(dir_path, original_dir_perms & ~(stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH))
         try:
-            (dir_path / f".tmp_{random.randrange(1 << 32)}").touch() # Verify the directory is actually non-writable
-            self.log.warn("Skipping non-writable directory test: unable to enforce read-only permissions")
-            return
+            if (dir_path / f".tmp_{random.randrange(1 << 32)}").touch(): # Verify the directory is actually non-writable
+                self.log.warning("Skipping non-writable directory test: unable to enforce read-only permissions")
+                return
+            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)
         except PermissionError:
             pass
-        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)
-        dir_path.chmod(dir_path.stat().st_mode | stat.S_IWRITE)
+        finally:
+            dir_path.chmod(original_dir_perms)

test/functional/wallet_createwallet.py

furszy · 2025-12-30T16:17:10Z

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.

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"

stickies-v · 2026-01-03T08:03:58Z

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 · 2026-01-03T17:17:06Z

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 · 2026-01-03T17:41:57Z

@stickies-v done. Run this simple test on master: furszy@85fa4e2
The node crashes after loading a wallet in a non-writable directory; generating a block is enough to trigger it.

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

stickies-v · 2026-01-05T07:41:34Z

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 · 2026-01-07T23:59:14Z

ACK 2ef6add

Tests passed

DrahtBot added the Wallet label Dec 29, 2025

furszy mentioned this pull request Dec 29, 2025

qa: Framework fails when initiating node from cached chain #34163

Open

DrahtBot added the CI failed label Dec 29, 2025

furszy force-pushed the 2025_wallet_check_db_permissions branch from 68a4677 to 16b82f1 Compare December 30, 2025 05:30

rkrux reviewed Dec 30, 2025

View reviewed changes

furszy force-pushed the 2025_wallet_check_db_permissions branch from c919a62 to 4ad08fc Compare December 30, 2025 16:09

DrahtBot removed the CI failed label Dec 30, 2025

furszy force-pushed the 2025_wallet_check_db_permissions branch from 4ad08fc to 0a9ddd3 Compare December 30, 2025 20:00

furszy mentioned this pull request Jan 2, 2026

wallet: make migration more robust against failures #34193

Open

furszy force-pushed the 2025_wallet_check_db_permissions branch from 651eb90 to 66ebb96 Compare January 3, 2026 18:17

DrahtBot added the CI failed label Jan 3, 2026

furszy added 2 commits January 3, 2026 14:31

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

504e214

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

2ef6add

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

furszy force-pushed the 2025_wallet_check_db_permissions branch from 66ebb96 to 2ef6add Compare January 3, 2026 19:32

DrahtBot removed the CI failed label Jan 3, 2026

DrahtBot mentioned this pull request Jan 4, 2026

rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures #33014

Open

DrahtBot requested a review from stickies-v January 7, 2026 23:59

DrahtBot mentioned this pull request Jan 14, 2026

wallet: disallow creating new or restoring to an unnamed (default) wallet #34269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

furszy commented Dec 29, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Dec 29, 2025 •

edited

Loading

Uh oh!

rkrux left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Dec 30, 2025

Uh oh!

stickies-v commented Jan 3, 2026 •

edited

Loading

Uh oh!

furszy commented Jan 3, 2026 •

edited

Loading

Uh oh!

furszy commented Jan 3, 2026

Uh oh!

stickies-v commented Jan 5, 2026

Uh oh!

bensig commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

Are you sure you want to change the base?

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

Conversation

furszy commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1) For Wallet Creation

2) For Wallet Loading

Uh oh!

DrahtBot commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Dec 30, 2025

Uh oh!

stickies-v commented Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jan 3, 2026

Uh oh!

stickies-v commented Jan 5, 2026

Uh oh!

bensig commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

furszy commented Dec 29, 2025 •

edited

Loading

DrahtBot commented Dec 29, 2025 •

edited

Loading

stickies-v commented Jan 3, 2026 •

edited

Loading

furszy commented Jan 3, 2026 •

edited

Loading