wallet: avoid a crash when creating a wallet with -nosettings #36176

pull Rob1Ham wants to merge 2 commits into bitcoin:master from Rob1Ham:fix/wallet-disabled-settings-upstream changing 3 files +34 −1
  1. Rob1Ham commented at 8:07 PM on September 5, 2026: contributor

    I hit a crash while creating a new wallet in Bitcoin-Qt 31.1 on an Apple silicon Mac with nosettings=1. After looking through the crash report and code, I traced it to saving the wallet's load-on-startup setting: the settings writer throws when dynamic settings are disabled.

    Wallet RPCs report errors with -nosettings after changing wallet state. In Qt, the same settings write causes an uncaught exception.

    Return a persistence failure when dynamic settings are disabled so wallet operations finish with their existing startup-setting warning. This avoids an uncaught exception in Qt and RPC errors after the wallet state has already changed. Keep in-memory and no-op updates unchanged.

    The first commit adds functional coverage for the current behavior. The second adds the fix, updates the assertions to expect success with warnings, and documents that failed settings writes keep the in-memory changes.

    Manual Reproduction

    Run on the parent commit and the fixed commit, using a fresh temporary regtest data directory each time:

    { cmake -B build-wallet-review -DBUILD_GUI=ON && cmake --build build-wallet-review -j --target bitcoin-qt; } >/dev/null 2>&1
build-wallet-review/bin/bitcoin-qt -regtest -datadir="$(mktemp -d)" -nosettings -noconnect

    Choose File > Create Wallet..., enter repro, leave the defaults unchanged, and click Create.

    Before the fix, the application terminates with:

    libc++abi: terminating due to uncaught exception of type std::logic_error: Attempt to write settings file when dynamic settings are disabled.

    After the fix, the wallet is created and the application displays:

    Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.
  2. DrahtBot added the label Wallet on Sep 5, 2026
  3. DrahtBot commented at 8:07 PM on September 5, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36176.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK l0rinc, jeanpablojp, kevkevinpal, achow101

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. in test/functional/wallet_startup.py:71 in a6dc9ccf7f 
      64 | @@ -64,6 +65,50 @@ def test_load_unwritable_wallet(self, node):
  65 |          # Reset directory permissions for cleanup
  66 |          dir_path.chmod(original_dir_perms)
  67 |  
  68 | +    def test_disabled_settings(self, node):
  69 | +        self.log.info("Test wallet startup preferences with dynamic settings disabled")
  70 | +
  71 | +        def wallet_rpc(method, **kwargs):
    achow101 commented at 9:56 PM on September 5, 2026:

    What is the point of this function?

    Rob1Ham commented at 10:22 PM on September 5, 2026:

    It was there to capture the pre-fix RPC errors in the characterization commit, so the fix commit only changed the expected responses. It isn’t needed in the final test, though; direct RPC calls would be simpler.

    I'll update that now when I remove the unit tests.

  5. achow101 commented at 9:59 PM on September 5, 2026: member

    I don't think a unit test is necessary for this, the functional test suffices.

  6. Rob1Ham commented at 10:05 PM on September 5, 2026: contributor

    I don't think a unit test is necessary for this, the functional test suffices.

    Happy to remove the unit test

  7. Rob1Ham force-pushed on Sep 5, 2026
  8. DrahtBot added the label CI failed on Sep 5, 2026
  9. Rob1Ham commented at 10:36 PM on September 5, 2026: contributor

    Updated the two-commit stack to remove the unit test and use direct RPC calls in the fixed functional test.

    The first commit characterizes the existing errors; the second applies the fix and checks for success with warnings. Functional tests pass at both commits.

  10. l0rinc commented at 10:57 PM on September 5, 2026: contributor

    Concept ACK

    I reproduced the crash with a local Qt build: run the following before and after the fix (consider adding it as manual reproducer to the PR description):

    { cmake -B build-wallet-review -DBUILD_GUI=ON && cmake --build build-wallet-review -j --target bitcoin-qt; } >/dev/null 2>&1
build-wallet-review/bin/bitcoin-qt -regtest -datadir="$(mktemp -d)" -nosettings -noconnect

    Then choose File → Create Wallet…, enter repro, leave the defaults unchanged, and click Create.

    Before the fix, the application crashes with:

    libc++abi: terminating due to uncaught exception of type std::logic_error: Attempt to write settings file when dynamic settings are disabled.

    After the fix, the wallet is created and the application displays:

    Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.

    +1 for keeping only the functional test and note that each commit should also build and pass its tests independently.

    I also put together a simplified version of the latest stack. It reuses w2 and moves the explicit unload last to remove two restarts, compares exact RPC responses instead of defaulting missing warnings to [], shares the repeated warning text, and documents that a failed settings write keeps the in-memory update.

  11. Rob1Ham force-pushed on Sep 6, 2026
  12. test: characterize disabled wallet settings
    Wallet RPCs report errors with -nosettings after changing wallet state.
Check these results alongside wallet usability, unchanged settings.json,
and restored startup preferences when settings are enabled again.

Reuse an existing wallet for loading and check explicit unloading last,
so the sequence does not depend on the skipped unload completion wait.
    b7113e6f42
  13. wallet: handle disabled startup settings
    Return a persistence failure when dynamic settings are disabled so
wallet operations finish with their existing startup-setting warning.
This avoids an uncaught exception in Qt and RPC errors after the wallet
state has already changed. Keep in-memory and no-op updates unchanged.
    a34fc8b11a
  14. Rob1Ham force-pushed on Sep 6, 2026
  15. l0rinc commented at 1:07 AM on September 6, 2026: contributor

    tested ACK a34fc8b11a420c1ed9877a5dd73aab57aba17cd0

    New push simplifies the regression test and clarifies how wallet operations can complete when settings cannot be saved.

  16. Rob1Ham commented at 1:24 AM on September 6, 2026: contributor

    I also put together a simplified version of the latest stack. It reuses w2 and moves the explicit unload last to remove two restarts, compares exact RPC responses instead of defaulting missing warnings to [], shares the repeated warning text, and documents that a failed settings write keeps the in-memory update.

    Thanks for the review! Updated

  17. DrahtBot removed the label CI failed on Sep 6, 2026
  18. jeanpablojp commented at 6:14 PM on September 6, 2026: contributor

    tACK a34fc8b11a420c1ed9877a5dd73aab57aba17cd0

    Built with the GUI and ran wallet_startup.py. Reverting the GetSettingsPath() guard makes createwallet fail again with the old -1 error, so the new test really does cover the fix.

  19. in test/functional/wallet_startup.py:81 in a34fc8b11a 
      76 | +        assert_equal(node.createwallet(wallet_name="no_settings", load_on_startup=True), {"name": "no_settings", "warnings": [load_message]})
  77 | +        assert_equal(set(node.listwallets()), {'', 'no_settings'})
  78 | +
  79 | +        # Leaving the startup preference unchanged does not warn, and the wallet remains usable.
  80 | +        assert_equal(node.unloadwallet(wallet_name="no_settings"), {})
  81 | +        assert_equal(node.loadwallet(filename="no_settings"), {"name": "no_settings"})
    jeanpablojp commented at 6:14 PM on September 6, 2026:

    Worth covering the other side of the new guard here? If the GetSettingsPath() check moves before the ||, SKIP_WRITE starts returning false, so reloading a wallet already in the startup list warns and nothing in this test fails. Passing the flag on this reload catches that, and the same assertion holds at the first commit, so it does not need to change between the two.

            # Re-adding a wallet already in the startup list skips the write, so it does not warn either.
        assert_equal(node.loadwallet(filename="no_settings", load_on_startup=True), {"name": "no_settings"})
  20. kevkevinpal commented at 2:12 PM on September 7, 2026: contributor

    tACK a34fc8b11a420c1ed9877a5dd73aab57aba17cd0

    Ran wallet_startup.py on the parent (RPC -1 after the wallet state already changes) and on this commit (success + the existing startup-setting warning, settings.json unchanged). Checking GetSettingsPath() before WriteSettingsFile() is the right layer. UpdateWalletSetting already treats a failed write as a warning.

  21. achow101 commented at 11:00 PM on September 7, 2026: member

    ACK a34fc8b11a420c1ed9877a5dd73aab57aba17cd0

  22. achow101 merged this on Sep 7, 2026
  23. achow101 closed this on Sep 7, 2026
Contributors
Rob1HamDrahtBotachow101l0rincjeanpablojpkevkevinpal
Labels
Wallet
Linked (view graph)
#36180 qt: don't abort or drop settings when dynamic settings are disabled
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-09-09 07:56 UTC