wallet: Allow read-only database access for info and dump commands #32685

PeterWrighten · 2025-06-05T20:36:09Z

Problem

The bitcoin-wallet info and bitcoin-wallet dump commands currently fail when wallet files are write-protected, throwing "Database opened in readonly mode but read-write permissions are needed" errors. This prevents users from safely inspecting write-protected wallet files, which is important for:

Security-conscious setups where wallets are stored on read-only filesystems
Forensic analysis of wallet files without risk of modification
Backup verification without accidentally corrupting wallet data
Compliance with principle of least privilege (read-only operations should only require read access)

Solution

This PR adds comprehensive read-only database support to enable bitcoin-wallet info and dump commands to work with write-protected wallet files.

Key Changes

Database Layer (src/wallet/db.h, src/wallet/sqlite.h, src/wallet/sqlite.cpp)

Add read_only boolean option to DatabaseOptions structure
Add IsReadOnly() virtual method to WalletDatabase interface
Modify SQLiteDatabase to properly handle read-only mode:
- Use SQLITE_OPEN_READONLY flag when read_only=true
- Skip write operations (pragma settings, table creation) in read-only mode
- Maintain full read capability for queries and data retrieval

Wallet Tool (src/wallet/wallettool.cpp)

Update bitcoin-wallet info command to use options.read_only = true
Update bitcoin-wallet dump command to use options.read_only = true

Wallet Loading (src/wallet/walletdb.cpp)

Enhance LoadWallet() to detect read-only mode via database.IsReadOnly()
Skip version updates and other write operations when in read-only mode
Prevent "Wallet corrupted" errors during read-only access

Legacy Support (src/wallet/migrate.h)

Add IsReadOnly() method to BerkeleyRODatabase for consistency

Testing

The implementation has been thoroughly tested:

Manual Testing

✅ Created test wallets and made them read-only (chmod 444)
✅ Verified bitcoin-wallet info works with read-only files
✅ Verified bitcoin-wallet dump works with read-only files
✅ Confirmed identical output between read-write and read-only access
✅ Verified normal read-write operations remain unaffected

Unit Tests

All existing wallet tests continue to pass, confirming no regression in normal wallet operations.

Safety Verification

File permissions are respected (no writes attempted on read-only files)
Data integrity maintained (read-only access produces identical results)
Backwards compatibility preserved (existing workflows unaffected)

Security Considerations

This change improves security by:

Enabling inspection of wallet files without write access (principle of least privilege)
Preventing accidental wallet corruption during read-only operations
Supporting secure forensic analysis workflows
Reducing attack surface for read-only operations

The implementation is conservative and safe:

Only affects explicitly read-only operations (info and dump commands)
No changes to consensus or validation logic
No impact on normal wallet operations
Uses SQLite's built-in read-only mode for safety

Backwards Compatibility

Full backwards compatibility is maintained:

Existing wallet operations work identically
No API changes for normal use cases
No changes to wallet file formats
No impact on other Bitcoin Core components

Related Work

This builds upon existing read-only database patterns in Bitcoin Core, particularly the BerkeleyRODatabase class used for wallet migration scenarios.

#28116

DrahtBot · 2025-06-05T20:36:12Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	shahsb

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:

#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#33032 (wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#32636 (Split CWallet::Create() into CreateNew and LoadExisting by davidgumberg)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#27865 (wallet: Track no-longer-spendable TXOs separately 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 · 2025-06-05T21:01:56Z

🚧 At least one of the CI tasks failed.
_{Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/43573963077}
_{LLM reason (✨ experimental): The build failed due to a compilation error caused by attempting to instantiate an abstract class wallet::MockableDatabase.}

Hints

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.

PeterWrighten · 2025-06-06T15:14:54Z

Review request: @achow101 @furszy @ryanofsky @Sjors

shahsb

Could we also guard higher-level wallet operations (e.g., in LoadWallet, TopUpKeyPool, UpgradeWallet, etc.) against unintended writes in read-only mode?.

This helps future-proof against accidental changes, even if developers mistakenly invoke write operations upstream.

shahsb

Concept ACK

PeterWrighten · 2025-06-09T10:17:17Z

Could we also guard higher-level wallet operations (e.g., in LoadWallet, TopUpKeyPool, UpgradeWallet, etc.) against unintended writes in read-only mode?.

This helps future-proof against accidental changes, even if developers mistakenly invoke write operations upstream.

Good idea. I would check it and work on it. But I just think they are well guarded because this PR is working for Database level read-only mode...

achow101

We require that all commits compile and pass the tests by themselves, but it looks like the commits in this PR do not. Please revise your commits so that they pass all of the tests individually.

src/wallet/sqlite.cpp

src/wallet/walletdb.cpp

src/wallet/sqlite.cpp

test/functional/tool_wallet.py

src/wallet/test/util.h

src/wallet/test/db_tests.cpp

src/wallet/test/walletdb_tests.cpp

test/functional/tool_wallet.py

PeterWrighten · 2025-06-10T03:54:21Z

We require that all commits compile and pass the tests by themselves, but it looks like the commits in this PR do not. Please revise your commits so that they pass all of the tests individually.

Okay, but it seems without permission, I cannot run some CIs and tests on GitHub. I have passed 11 CIs on GitHub, and all tests on my machine. How should I confirm whether they are all passed on Git Action or not?

achow101 · 2025-06-10T04:28:36Z

I cannot run some CIs and tests on GitHub.

Approved, they should be run automatically now.

PeterWrighten · 2025-06-10T18:52:09Z

I cannot run some CIs and tests on GitHub.

Approved, they should be run automatically now.

Seems that I still could not run all ci tests without approval. Can you fix it?

PeterWrighten · 2025-06-12T17:03:31Z

@achow101 I modified the code as your review comments, and I hope you can recheck them. It seems i could still not run all ci tests without your approval, is it because of mechanism? or other reason which we did not realize...

DrahtBot · 2025-06-12T17:39:09Z

🚧 At least one of the CI tasks failed.
_{Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/43991064690}
_{LLM reason (✨ experimental): The CI failure is caused by a compilation error due to an unused private field causing the build to stop.}

Hints

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.

PeterWrighten

Run all tests.

achow101 · 2025-06-12T18:21:37Z

Please squash your commits; reviewer feedback does not need to be addressed in a separate commit. The current commit layout is difficult to review as code is introduced in one commit, only to be removed/rewritten in the next commit.

When I said that all commits need to pass the tests, I did not mean that everything should be squashed into one commit. While that can be okay to do, it's better to have individual commits that introduce specific things, as long as those commits still pass the tests. My suggestion was more that the commits could be reordered, and some of them squashed, in order to allow all of the tests to pass.

I highly advise that you compile and run the tests yourself instead of relying on CI. CI on PRs submitted by new contributors requires manual approval to run. Please stop asking for it to be run, it will be run when a maintainer notices that it was not run. You can compile and run everything locally which will cover most things that CI fails on.

PeterWrighten · 2025-06-20T03:31:48Z

Review Request: @rkrux @achow101 @maflcko

PeterWrighten · 2025-06-23T17:05:44Z

Review Request: @rkrux @achow101 @maflcko

Review Request: @achow101 @Sjors @maflcko

DrahtBot · 2025-06-27T17:12:13Z

🚧 At least one of the CI tasks failed.
_{Task tidy: https://github.com/bitcoin/bitcoin/runs/44942755882}
_{LLM reason (✨ experimental): The CI failed due to a compilation error caused by the undeclared identifier 'upgraded_txs' in walletdb.cpp.}

Hints

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.

PeterWrighten · 2025-07-07T18:02:46Z

Review Request: @achow101 @Sjors @maflcko

I have passed all cis, but bot did not remove "ci failed" label...

maflcko · 2025-07-08T16:13:07Z

You'll have to rebase. Did you see the immediately previous comment? Also, ref #32818

rkrux · 2025-07-14T17:48:25Z

Re: Merge branch 'master' into wallet-readonly-access

I don't think merging master in the PR branch is preferred, instead rebasing is done.
https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

Fixes bitcoin#29559 1. Add read-only database access for bitcoin-wallet info and dump commands 2. Implement read-only mode in database layer with SQLite handling 3. Prevent write operations when wallet files are write-protected 4. Enable safe inspection of wallet files without risk of modification

- Add ReadName() and ReadPurpose() methods to WalletBatch class - Replace MockableDatabase direct tests with WalletBatch-based tests - Deduplicate tool_wallet.py test methods using lambda functions

PeterWrighten · 2025-07-15T14:26:10Z

Re: Merge branch 'master' into wallet-readonly-access

I don't think merging master in the PR branch is preferred, instead rebasing is done. https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

I have rebased but it seems one ci failed because of timeout issue. To confirm this issue did't fail accidentally, I request to rerun it.

PeterWrighten · 2025-07-17T18:25:35Z

You'll have to rebase. Did you see the immediately previous comment? Also, ref #32818

I have rebased and read comment. I consider my implementation is better...

DrahtBot added the Wallet label Jun 5, 2025

PeterWrighten marked this pull request as draft June 5, 2025 20:46

PeterWrighten force-pushed the wallet-readonly-access branch from cd8109d to f6f1f41 Compare June 5, 2025 21:01

DrahtBot added the CI failed label Jun 5, 2025

PeterWrighten mentioned this pull request Jun 5, 2025

bitcoin-wallet requires write permissions when unneeded #29559

Open

1 task

DrahtBot removed the CI failed label Jun 5, 2025

PeterWrighten marked this pull request as ready for review June 5, 2025 23:18

This was referenced Jun 6, 2025

wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet #31423

Merged

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

shahsb reviewed Jun 9, 2025

View reviewed changes

achow101 reviewed Jun 9, 2025

View reviewed changes

PeterWrighten force-pushed the wallet-readonly-access branch from 719afbe to e4e94df Compare June 10, 2025 18:37

PeterWrighten force-pushed the wallet-readonly-access branch from e4e94df to 4dc9a17 Compare June 12, 2025 16:59

PeterWrighten requested a review from achow101 June 12, 2025 17:01

DrahtBot added the CI failed label Jun 12, 2025

PeterWrighten force-pushed the wallet-readonly-access branch 2 times, most recently from 7d5e234 to 8e83621 Compare June 12, 2025 17:54

PeterWrighten commented Jun 12, 2025

View reviewed changes

achow101 removed their request for review June 12, 2025 18:30

DrahtBot removed the CI failed label Jun 18, 2025

DrahtBot added the Needs rebase label Jun 25, 2025

achow101 mentioned this pull request Jun 26, 2025

Add read-only mode to sqlite db and use in bitcoin-wallet #32818

Draft

DrahtBot added the CI failed label Jun 27, 2025

DrahtBot removed the Needs rebase label Jun 27, 2025

PeterWrighten force-pushed the wallet-readonly-access branch from d6f3f85 to 0fc315f Compare June 27, 2025 20:09

DrahtBot added the Needs rebase label Jun 27, 2025

DrahtBot removed the Needs rebase label Jul 14, 2025

This was referenced Jul 14, 2025

wallet: Prepare for future upgrades by recording versions of last client to open and decrypt #32895

Open

wallet: Track no-longer-spendable TXOs separately #27865

Open

PeterWrighten force-pushed the wallet-readonly-access branch from fa0699e to dac8bfb Compare July 15, 2025 07:03

DrahtBot added the Needs rebase label Jul 15, 2025

PeterWrighten force-pushed the wallet-readonly-access branch from dac8bfb to 6efd78c Compare July 15, 2025 10:34

PeterWrighten added 2 commits July 15, 2025 19:52

wallet, test: refactor read-only tests to use production interfaces

ec679bc

- Add ReadName() and ReadPurpose() methods to WalletBatch class - Replace MockableDatabase direct tests with WalletBatch-based tests - Deduplicate tool_wallet.py test methods using lambda functions

PeterWrighten force-pushed the wallet-readonly-access branch from 6efd78c to ec679bc Compare July 15, 2025 10:57

DrahtBot removed the Needs rebase label Jul 15, 2025

DrahtBot removed the CI failed label Jul 15, 2025

This was referenced Jul 22, 2025

wallet: Store transactions in a separate sqlite table #33034

Draft

wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase #33032

Open

DrahtBot mentioned this pull request Aug 2, 2025

refactor: Use util::Result class for wallet loading #25722

Draft

DrahtBot mentioned this pull request Aug 23, 2025

Split CWallet::Create() into CreateNew and LoadExisting #32636

Open

wallet: Allow read-only database access for info and dump commands #32685

Are you sure you want to change the base?

wallet: Allow read-only database access for info and dump commands #32685

Uh oh!

Conversation

PeterWrighten commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Key Changes

Testing

Manual Testing

Unit Tests

Safety Verification

Security Considerations

Backwards Compatibility

Related Work

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jun 5, 2025

Uh oh!

PeterWrighten commented Jun 6, 2025

Uh oh!

shahsb left a comment

Choose a reason for hiding this comment

Uh oh!

shahsb left a comment

Choose a reason for hiding this comment

Uh oh!

PeterWrighten commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PeterWrighten commented Jun 10, 2025

Uh oh!

achow101 commented Jun 10, 2025

Uh oh!

PeterWrighten commented Jun 10, 2025

Uh oh!

PeterWrighten commented Jun 12, 2025

Uh oh!

DrahtBot commented Jun 12, 2025

Uh oh!

PeterWrighten left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterWrighten commented Jun 20, 2025

Uh oh!

PeterWrighten commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 27, 2025

Uh oh!

PeterWrighten commented Jul 7, 2025

Uh oh!

maflcko commented Jul 8, 2025

Uh oh!

rkrux commented Jul 14, 2025

Uh oh!

PeterWrighten commented Jul 15, 2025

PeterWrighten commented Jun 5, 2025 •

edited

Loading

DrahtBot commented Jun 5, 2025 •

edited

Loading

PeterWrighten commented Jun 9, 2025 •

edited

Loading

achow101 commented Jun 12, 2025 •

edited

Loading

PeterWrighten commented Jun 23, 2025 •

edited

Loading