wallet: Improve log output for errors during load #15491

gwillen · 2019-02-27T02:27:18Z

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.

src/wallet/wallet.cpp

src/wallet/walletdb.cpp

DrahtBot · 2019-02-27T04:43:16Z

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

Conflicts

No conflicts as of last run.

Sjors · 2019-02-27T08:44:21Z

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

gwillen · 2019-02-27T08:54:58Z

@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.)

Sjors · 2019-02-27T13:56:05Z

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.

instagibbs · 2019-02-27T14:04:47Z

@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.

instagibbs · 2019-02-27T14:05:23Z

utACK e5637b1

promag

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

src/wallet/walletdb.cpp

gwillen · 2019-03-04T08:42:07Z

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.

gwillen · 2019-03-06T04:36:14Z

Rebased.

promag · 2019-03-06T09:25:49Z

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

promag · 2019-03-06T09:28:57Z

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

gwillen · 2019-03-06T20:20:55Z

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.

gwillen · 2019-03-06T21:24:44Z

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.)

gwillen · 2019-03-06T21:29:49Z

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?

ryanofsky · 2019-03-06T22:09:06Z

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()

gwillen · 2019-03-08T01:57:11Z

@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.)

ryanofsky · 2019-03-08T11:00:43Z

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.)

gwillen · 2019-03-13T00:22:08Z

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

ryanofsky

utACK 17d0114

It would be good to update the PR description #15491 (comment) which still references "[default wallet]".

promag

utACK 17d0114.

promag · 2019-03-13T00:40:58Z

src/wallet/db.cpp

@@ -84,6 +84,13 @@ bool IsWalletLoaded(const fs::path& wallet_path)
    return database && database->IsDatabaseLoaded(database_filename);
 }

+fs::path WalletDataFilePath(const fs::path& wallet_path) {


nit, { should be in new line.

Curses, done.

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

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.

gwillen · 2019-03-15T01:53:07Z

Fixed the PR description.

Fixed the brace @promag was complaining about.

No other changes.

meshcollider · 2019-03-18T07:36:52Z

utACK faf3698

faf3698 wallet: Improve log output for errors during load (Glenn Willen) Pull request description: 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. Tree-SHA512: 435247628db669579bb694ba4b53ba174fe42c0329fc72f09fc274bb28463ee69f53412abb2a3b45bb8f59f7eb928c0167e397b8d0a514135142192a87294614

maflcko reviewed Feb 27, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

fanquake added the Wallet label Feb 27, 2019

instagibbs reviewed Feb 27, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

gwillen force-pushed the feature-improve-wallet-load-debug-output branch 2 times, most recently from 4b4127b to e5637b1 Compare February 27, 2019 02:43

promag reviewed Mar 3, 2019

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

gwillen force-pushed the feature-improve-wallet-load-debug-output branch from e5637b1 to 3bd35f2 Compare March 4, 2019 08:39

DrahtBot added the Needs rebase label Mar 4, 2019

gwillen force-pushed the feature-improve-wallet-load-debug-output branch from 3bd35f2 to 0eea55f Compare March 6, 2019 04:36

DrahtBot removed the Needs rebase label Mar 6, 2019

gwillen force-pushed the feature-improve-wallet-load-debug-output branch from 0eea55f to aa711d9 Compare March 6, 2019 21:23

gwillen force-pushed the feature-improve-wallet-load-debug-output branch from aa711d9 to 7cf7290 Compare March 8, 2019 01:54

gwillen force-pushed the feature-improve-wallet-load-debug-output branch from 7cf7290 to 17d0114 Compare March 13, 2019 00:22

ryanofsky approved these changes Mar 14, 2019

View reviewed changes

promag reviewed Mar 14, 2019

View reviewed changes

gwillen force-pushed the feature-improve-wallet-load-debug-output branch from 17d0114 to faf3698 Compare March 15, 2019 01:49

meshcollider merged commit faf3698 into bitcoin:master Mar 18, 2019

practicalswift mentioned this pull request Aug 19, 2019

wallet: Fix segfault in CreateWalletFromFile, Pass error to rpc caller #16661

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: Improve log output for errors during load #15491

wallet: Improve log output for errors during load #15491

gwillen commented Feb 27, 2019 •

edited

DrahtBot commented Feb 27, 2019 •

edited

Sjors commented Feb 27, 2019

gwillen commented Feb 27, 2019 •

edited

Sjors commented Feb 27, 2019

instagibbs commented Feb 27, 2019

instagibbs commented Feb 27, 2019

promag left a comment

gwillen commented Mar 4, 2019

gwillen commented Mar 6, 2019

promag commented Mar 6, 2019

promag commented Mar 6, 2019

gwillen commented Mar 6, 2019

gwillen commented Mar 6, 2019

gwillen commented Mar 6, 2019

ryanofsky commented Mar 6, 2019

gwillen commented Mar 8, 2019

ryanofsky commented Mar 8, 2019

gwillen commented Mar 13, 2019

ryanofsky left a comment

promag left a comment

promag Mar 13, 2019

gwillen Mar 15, 2019

gwillen commented Mar 15, 2019

meshcollider commented Mar 18, 2019

wallet: Improve log output for errors during load #15491

wallet: Improve log output for errors during load #15491

Conversation

gwillen commented Feb 27, 2019 • edited

DrahtBot commented Feb 27, 2019 • edited

Conflicts

Sjors commented Feb 27, 2019

gwillen commented Feb 27, 2019 • edited

Sjors commented Feb 27, 2019

instagibbs commented Feb 27, 2019

instagibbs commented Feb 27, 2019

promag left a comment

Choose a reason for hiding this comment

gwillen commented Mar 4, 2019

gwillen commented Mar 6, 2019

promag commented Mar 6, 2019

promag commented Mar 6, 2019

gwillen commented Mar 6, 2019

gwillen commented Mar 6, 2019

gwillen commented Mar 6, 2019

ryanofsky commented Mar 6, 2019

gwillen commented Mar 8, 2019

ryanofsky commented Mar 8, 2019

gwillen commented Mar 13, 2019

ryanofsky left a comment

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

promag Mar 13, 2019

Choose a reason for hiding this comment

gwillen Mar 15, 2019

Choose a reason for hiding this comment

gwillen commented Mar 15, 2019

meshcollider commented Mar 18, 2019

gwillen commented Feb 27, 2019 •

edited

DrahtBot commented Feb 27, 2019 •

edited

gwillen commented Feb 27, 2019 •

edited