bitcoin-wallet requires write permissions when unneeded

BebeSparkelSparkel commented at 7:04 pm on March 4, 2024: none

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

I have some wallets that are write protected and want to use bitcoin-wallet info and bitcoin-wallet dump but am unable to do so because these commands require write permission.

0$ bitcoin-wallet -datadir=. -wallet=wallet-name -dumpfile=wallet.dump dump
1SQLiteDatabase: Database opened in readonly mode but read-write permissions are needed
2$ bitcoin-wallet -datadir=. -wallet=wallet-name info                     
3SQLiteDatabase: Database opened in readonly mode but read-write permissions are needed

Expected behaviour

I would expect commands that are not intended to change the wallet to only require read permissions and be able to work on wallet directories and files that are read only.

This will also stop mistakes from corrupting the database.

Steps to reproduce

0chmod -R a-w <wallet-directory>
1bitcoin-wallet -datadir=<wallet-directory> -wallet=<wallet-name> -dumpfile=wallet.dump dump
2bitcoin-wallet -datadir=<wallet-directory> -wallet=<wallet-name> info

Relevant log output

No response

How did you obtain Bitcoin Core

Package manager

What version of Bitcoin Core are you using?

v25.0.0

Operating system and version

OpenBSD 7.4 Stable

Machine specifications

No response

BebeSparkelSparkel commented at 8:14 pm on March 4, 2024: none

Seems like this should solve it

struct DatabaseOptions in src/wallet/db.h needs a writePermission field of type bool with a default value of false
the method Open of classSQLiteDatabase in src/wallet/sqlite.h needs a flag argument of type Int added
the SQLiteDatabase constructor in src/wallet/sqlite.cpp needs to pass the flag SQLITE_OPEN_READONLY if options.writePermission is false else flag SQLITE_OPEN_READWRITE
in src/wallet/wallettool.cpp when command == create, salvage, or createfromdump then options.writePermission = true;

PeterWrighten commented at 9:12 am on March 5, 2024: none

I consider this permission issue is necesary to fix, and wanna contribute for it. Is it assigned for other guys or anyone is working on it right now?

PeterWrighten commented at 9:29 am on March 5, 2024: none

Seems like this should solve it

struct DatabaseOptions in src/wallet/db.h needs a writePermission field of type bool with a default value of false

the method Open of classSQLiteDatabase in src/wallet/sqlite.h needs a flag argument of type Int added

the SQLiteDatabase constructor in src/wallet/sqlite.cpp needs to pass the flag SQLITE_OPEN_READONLY if options.writePermission is false else flag SQLITE_OPEN_READWRITE

in src/wallet/wallettool.cpp when command == create, salvage, or createfromdump then options.writePermission = true;

As for me, I consider modifying the code to open write permission is actually an unsafe operation. If you just wanna access info, read-only permission seems sufficient. Perhaps there is also other issue…

Sjors commented at 10:59 am on March 5, 2024: member

cc @achow101

It seems like a good safety feature if commands like bitcoin-wallet info and bitcoin-wallet dump open the database in read-only mode.

Is it assigned for other guys or anyone is working on it right now?

Issues are never assigned. There’s no recent pull request open either. It’s also not the end of the world if two people open a bug fix PR at the same time; reviewers can just pick the best solution.

PeterWrighten commented at 12:10 pm on March 5, 2024: none

cc @achow101

It seems like a good safety feature if commands like bitcoin-wallet info and bitcoin-wallet dump open the database in read-only mode.

Is it assigned for other guys or anyone is working on it right now?

Issues are never assigned. There’s no recent pull request open either. It’s also not the end of the world if two people open a bug fix PR at the same time; reviewers can just pick the best solution.

Thanks! I would work on it and PR when finished it.

achow101 commented at 5:11 pm on March 5, 2024: member

This has been something I’ve looked at before, and it ended up being somewhat complicated and I didn’t have the time to try to properly fix. Even if you can open the databases in readonly mode, you’d still have to make sure that nothing in the call stack is attempting to write the database. There are a couple of places during wallet loading where writes are actually made, and those might fail with a crash, or fail silently, both of which we don’t want to happen.

Also, we have two database types, and both need to be passed the correct flags to open as readonly.

BebeSparkelSparkel commented at 5:19 pm on March 6, 2024: none

@PeterWrighten I have made some modifications but do not know c++ well enough to diagnose where sqlite is requiring write permissions.

https://github.com/BebeSparkelSparkel/bitcoin/tree/readonly-db

PeterWrighten commented at 8:14 pm on March 6, 2024: none

@PeterWrighten I have made some modifications but do not know c++ well enough to diagnose where sqlite is requiring write permissions.

https://github.com/BebeSparkelSparkel/bitcoin/tree/readonly-db

Seems good for sqlite db. But I consider adding permissions in DatabaseOptions is not a good idea. Perhaps adding an OpenReadOnly() Function in SqliteDatabase is better.

Sjors commented at 10:49 am on March 7, 2024: member

Also, we have two database types, and both need to be passed the correct flags to open as readonly.

I think it’s fair to require users to migrate to sqlite if they wish to use a read-only file system for their wallet. So it sounds like the bitcoin-wallet info and bitcoin-wallet dump calls could check if the file is read-only and the format it BDB, and then exit.

That then leaves the complexity of handling unexpected errors with sqlite wallets. Especially for the the dump call a silent failure would be very dangerous, if somehow you can end up with an incomplete dump. Though as long as you end up with no dump it’s fine. @BebeSparkelSparkel:

but do not know c++ well enough to diagnose where sqlite is requiring write permissions.

In that case this might not be the best issue to tackle for now. Not that our c++ usage is very advanced, but the wallet codebase is complicated and requires some familiarity.

(but to be clear: you don’t need permission to try anyway!)

BebeSparkelSparkel commented at 2:52 pm on March 7, 2024: none

@PeterWrighten It appears like it should work but I get an error stating that write permissions are required somewhere. I was trying to use gdb to find the source but was having problems getting the symbol tables into it even when following the instructions https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-debugging

If you could try and let me know if you are able to find where the write permission is required it would be helpful.

BebeSparkelSparkel commented at 2:58 pm on March 7, 2024: none

@Sjors I do not see any proposals to deprecate the BDB in issues. If BDB is meant to be supported, it does not seem like a good idea to have divergence between the two.

Sjors commented at 3:05 pm on March 7, 2024: member

@BebeSparkelSparkel see #20160.

BebeSparkelSparkel commented at 1:15 am on March 8, 2024: none

@Sjors Thanks. Then then what you propose makes sense.

BebeSparkelSparkel commented at 6:07 pm on March 10, 2024: none

@PeterWrighten Any luck?

maflcko added the label Wallet on Mar 11, 2024

maflcko added the label Utils/log/libs on Mar 11, 2024

bitcoin-wallet requires write permissions when unneeded #29559