rpc: Improve error when wallet is already loaded #26192

pull aureleoules wants to merge 1 commits into bitcoin:master from aureleoules:2022-09-load-wallet-err changing 2 files +10 −6
  1. aureleoules commented at 1:15 PM on September 28, 2022: member

    Currently, trying to load a descriptor (sqlite) wallet that is already loaded throws the following error:

    error code: -4 error message: Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?

    I don't think it is very clear what it means for a user.

    While a legacy wallet would throw:

    error code: -35 error message: Wallet file verification failed. Refusing to load database. Data file '/home/user/.bitcoin/signet/wallets/test_wallet/wallet.dat' is already loaded.

    This PR changes the error message for both types of wallet to:

    error code: -35 error message: Wallet file verification failed. Wallet "test_wallet" is already loaded.

  2. DrahtBot commented at 2:59 PM on September 28, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hernanmarino, theStack, achow101
    Concept ACK rajarshimaitra, pablomartin4btc
    Stale ACK w0xlt

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. glozow added the label Wallet on Sep 28, 2022
  4. aureleoules force-pushed on Sep 28, 2022
  5. in test/functional/wallet_multiwallet.py:307 in ca8eba38a1 outdated 
     303 | @@ -304,9 +304,9 @@ def wallet_file(name):
 304 |          # Fail to load duplicate wallets
 305 |          path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w1", "wallet.dat")
 306 |          if self.options.descriptors:
 307 | -            assert_raises_rpc_error(-4, f"Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of {self.config['environment']['PACKAGE_NAME']}?", self.nodes[0].loadwallet, wallet_names[0])
 308 | +            assert_raises_rpc_error(-35, f"Wallet file verification failed. Wallet \"{wallet_names[0]}\" is already loaded.", self.nodes[0].loadwallet, wallet_names[0])
    ryanofsky commented at 8:49 PM on September 29, 2022:

    In commit "rpc: Improve error when wallet is already loaded" (ca8eba38a19a3c1eac8fc41ae7f9f9a17660710c)

    Note for other reviewers: it seemed like a potential concern that this test is dropping coverage for the "Unable to obtain an exclusive lock" error, which can still be triggered. But it looks like there is another check for that error in in feature_filelock.py, so this should be ok.

  6. in src/wallet/wallet.cpp:2792 in ca8eba38a1 outdated 
    2783 | @@ -2784,6 +2784,20 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
2784 |          status = DatabaseStatus::FAILED_BAD_PATH;
2785 |          return nullptr;
2786 |      }
2787 | +
2788 | +    {
2789 | +        LOCK(context.wallets_mutex);
2790 | +        if (std::any_of(context.wallets.begin(), context.wallets.end(), [&wallet_path](const auto& wallet) {
2791 | +                // fs::equivalent alternative
2792 | +                // return SQLiteDataFile(wallet_path) == wallet->GetDatabase().Filename();
    aureleoules commented at 8:54 PM on September 29, 2022:

    forgot to remove this, will remove if this gets concept acked

  7. luke-jr commented at 1:01 AM on October 1, 2022: member

    Instead of getting this deep into loading, maybe just check if the wallet name is already loaded?

  8. aureleoules force-pushed on Oct 1, 2022
  9. aureleoules force-pushed on Oct 1, 2022
  10. aureleoules force-pushed on Oct 1, 2022
  11. aureleoules commented at 11:57 PM on October 1, 2022: member

    Instead of getting this deep into loading, maybe just check if the wallet name is already loaded?

    Yes that makes more sense, pushed it. For some reason I wanted to implement it the same way it was done for the legacy wallet, but this is better.

  12. rajarshimaitra approved
  13. rajarshimaitra commented at 1:53 PM on October 16, 2022: contributor

    Concept ACK on unifying the wallet loading error message. I remember this caused me confusion and took some time to figure that its an "already loaded" error.

    Code changes looks clean and tests performing as expected..

  14. w0xlt approved
  16. in test/functional/wallet_multiwallet.py:305 in 6cde3551e4 outdated 
     302 | @@ -303,11 +303,8 @@ def wallet_file(name):
 303 |  
 304 |          # Fail to load duplicate wallets
 305 |          path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w1", "wallet.dat")
    hernanmarino commented at 4:13 PM on October 18, 2022:

    A small nit, I think this line is not really needed, since the variable path is not used anymore with the new error message.

  17. hernanmarino changes_requested
  18. hernanmarino commented at 4:16 PM on October 18, 2022: contributor

    tested ACK, suggested a small nit as a comment.

  19. pablomartin4btc commented at 4:57 PM on October 18, 2022: member

    Tested ACK, verified the errors before and after this fix debugging the python funtional test. It was clever to check the wallet name on the context at wallet.cpp to avoid the db error.

  20. rpc: Improve error when wallet is already loaded 04609284ad
  21. aureleoules force-pushed on Oct 20, 2022
  22. aureleoules commented at 9:57 AM on October 20, 2022: member

    Removed unused line.

  23. aureleoules requested review from hernanmarino on Oct 20, 2022
  24. aureleoules removed review request from hernanmarino on Oct 20, 2022
  25. aureleoules requested review from w0xlt on Oct 20, 2022
  26. aureleoules removed review request from w0xlt on Oct 20, 2022
  27. aureleoules requested review from pablomartin4btc on Oct 20, 2022
  28. pablomartin4btc approved
  29. pablomartin4btc commented at 2:30 PM on October 21, 2022: member

    re-ack 0460928

  30. hernanmarino approved
  31. hernanmarino commented at 2:36 PM on October 21, 2022: contributor

    ACK 0460928

  32. theStack approved
  33. theStack commented at 12:49 PM on January 3, 2023: contributor

    Tested ACK 04609284ad5e0b72651f2d4b43263461ada40816

  34. maflcko assigned achow101 on Jan 3, 2023
  35. achow101 commented at 5:50 PM on January 3, 2023: member

    ACK 04609284ad5e0b72651f2d4b43263461ada40816

  36. achow101 merged this on Jan 3, 2023
  37. achow101 closed this on Jan 3, 2023
  38. sidhujag referenced this in commit c24918cb27 on Jan 4, 2023
  39. aureleoules deleted the branch on Jan 12, 2023
  40. bitcoin locked this on Jan 12, 2024
Contributors
aureleoulesDrahtBotryanofskyluke-jrrajarshimaitraw0xlthernanmarinopablomartin4btctheStackachow101
Labels
Wallet
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-04-21 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me