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

1. 
   willcl-ark commented at 2:08 pm on June 26, 2025: member

   Currently our wallet-tool performs a few “read-only” operations (info, dump) on sqlite databases. However, currently this involves opening the wallet in RW mode, with the side-effects of modifying the wallet file, and failing to open a “read-only” wallet file.

   See #15608 for more context.

   Since we have dropped the BDB wallet, this change got a lot simpler; the BDB parser is now custom and only operates in read-only mode anyway.

   This change adds a read_only bool to DatabaseOptions. This can be used by the sqlite dbwrapper to open the database in readonly mode.

   Includes tests to verify indempotence of wallet tool “read only” operations.

   Fixes: #15608

2. db: add read_only mode to sqlite db options

   Adds a read_only bool to DatabaseOptions. This can be used by the sqlite
   dbwrapper to open the database in readonly mode.
   
   Add helper function to check if we are opened in read_only mode.

   a81ab0bc17
3. db: handle read_only mode flag in sqlitedb c2415dc19e
4. tools: use read-only mode in wallet tool read operations c8734a7aa0
5. test: add wallet tool indempotence tests

   Adds various tests checking that the wallet tool does not modify the
   wallet db file when performing read-only operations, and that it can
   operate on read-only db files.

   5acceed8e4
6. 
   DrahtBot commented at 2:08 pm on June 26, 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/32818.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31423 (wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet by furszy)
   • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

   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.

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • indempotent -> idempotent [“indempotent” is a misspelling of “idempotent”]
   • indempotence -> idempotence [“indempotence” is the noun form of “idempotent,” spelled with an “i”]

   drahtbot_id_4_m

7. 
   fanquake commented at 3:22 pm on June 26, 2025: member

   https://github.com/bitcoin/bitcoin/actions/runs/15904154552/job/44856855999?pr=32818#step:12:437:

    0 test  2025-06-26T14:50:15.271166Z TestFramework (ERROR): Assertion failed 
    1                                   Traceback (most recent call last):
    2                                     File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 190, in main
    3                                       self.run_test()
    4                                       ~~~~~~~~~~~~~^^
    5                                     File "D:\a\bitcoin\bitcoin/test/functional/tool_wallet.py", line 450, in run_test
    6                                       self.test_tool_wallet_info()
    7                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~^^
    8                                     File "D:\a\bitcoin\bitcoin/test/functional/tool_wallet.py", line 164, in test_tool_wallet_info
    9                                       assert self.wallet_permissions() in ['400']
   10                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   11                                   AssertionError
   

8. 
   achow101 commented at 4:00 pm on June 26, 2025: member

   How does this compare with #32685?
9. 
   NowYu commented at 7:47 pm on June 26, 2025: none

   Before I get off
10. 
    willcl-ark commented at 7:55 pm on June 26, 2025: member

    How does this compare with #32685?

    Ah, I didn’t know (or had perhaps forgotten) about that PR. I came via #15608 and did not check what was open. Shame on me :(

    I will mark this as draft and take a look over that PR.

11. willcl-ark marked this as a draft on Jun 26, 2025
12. in test/functional/tool_wallet.py:164 in 5acceed8e4

    177+
    178+        # Test with read-only file permissions
    179+        self.log.info('Testing wallet tool info with read-only file permissions')
    180+        self.log.debug('Setting wallet file permissions to 400 (read-only)')
    181+        os.chmod(self.wallet_path, stat.S_IRUSR)
    182+        assert self.wallet_permissions() in ['400']
    

    ---

    ---

    

    PeterWrighten commented at 9:04 am on June 27, 2025:

    In this code, you should add more seeds number of platform to pass CI.

    ---

    

    pinheadmz commented at 10:05 am on June 27, 2025:

    🤔
13. bitcoin deleted a comment on Jun 27, 2025
14. 
    PeterWrighten commented at 9:07 am on June 27, 2025: none

    How does this compare with #32685?

    Ah, I didn’t know (or had perhaps forgotten) about that PR. I came via #15608 and did not check what was open. Shame on me :(

    I will mark this as draft and take a look over that PR.

    Never mind. Your implementation is also great. But seems there is lack of some tests for walletdb and db…

15. 
    ryanofsky commented at 3:03 pm on June 27, 2025: contributor

    Note IIRC, the SQLITE_OPEN_READONLY flag only guarantees that sqlite won’t try to write to the main database file, not any wal or jrn files associated with the database. So if the goal is being able to access wallets on read-only filesystems this may not always work and the immutable mode might need to be used instead.

Contributors
willcl-ark DrahtBot fanquake achow101 NowYu PeterWrighten pinheadmz ryanofsky