wallet: List all wallets in non-SQLite and non-BDB builds

ryanofsky commented at 9:19 pm on October 30, 2020: member

This PR does not change behavior when bitcoin is built normally with both the SQLite and BDB libraries. It just makes non-SQLite and non-BDB builds more similar to the normal build. Specifically:

It makes wallet directory lists always include all wallets so wallets don’t appear missing depending on the build.
It now triggers specific “Build does not support SQLite database format” and “Build does not support Berkeley DB database format” errors if a wallet can’t be loaded instead of the more ambiguous and scary “Data is not in recognized format” error.

Both changes are implemented in the last commit. The previous commits are just refactoring cleanups that make the last commit possible and consolidate and reduce code.

fanquake added the label Wallet on Oct 30, 2020

hebasto commented at 9:34 pm on October 30, 2020: member

Concept ACK.

In 606622195fdf6a1fb6adc41d1091ce2ae7501125 commit message:

“Remove circular dependencies between wallet translation units”

What are those dependencies?

ryanofsky commented at 9:47 pm on October 30, 2020: member

What are those dependencies?

Oh, I think I need to drop that comment. The dependency I was thinking of was walletutil.cpp depending on bdb.cpp for ExistsBerkeleyDatabase, and bdb.cpp depending on walletutil.cpp for SplitWalletPath. And I thought there were others. But actually SplitWalletPath is in db.cpp not walletutil.cpp, and now I don’t think there are other dependencies.

DrahtBot commented at 11:58 pm on October 30, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20206 (wallet, refactor: Include headers instead of function declarations by hebasto)
#19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)

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.

in src/wallet/db.cpp:30 in 9d1fd52952 outdated

26@@ -34,10 +27,10 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
27         const fs::path path = it->path().string().substr(offset);
28 
29         if (it->status().type() == fs::directory_file &&
30-            (ExistsBerkeleyDatabase(it->path()) || ExistsSQLiteDatabase(it->path()))) {
31+            (IsBDBFile(BDBDataFile(it->path())) || IsSQLiteFile(SQLiteDataFile(it->path())))) {

promag commented at 4:23 pm on November 1, 2020:

9d1fd52952cb6a944e7de4dd7fca241e3341894b

As a next step it would be nice to refactor this and MakeWalletDatabase to avoid duplicate code. Something like:

0DatabaseStatus CheckWalletDatabase(const fs::path& path);
1DatabaseStatus CheckWalletDatabase(const std::string& name)
2{
3    return CheckWalletDatabase(fs::absolute(name, GetWalletDir()));
4}

Which would be called in both places.

ryanofsky commented at 2:26 pm on November 4, 2020:

re: #20275 (review)

9d1fd52

As a next step it would be nice to refactor this and MakeWalletDatabase to avoid duplicate code.

Definitely could be a next step. I’d be a little skeptical of adding another similar sounding function when it’s nice to have fewer distinct functions, but would have to see what this simplifies

promag commented at 4:23 pm on November 1, 2020: member

Concept ACK.

in src/wallet/db.cpp:87 in e21dc6c05c outdated

83+    auto size = fs::file_size(path, ec);
84+    if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
85+    if (size < 4096) return false;
86+
87+    fsbridge::ifstream file(path, std::ios::binary);
88+    if (!file.is_open()) return false;

mjdietzx commented at 9:13 pm on November 1, 2020:

Is it worth breaking this into a function so IsSQLiteFile can re-use this as well? With minSize is as parameter

ryanofsky commented at 2:35 pm on November 4, 2020:

re: #20275 (review)

Is it worth breaking this into a function so IsSQLiteFile can re-use this as well? With minSize is as parameter

This is preexisting code in a move-only function, but it seems like a reasonable thing to do. Not sure if you think this would be clearer or just minimize code or have another benefit.

in src/wallet/db.cpp:77 in e21dc6c05c outdated

73+    return path / "wallet.dat";
74+}
75+
76+bool IsBDBFile(const fs::path& path)
77+{
78+    if (!fs::exists(path)) return false;

mjdietzx commented at 9:17 pm on November 1, 2020:

Is this check necessary?

ryanofsky commented at 2:34 pm on November 4, 2020:

re: #20275 (review)

Is this check necessary?

This is preexisting code in a move-only function. If you wanted to drop this line you would need to add an error_code check below to avoid logging an error if the path is not found. That’s probably the way I would do it, but I don’t think the difference is significant.

in src/wallet/db.cpp:91 in e21dc6c05c outdated

87+    fsbridge::ifstream file(path, std::ios::binary);
88+    if (!file.is_open()) return false;
89+
90+    file.seekg(12, std::ios::beg); // Magic bytes start at offset 12
91+    uint32_t data = 0;
92+    file.read((char*) &data, sizeof(data)); // Read 4 bytes of file to compare against magic

mjdietzx commented at 9:18 pm on November 1, 2020:

Do you need to close the file?

ryanofsky commented at 2:36 pm on November 4, 2020:

re: #20275 (review)

Do you need to close the file?

No need, it should be closed in the destructor. (Also this is more preexisting code in the move-only function)

ryanofsky commented at 10:46 pm on November 1, 2020: member

@mjdietzx your questions about the IsBDBFile are good but I think not directly relevant to this PR, which is just moving the IsBDBFile function from one file to another, not changing it at all.

Viewing individual commits https://github.com/bitcoin/bitcoin/pull/20275/commits/ may be clearer than viewing the consolidated diff. Any case, I’ll try follow up with answers soon.

jonatack commented at 10:56 pm on November 1, 2020: member

Concept ACK

achow101 commented at 4:59 pm on November 2, 2020: member

ACK e21dc6c05c02a0da18b9af2c0eb1d7996c378541

promag commented at 6:23 pm on November 2, 2020: member

Tested ACK e21dc6c05c02a0da18b9af2c0eb1d7996c378541. Tested listwalletdir and loadwallet with a wallet in walletdir and with an external wallet.

Startup fails if any wallets -wallet or settings.json is sqlite.

ryanofsky commented at 2:50 pm on November 4, 2020: member

This has two ACKS so I didn’t make any tweaks. Just responding to review comments

MarcoFalke commented at 3:05 pm on November 4, 2020: member

Is this for 0.21?

ryanofsky commented at 5:01 pm on November 4, 2020: member

Is this for 0.21?

I’m not requesting it. I see this mostly as a code cleanup. For users, the only ones who would be affected would be those compiling from scratch and disabling sqlite, or maybe those using gentoo or nixos type packages with build customizations. The effect for these users at worst would be some confusion seeing incomplete wallet listings, or seeing scarier “Data is not in recognized format” error in place of “Build does not support SQLite database format”

promag commented at 10:54 pm on November 4, 2020: member

or seeing scarier “Data is not in recognized format”

The same error would also happen if the the user attempts to open sqlite wallet with previous versions.

MarcoFalke commented at 6:59 am on November 5, 2020: member

Also, it would be hard to create a sqlite wallet with a non-sqlite build. Assigning 22.0 milestone

MarcoFalke added this to the milestone 22.0 on Nov 5, 2020

DrahtBot added the label Needs rebase on Nov 12, 2020

ryanofsky force-pushed on Nov 13, 2020

DrahtBot removed the label Needs rebase on Nov 13, 2020

ryanofsky commented at 3:33 pm on November 13, 2020: member

Rebased e21dc6c05c02a0da18b9af2c0eb1d7996c378541 -> 417e95a97b4e00a9add97d94ce3b9c38904a354b (pr/exist.1 -> pr/exist.2, compare) due to conflict with #19502 Rebased 417e95a97b4e00a9add97d94ce3b9c38904a354b -> 727e0aad58b41facf55bbedfc5de45b2c41a98ae (pr/exist.2 -> pr/exist.3, compare) due to conflict with #18836

in src/wallet/walletdb.cpp:1071 in 417e95a97b outdated

1058-#else
1059-    assert(format != DatabaseFormat::SQLITE);
1060 #endif
1061+        error = Untranslated(strprintf("Failed to load database path '%s'. Build does not support SQLite database format.", path.string()));
1062+        status = DatabaseStatus::FAILED_BAD_FORMAT;
1063+        return nullptr;

luke-jr commented at 4:47 pm on November 13, 2020:

Prefer to keep unreachable code under an #else here

ryanofsky commented at 5:09 pm on November 13, 2020:

Prefer to keep unreachable code under an #else here

Prefer not to so there is less unnecessary variation between build configurations, and so code is checked by the compiler.

luke-jr changes_requested

DrahtBot added the label Needs rebase on Nov 16, 2020

ryanofsky force-pushed on Nov 16, 2020

DrahtBot removed the label Needs rebase on Nov 16, 2020

DrahtBot added the label Needs rebase on Nov 23, 2020

MOVEONLY: Move IsBDBFile, IsSQLiteFile, and ListWalletDir

This commit does not change to any code and behavior. It it is easily reviewed
with the --color-moved=dimmed_zebra git diff option.

Motivation for this change is to:

- Consolidate redundant functions
  IsBDBFile /ExistsBerkeleyDatabase / SplitWalletPath, and
  IsSQLiteFile / ExistsSQLiteDatabase in the next commits

- Detect SQLite wallets consistently regardless whether bitcoin is built with
  SQLite support in the next commits

- Avoid attempting to open SQLite databases with the BDB library when bitcoin
  is built without SQLite support in the next commits

5aaeb6cf87

refactor: Replace ListWalletDir() function with ListDatabases()

No change to behavior. This is just cleanup after previous MOVEONLY commit to
make db.h list function fit conventions of surrounding functions.

6ee9cbdd18

refactor: Drop call to GetWalletEnv in wallet salvage code

No observable change in behavior. This just avoids a redundant environment
lookup. Motivation is to be able to simplify the GetWalletEnv implementation in
an upcoming commit.

6a7a63644c

refactor: Consolidate redundant wallet database path and exists functions

No change in behavior. Just remove a little bit of code, reduce macro usage,
remove duplicative functions, and make BDB and SQLite implementations more
consistent with each other.

d70dc89e78

wallet: List all wallets in non-SQLite or non-BDB builds

This commit does not change behavior when bitcoin is built normally with both
the SQLite and BDB libraries. It just makes non-SQLite and non-BDB builds
more similar to the normal build. Specifically:

- It makes wallet directory lists always include all wallets so wallets don't
  appear missing depending on the build.

- It now triggers specific "Build does not support SQLite database format" and
  "Build does not support Berkeley DB database format" errors if a wallet can't
  be loaded instead of the more ambiguous and scary "Data is not in recognized
  format" error.

f3d870fc22

ryanofsky force-pushed on Dec 4, 2020

ryanofsky renamed this:
~~wallet: List SQLite wallets in non-SQLite builds~~
wallet: List all wallets in non-SQLite and non-BDB builds
on Dec 4, 2020

ryanofsky commented at 9:13 pm on December 4, 2020: member

Rebased 727e0aad58b41facf55bbedfc5de45b2c41a98ae -> f3d870fc2271bf45e0269e5ae135bced1a26f620 (pr/exist.3 -> pr/exist.4, compare) due to conflict with #20202. #20202 added some new code that was originally part of this PR so this PR is a little simpler now.

DrahtBot removed the label Needs rebase on Dec 4, 2020

jonasschnelli commented at 8:29 am on December 7, 2020: contributor

Concept ACK

ryanofsky commented at 3:36 pm on December 11, 2020: member

@achow101 @promag you ACKed this previously and might be interested to reACK. There haven’t been any changes except rebasing on top of #20202 making this PR a little smaller (new MakeDatabase code originally added here got added in #20202 instead)

achow101 commented at 5:50 pm on December 11, 2020: member

ACK f3d870fc2271bf45e0269e5ae135bced1a26f620

promag commented at 11:19 pm on December 11, 2020: member

Tested ACK f3d870fc2271bf45e0269e5ae135bced1a26f620. Tested a –without-sqlite build with sqlite wallets.

Startup fails if any wallets -wallet or settings.json is sqlite.

This still happens - specifying sqlite wallet on a –without-sqlite build at command line (or settings.json) leads to startup failure. Not sure if this is expected or if it should just ignore it. Note that startup succeeds if you specify a wallet that doesn’t exists.

ryanofsky commented at 0:24 am on December 12, 2020: member

This still happens - specifying sqlite wallet on a –without-sqlite build at command line (or settings.json) leads to startup failure. Not sure if this is expected or if it should just ignore it. Note that startup succeeds if you specify a wallet that doesn’t exists.

Does the PR cause this somehow? Or is this a preexisting bug not related to the PR?

promag commented at 0:28 am on December 12, 2020: member

Existing behavior is to fail startup if -wallet points to a unknown file format. In this case it’s a known but unsupported file format. I’m fine with both outcomes.

ryanofsky commented at 0:44 am on December 12, 2020: member

I don’t understand what’s being reported still. AFAIK, before this PR this case failed at startup, and after this PR it still fails at startup. But before this PR the error message was misleading, and after this PR the error message is more accurate.

If the only thing that’s changing is the error message, that’s good to know and thanks for testing it. But if something else is changing here besides the message, that’s not intended, and is probably something that should be fixed here or at least noted in the PR description.

promag commented at 0:50 am on December 12, 2020: member

That’s my question - If it should remain a startup error of if it should be ignored since now it’s a known wallet format wallet. If the user loads a sqlite wallet, it’s saved on settings, then restart with a –without-sqlite build will always fail.

ryanofsky commented at 1:10 am on December 12, 2020: member

That’s my question - If it should remain a startup error of if it should be ignored since now it’s a known wallet format wallet. If the user loads a sqlite wallet, it’s saved on settings, then restart with a –without-sqlite build will always fail.

These are different questions. I’m asking what does happen, and you are asking what should happen. It sounds like in the last sentence you are confirming that the PR description here is accurate, and the PR is behaving as expected, and that only change before this PR and after this PR is that some error text is slightly improved.

If this is correct, then the answer to your question about what should happen is a matter of opinion. My opinion is that any of the 4 alternatives in https://github.com/bitcoin-core/gui/issues/95#issuecomment-694236940 would be a good solution. I like alternative 4 in that list the best, but there a lot of good options. But this issue seems only vaguely related to this PR if the only effect of this PR is tweaking and improving the error text, so it probably should be discussed separately.

promag commented at 1:35 am on December 12, 2020: member

you are confirming that the PR description here is accurate, and the PR is behaving as expected

Yes, that’s what I ACK’ed above.

Ah I keep forgetting bitcoin-core/gui#95 :weary: thanks for pointing it.

ryanofsky commented at 1:39 am on December 12, 2020: member

Thanks for testing and clarifying! Sorry I didn’t understand right away.

MarcoFalke merged this on Dec 12, 2020

MarcoFalke closed this on Dec 12, 2020

sidhujag referenced this in commit 35acbc4a96 on Dec 13, 2020

DrahtBot locked this on Feb 15, 2022

wallet: List all wallets in non-SQLite and non-BDB builds #20275

Conflicts