wallet: Improve log output for errors during load #15491

pull gwillen wants to merge 1 commits into bitcoin:master from gwillen:feature-improve-wallet-load-debug-output changing 4 files +21 −3
  1. gwillen commented at 2:27 AM on February 27, 2019: contributor

    When loading the wallet, display the entire path in error messages, instead of the name (which, for the default wallet, is the empty string.)

    When an exception occurs during wallet loading, display e.what() if possible, instead of nothing.

  2. in src/wallet/wallet.cpp:4100 in 4bfac14efa outdated 
    4096 | @@ -4097,7 +4097,7 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b
4097 |  
4098 |  std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, uint64_t wallet_creation_flags)
4099 |  {
4100 | -    const std::string& walletFile = location.GetName();
4101 | +    const std::string& walletFile = (location.GetName() == "" ? "[default wallet]" : location.GetName());
    MarcoFalke commented at 2:31 AM on February 27, 2019: 
        const std::string& log_wallet_name = (location.GetName() == "" ? "[default wallet]" : location.GetName());

    nit: I'd feel more safe if this was renamed to log_wallet_name

    instagibbs commented at 2:32 AM on February 27, 2019:

    alternatively, why not just have [default wallet] be the thing set in the constructor for WalletLocation and just remove the need for this special casing?

        const std::string& walletFile = location.GetName().empty() ? "[default wallet]" : location.GetName();
    gwillen commented at 2:36 AM on February 27, 2019:

    that makes sense, I will do that.

    gwillen commented at 2:40 AM on February 27, 2019:

    I was uncomfortable with modifying it in WalletLocation because I don't fully understand all the uses that the 'wallet name' is put to, and it seems like sometimes it's used as a path/filename.

    gwillen commented at 2:42 AM on February 27, 2019:

    Done.

  3. fanquake added the label Wallet on Feb 27, 2019
  4. in src/wallet/walletdb.cpp:433 in 4bfac14efa outdated 
     428 | +            strErr = e.what();
 429 | +        }
 430 | +        return false;
 431 |      } catch (...)
 432 |      {
 433 | +        if (strErr == "") {
    instagibbs commented at 2:36 AM on February 27, 2019: 
            if (strErr.empty()) {
    gwillen commented at 2:42 AM on February 27, 2019:

    Done.

  5. in src/wallet/walletdb.cpp:427 in 4bfac14efa outdated 
     421 | @@ -422,8 +422,17 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 422 |                  strType != "minversion" && strType != "acentry") {
 423 |              wss.m_unknown_records++;
 424 |          }
 425 | +    } catch (const std::exception& e)
 426 | +    {
 427 | +        if (strErr == "") {
    instagibbs commented at 2:36 AM on February 27, 2019: 
            if (strErr.empty()) {
    gwillen commented at 2:42 AM on February 27, 2019:

    Done.

  6. gwillen force-pushed on Feb 27, 2019
  7. gwillen force-pushed on Feb 27, 2019
  8. DrahtBot commented at 4:43 AM on February 27, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  9. Sjors commented at 8:44 AM on February 27, 2019: member

    tACK e5637b1

    I now get a flood of this with one my broken wallets:

    2019-02-27T08:32:30Z [default wallet] CDataStream::read(): end of data: unspecified iostream_category error
2019-02-27T08:32:30Z [default wallet] CDataStream::read(): end of data: unspecified iostream_category error
2019-02-27T08:32:30Z [default wallet] CDataStream::read(): end of data: unspecified iostream_category error
  10. gwillen commented at 8:54 AM on February 27, 2019: contributor

    @Sjors Yep, that's the same thing I get -- at least in my case, each of those is a 'keymeta' entry, whose value is missing the final has_key_origin field of CKeyMetaData.

    I think the flood of log entries is definitely better than the lack of any, although the volume is a little annoying (but I think it makes sense when encountering a broken wallet file to be verbose about it.)

  11. Sjors commented at 1:56 PM on February 27, 2019: member

    It's fine. This is the type of error that should really never happen in the real world, so indeed when it does, the more information the better.

  12. instagibbs commented at 2:04 PM on February 27, 2019: member

    @Sjors frankly these type of changes are mostly for developer's sanity. I've run into similar issues with wallet entries and felt compelled to upstream changes.

  14. in src/wallet/walletdb.cpp:425 in e5637b1dca outdated 
     421 | @@ -422,8 +422,17 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 422 |                  strType != "minversion" && strType != "acentry") {
 423 |              wss.m_unknown_records++;
 424 |          }
 425 | +    } catch (const std::exception& e)
    promag commented at 4:55 PM on March 3, 2019:

    nit, { here.

    gwillen commented at 8:39 AM on March 4, 2019:

    Fixed. It's annoying that the linter doesn't catch stuff like this. Then the wrong version can crop up in the code, and people like me can copy it. :-)

  15. promag commented at 4:57 PM on March 3, 2019: member

    Does it make sense to commit some broken wallets to the repo?

  16. gwillen force-pushed on Mar 4, 2019
  17. gwillen commented at 8:42 AM on March 4, 2019: contributor

    The idea of committing a broken wallet as a test case makes sense to me, but (1) I seem to recall I've heard people say in past cases that committing a binary wallet file to the repo is not reasonable even as test data, and (2) I definitely don't know how to go about minimizing such a test case, which seems both necessary and painful.

  18. DrahtBot added the label Needs rebase on Mar 4, 2019
  19. gwillen force-pushed on Mar 6, 2019
  20. gwillen commented at 4:36 AM on March 6, 2019: contributor

    Rebased.

  21. DrahtBot removed the label Needs rebase on Mar 6, 2019
  22. promag commented at 9:25 AM on March 6, 2019: member

    Restarted travis job 6 - failed on commit 4e7fea2 with:

    47/118 - rpc_psbt.py failed, Duration: 4 s
stdout:
2019-03-06T05:02:08.027000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190306_045534/rpc_psbt_69
2019-03-06T05:02:11.006000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 175, in main
    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/rpc_psbt.py", line 149, in run_test
    assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].converttopsbt, signedtx['hex'])
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 105, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
AssertionError: No exception raised
  23. promag commented at 9:28 AM on March 6, 2019: member

    @gwillen have you seen #15334? Do you think logging the absolute path makes it more clear - instead of "[default wallet]"?

  24. gwillen commented at 8:20 PM on March 6, 2019: contributor

    Yeah, I think switching to absolute path would make sense. I assume I just need to switch from GetName to GetPath. Will try it out.

  25. gwillen force-pushed on Mar 6, 2019
  26. gwillen commented at 9:24 PM on March 6, 2019: contributor

    Switched to GetPath().string() instead of GetName(). Removed the rename to log_wallet_name, in the interest of minimizing the size of the diff, since it is once again a wallet file name. (I can reinstate the rename if preferred.)

  27. gwillen commented at 9:29 PM on March 6, 2019: contributor

    Oh, no, this didn't do what I wanted at all. On further inspection of #15334, it doesn't actually log the absolute path, it just logs the filename, but more importantly it doesn't use WalletLocation::GetPath, but it has some other place it's getting this info. GetPath is giving me the wallet directory, excluding the filename, it seems.

    Can someone clue me in to the exact semantics / interpretation of the name and path in WalletLocation? Can I just slap "wallet.dat" on the end of the output of GetPath and be good, or are there other cases?

  28. ryanofsky commented at 10:09 PM on March 6, 2019: member

    I'd suggest adding a new method to the BerkeleyDatabase class:

    +    fs::path DataFilePath() const { return env->Directory() / strFile; }

    and printing the path with GetDBHandle().DataFilePath().string()

  29. gwillen force-pushed on Mar 8, 2019
  30. gwillen commented at 1:57 AM on March 8, 2019: contributor

    @ryanofsky I ended up taking a slightly different approach, because at this point we do not have a CWallet yet to get a db handle from (we are in the flow to open one.) Instead, I added a free function for normalizing paths.

    (I could have accomplished the same thing without adding a function, by calling SplitWalletPath (but it's static, and a slightly weird interface for this) or GetWalletEnv (but it's a very weird interface for this, and also it manipulates g_dbenvs, which I don't know the consequences of and seems a little heavy-handed for just figuring out a path.)

  31. ryanofsky commented at 11:00 AM on March 8, 2019: member

    Looks good if you s/wallet database file/data file path/ and s/FullWalletFilePath/WalletDataFilePath/.

    I'd suggest:

    /** Given a wallet directory path or legacy file path, return path to main data file in the wallet database. */
fs::path WalletDataFilePath(const fs::path& wallet_path);

    Wallet databases are really directories, not files. At a low level, wallet databases include not just wallet.dat files but also write-ahead database/log.?????????? files. At the user level, the -wallet option and createwallet rpc create new wallets as directories, and the -rpcwallet option and getwalletinfo rpc use directory paths.

    The only reason it makes sense to reference files instead of directories in log messages is to disambiguate in the legacy case where there can be multiple data files for different wallets stored in the same directory and sharing mixed log files. (This was a misfeature which is mostly removed, except we still support loading these wallets in the top level walletdir.)

  32. gwillen commented at 12:22 AM on March 13, 2019: contributor

    Got it, thanks @ryanofsky , makes sense. Changed as suggested!

  33. gwillen force-pushed on Mar 13, 2019
  34. in src/wallet/db.cpp:87 in 17d0114bc2 outdated 
      83 | @@ -84,6 +84,13 @@ bool IsWalletLoaded(const fs::path& wallet_path)
  84 |      return database && database->IsDatabaseLoaded(database_filename);
  85 |  }
  86 |  
  87 | +fs::path WalletDataFilePath(const fs::path& wallet_path) {
    promag commented at 12:40 AM on March 13, 2019:

    nit, { should be in new line.

    gwillen commented at 1:52 AM on March 15, 2019:

    Curses, done.

    I'm afraid my brain is stuck in the wrong style for life, sorry.

  35. ryanofsky approved
  36. ryanofsky commented at 8:32 PM on March 14, 2019: member

    utACK 17d0114bc2e307cdf7feba7d12adc71dfc4f9d6d

    It would be good to update the PR description #15491#issue-256517395 which still references "[default wallet]".

  37. promag commented at 8:34 PM on March 14, 2019: member

    utACK 17d0114.

  38. wallet: Improve log output for errors during load
    When loading the wallet, display the entire path in error messages, instead of
the name (which, for the default wallet, is the empty string.)

When an exception occurs during wallet loading, display e.what() if possible,
instead of nothing.
    faf3698808
  39. gwillen force-pushed on Mar 15, 2019
  40. gwillen commented at 1:53 AM on March 15, 2019: contributor

    Fixed the PR description.

    Fixed the brace @promag was complaining about.

    No other changes.

  42. meshcollider merged this on Mar 18, 2019
  43. meshcollider closed this on Mar 18, 2019
  44. meshcollider referenced this in commit 7ec7aea442 on Mar 18, 2019
  45. linuxsh2 referenced this in commit a0bee299b9 on Aug 11, 2021
  46. DrahtBot locked this on Dec 16, 2021
Contributors
gwillenMarcoFalkeinstagibbsDrahtBotSjorspromagryanofskymeshcollider
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-14 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me